-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-1931 stopTrackingDatadogEvents added to avoid memory leaks #745
Conversation
Without removing ScriptMessageHandlers explicitly, they stay alive in memory even after deallocation of the webview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I've minor suggestions tho
/// Disables Datadog iOS SDK and Datadog Browser SDK integration. | ||
/// | ||
/// Removes Datadog's ScriptMessageHandler and UserScript from the caller. | ||
/// _NOTE:_ This method **must** be called when the webview can be deinitialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't render properly, better to use
/// _NOTE:_ This method **must** be called when the webview can be deinitialized. | |
/// - Note: This method **must** be called when the webview can be deinitialized. |
func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/nit
func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) { | |
return | |
} | |
func userContentController(_ userContentController: WKUserContentController, didReceive message: WKScriptMessage) { } |
|
||
controller.trackDatadogEvents(in: []) | ||
|
||
let componentCount = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make it 10? I guess adding a single or a couple of other scripts will cover this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 It would be also good to add stopTrackingDatadogEvents()
in "Webviews integration" screen in Example app. Otherwise we don't have any use of this API outside unit tests.
c43e372
to
deb2ddd
Compare
What and why?
Without removing
WKScriptMessageHandlers
explicitly, they stay alive in memory even after deallocation of the webviewHow?
By calling
stopTrackingDatadogEvents
, our users can remove Datadog components from theirWKWebViewConfiguration
instances so thatDatadogMessageHandler
can be deallocated.Review checklist