-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Fix bugs in Dart Implementation of the Pigeon API #4461
Fix bugs in Dart Implementation of the Pigeon API #4461
Conversation
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Outdated
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| static bool _flutterApisHaveBeenSetup = false; |
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.
Same.
Would it make sense for all this setup to be done in one central location, perhaps?
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.
I wasn't sure where to do it at. Options that I was considering were:
- In the constructor of
WebViewsince it is the central class of the plugin. However, this pattern wouldn't be reproducible for plugins where there wasn't a central class. - In the constructor of
AndroidWebView. However, a user may want to use theandroid_webviewAPI without using the shared platform interface. - It could be done in the plugin registration when it's ready, so this would just be temporary. I could add a
TODOif this should be the case. - The current solution. I like this one because it only sets the Flutter API when a user starts to use a WebView. I remember that there is a common complaint that Flutter startup is slow because plugin initialization is done for every plugin at startup. This solution follows the idea of initializing only once it is being used.
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.
Is there any real benefit for the setup to be incremental at a per-class-within-the-plugin-level though?
Rather than having the same static and if in every single class, you could have a ensurePigeonApis helper in a separate file that does that check and then calls all of the API setup calls. Then each class could call it (unconditionally) where you currently have all of the logic replicated in each class.
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.
That's a good point. At this small of a plugin, it probably has little to no benefit. I created AndroidWebViewFlutterApis.ensureSetUp.
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Outdated
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview_api_impls.dart
Show resolved
Hide resolved
packages/webview_flutter/webview_flutter_android/lib/src/android_webview.dart
Outdated
Show resolved
Hide resolved
stuartmorgan-g
left a comment
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
Split of #4455
The pigeon implementation and current implementation has potential leaking callbacks when a
WebViewis disposed. This is a possible cause of flaky tests for WebView, so this PR attempts to prevent callbacks when the correspondingWebViewobject is disposed. In #4455, the integration tests now seem to pass consistently. Along with fixing the resize test: #4460This also fixes some trivial bugs found though the integration and widget tests in #4455
Java Portion: #4459
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.