-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[webview_flutter_web] Add WebWebViewController.runJavaScript #9835
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Code Review
This pull request introduces the runJavaScript
method for the web implementation of WebView
and refactors loadHtmlString
to better support the baseUrl
parameter. The changes are generally good, but there are a few areas for improvement regarding event listener management and robustness against race conditions. I've identified a potential memory leak in loadHtmlString
and a high-severity issue in _getIFrameDocument
that could lead to an infinite loop. I've provided suggestions to address these issues by using one-time event listeners and replacing polling with a more robust event-based approach.
packages/webview_flutter/webview_flutter_web/lib/src/web_webview_controller.dart
Show resolved
Hide resolved
_webWebViewParams.iFrame.addEventListener( | ||
'load', | ||
() { | ||
_webWebViewParams.iFrame.contentDocument?.write(html.toJS); | ||
loading.complete(); | ||
}.toJS); |
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.
The load
event listener is added every time loadHtmlString
is called but is never removed. This can lead to a memory leak if this method is called multiple times on the same controller, as multiple listeners will be attached.
To ensure the listener is only called once per loadHtmlString
call, you can use the once
option in addEventListener
. This will automatically remove the listener after it has been invoked.
_webWebViewParams.iFrame.addEventListener(
'load',
(web.Event _) {
_webWebViewParams.iFrame.contentDocument?.write(html.toJS);
loading.complete();
}.toJS,
AddEventListenerOptions(once: true),
);
packages/webview_flutter/webview_flutter_web/lib/src/web_webview_controller.dart
Outdated
Show resolved
Hide resolved
From triage: @sophiebremer Are you planning on updating this PR to have graceful fallback to a clear error when run against anything other than a same-origin iframe, instead of crashing and/or hanging? |
@stuartmorgan-g I will add the |
83b4252
to
10e39dd
Compare
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
…er_packages into runJavaScript
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
This PR adds an implementation for
WebWebViewController.runJavaScript
and changes the wayWebWebViewController.loadHtmlString
injects HTML code while also adding support for thebaseUrl
option.The following issues are fixed:
runJavaScript
for Web when the content has the same origin flutter#173899Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3