-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[webview_flutter] Refactored creation of Android WebView for testability. #4178
[webview_flutter] Refactored creation of Android WebView for testability. #4178
Conversation
| */ | ||
| @VisibleForTesting | ||
| static WebView createWebView( | ||
| WebViewBuilder webViewBuilder, Map<String, Object> params, WebChromeClient webChromeClient) { |
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 wonder if we should pass a Map<String, Object> object here. I think a configuration object is better and we can make a static method to map the Map to a configuration object. Might be a good idea to already pass a configuration object to the FlutterWebView constructor.
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 agree, but then we should directly pass it into the FlutterWebView constructor. I can make that happen.
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.
Looking into this more detailed, changing this to a configuration object requires quite a large refactor as the params also contain a second Map<String, Object> collection containing several web settings.
This means we need to change a lot of code that currently validates how to handle different situations (bases on if a key is part of the params or web settings hashmap or not). These changes are not relevant for the problem this PR is trying to solve. So for now I think it would be better to leave it as is and maybe do a separate PR on updating this if needed.
.../webview_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterWebView.java
Outdated
Show resolved
Hide resolved
.../webview_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewBuilder.java
Outdated
Show resolved
Hide resolved
...view_flutter/android/src/test/java/io/flutter/plugins/webviewflutter/WebViewBuilderTest.java
Show resolved
Hide resolved
renefloor
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!
Currently the constructor of the Java
FlutterWebViewis populated with several lines of code that initialise and configure the Android WebView control. This has two major drawback:This PR solves this problem by introducing a
WebViewBuilderwhich is responsible for correctly creating the right Android WebView control and applying the necessary configuration.The following PRs will directly benefit from this change:
loadRequestmethod #4169If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
dart format.)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.