Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[webview_flutter] Refactored creation of Android WebView for testability. #4178

Merged

Conversation

mvanbeusekom
Copy link
Contributor

Currently the constructor of the Java FlutterWebView is populated with several lines of code that initialise and configure the Android WebView control. This has two major drawback:

  1. The code becomes difficult to understand;
  2. The code is extremely hard to test as there is no good way to mock the WebView control.

This PR solves this problem by introducing a WebViewBuilder which is responsible for correctly creating the right Android WebView control and applying the necessary configuration.

The following PRs will directly benefit from this change:

If 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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
    • No release needed as nothing changed in the behaviour of the plugin. Simply a refactor of code structure to make the code easier to test for later features (see PRs who will benefit).
  • I updated CHANGELOG.md to add a description of the change.
    • No release needed as nothing changed in the behaviour of the plugin. Simply a refactor of code structure to make the code easier to test for later features (see PRs who will benefit).
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@google-cla google-cla bot added the cla: yes label Jul 21, 2021
@github-actions github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-android labels Jul 21, 2021
*/
@VisibleForTesting
static WebView createWebView(
WebViewBuilder webViewBuilder, Map<String, Object> params, WebChromeClient webChromeClient) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@renefloor renefloor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mvanbeusekom mvanbeusekom added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Jul 22, 2021
@fluttergithubbot fluttergithubbot merged commit e51722c into flutter:master Jul 22, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 27, 2021
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@mvanbeusekom mvanbeusekom deleted the webview/android_webview_builder branch September 21, 2021 09:30
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: webview_flutter Edits files for a webview_flutter plugin platform-android waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants