[webview_flutter_android] Implementation of Android WebView widget using pigeon #4503

bparrishMines · 2021-11-11T22:30:49Z

Replaces MethodChannelPlatformController with WebViewAndroidPlatformController and WebViewAndroidWidget.

Android portion of flutter/flutter#93732

Pre-launch Checklist

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

…for runjavascript

stuartmorgan-g · 2021-11-12T19:22:39Z

...er_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+
+  @Override
+  public void onAttachedToActivity(@NonNull ActivityPluginBinding activityPluginBinding) {
+    setUp(


Can't onAttachedToActivity be called multiple times? What are the implications of moving platform view registration from the engine attachment to the activity attachment?

And similarly, are the pigeon setup APIs safe to call repeatedly?

We had a similar problem with doing this with google_mobile_ads. It mostly only affect loading views with cached engines. The solution was to set everything up in onAttachedToEngine with binding.applicationContext and then switch the context when the plugin was attached to an Activity. I went ahead and changed this plugin to do the same. I'm not aware of any other problems this implementation causes.

stuartmorgan-g · 2021-11-12T19:23:37Z

...er_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+  }
+
+  @Override
+  public void onDetachedFromActivityForConfigChanges() {}


The activity was passed to WebViewHostApi; doesn't that need to be disconnected somehow at this point?

I switch the context for WebViewHostApi to FlutterPluginBinding.applicationContext() for the explanation I gave above for onAttachedToActivity.

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

stuartmorgan-g · 2021-11-12T19:31:56Z

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

+      _trySetJavaScriptMode(settings.javascriptMode),
+      _trySetDebuggingEnabled(settings.debuggingEnabled),
+      _trySetUserAgent(settings.userAgent),
+      _trySetZoomEnabled(settings.zoomEnabled),


stuartmorgan-g · 2021-11-12T19:33:04Z

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

+        break;
+      default:
+        webView.settings.setMediaPlaybackRequiresUserGesture(true);
+    }


Why not just:
webView.settings.setMediaPlaybackRequiresUserGesture(autoMediaPlaybackPolicy != AutoMediaPlaybackPolicy.always_allow)?

I prefer to use switch statements for enums because the Dart code will throw a lint error when one of the values wasn't listed. We had a bug once when we didn't support all the enum values.

default covers all cases, so you'll never get a linter warning here no matter what is added to the enum.

We had a bug once when we didn't support all the enum values.

If someone is adding new functionality, they should be adding test coverage, which is how we should catch this. Relying on the lint (by removing default to enable it) would make adding new enum values repository-breaking. We've already had a sever issue with this; see flutter/flutter#89866

I don't think flutter/flutter#89866 actually provides a recommendation on the implementation. Are you suggesting that we keep the default value? If the platform interface adds a new enum value, wouldn't only an integration test in the main plugin (e.g. webview_flutter) catch a change like that? The unit tests in the platform interface and main plugin should be platform agnostic.

I think I prefer option 3 from flutter/flutter#89866. That way there is a process to locate everywhere enum used in the implementations. And we can add TODOs to update at a later point or in a followup PR.

I don't think flutter/flutter#89866 actually provides a recommendation on the implementation. Are you suggesting that we keep the default value?

I think in cases where we can have a reasonable default, we should do that, because it reduces the chances of cross-package breakage. (We don't yet have a way to detect that in CI.)

If the platform interface adds a new enum value, wouldn't only an integration test in the main plugin (e.g. webview_flutter) catch a change like that? The unit tests in the platform interface and main plugin should be platform agnostic.

I'm not sure I follow. If someone is adding a new enum value, it's presumably to add new functionality based on that new value. That would generally require a change to each platform implementation package to actually use the new enum value, and those changes should have tests. So it should be something that would be caught by tests both at the platform implementation level and potentially at the app-facing level.

Ah, ok. I assumed someone would be able to make a change to a platform interface enum without needing to update the implementations. If that's the case, then yea we should be able to catch all the uses if we require this.

I changed it to your suggestion

stuartmorgan-g · 2021-11-12T19:34:32Z

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

+    addJavascriptChannels(creationParams.javascriptChannelNames);
+  }
+
+  Future<void> _trySetHasProgressTracking(bool? hasProgressTracking) {


Can we call all of these something more like setFooOrIgnoreNull? I wasn't sure what "try" meant at the call sites.

Alternately, you could make these not take nullables and use conditionals at the call site:

return Future.wait(<Future<void>>[ if (settings.hasProgressTracking != null) _setHasProgressTracking(settings.hasProgressTracking), if (settings.hasNavigationDelegate) _trySetHasNavigationDelegate(settings.hasNavigationDelegate), ...

Done. I decided to go with this solution.

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

stuartmorgan-g · 2021-11-12T19:40:10Z

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

+  }
+
+  Future<void> _trySetZoomEnabled(bool? zoomEnabled) async {
+    if (zoomEnabled == null) return Future<void>.sync(() => null);


This inline condition+statement reminds me: since you're rewriting ~all of the Dart, how hard would it be to flip the analysis options of this package to the new options? It would substantially cut down on later churn.

(To do just this one, you'd move packages/webview_flutter/analysis_option.yaml into all of the subpackages except for this one.)

Done. I updated the analysis options for this plugin

packages/webview_flutter/webview_flutter_android/lib/webview_android_widget.dart

stuartmorgan-g · 2021-11-12T19:43:00Z

Adding @mvanbeusekom for a second review, since this is a substantial rewrite, so would benefit from more eyes.

stuartmorgan-g · 2021-11-15T20:55:34Z

Is this ready for re-review?

mvanbeusekom

I am wondering if it would be possible to replace the create method channel call with the default structure that is offered by the AndroidView widget. See also my comment for the FlutterWebViewFactory.java file.

mvanbeusekom · 2021-11-16T09:13:36Z

...r_android/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterWebViewFactory.java

  @Override
  public PlatformView create(Context context, int id, Object args) {
-    Map<String, Object> params = (Map<String, Object>) args;
-    MethodChannel methodChannel = new MethodChannel(messenger, "plugins.flutter.io/webview_" + id);
-    return new FlutterWebView(context, methodChannel, params, containerView);
+    final PlatformView view = (PlatformView) instanceManager.getInstance((Integer) args);
+    if (view == null) {
+      throw new IllegalStateException("Unable to find WebView instance: " + args);
+    }
+    return view;
  }


Is there is special reason why the Android WebView control is created through the separate create method channel command and not by this PlatformViewFactory?

I understand how it works at the moment but I can't really see the benefit. And since this is a factory class I would expect this class to create the instance and register it with the InstanceManager.

The current implementation also means that one should always make sure the create method is called before the AndroidView widget is added to the widget tree. In general the current Dart implementation will take care of that however it is something that deviates from what I would expect and thus requires extra attention.

To me it seems possible to create the Dart WebView instance and register it with the Dart version of the InstanceManager and only create it on the Java side through this factory. This would remove the need for the create method channel call. In my opinion this would simplify the API a little bit and also makes use of the PlatformViewFactory in a way I would expect it to work (namely, responsible for creating the WebView control).

The main reason I didn't want create to the WebView in the PlatformViewFactory was that I wanted to eventually have an update for flutter/flutter#51875. Where a user would be able to load a URL for a WebView and make calls to scrolling without needing to add the Widget to the widget tree. We probably should actually have the PlatformView API provide support to create the View without adding it to the tree, but I think this is the only way around it at the moment.

Also, the Pigeon API is built to mimic the android.webkit API. My goal was to avoid significant deviation from this because it can make it harder to handle changes to the API later or run into conflicts when adding features we weren't aware of before.

In the long term, it should become a standard to always call create in the constructor of a class and dispose would be handled automatically by garbage collection. This is probably at least 2 quarters away, but the changes to webview_flutter are essentially the stepping stones to get there.

Thank you for elaborating on this @bparrishMines. I indeed see the clear benefit doing it this way if you take flutter/flutter#51875 into account.

bparrishMines · 2021-11-16T18:39:25Z

@stuartmorgan This is ready for another review!

stuartmorgan-g

LGTM with nits

(With the caveat that with a full rewrite like this, it's of course very hard to have confidence that nothing regressed.)

stuartmorgan-g · 2021-11-16T19:33:34Z

packages/webview_flutter/webview_flutter_android/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.0
+
+* Replace platform implementation with API built with pigeon.


Replaces

(Per the new style guide.)

stuartmorgan-g · 2021-11-16T20:27:49Z

...er_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+  @Override
+  public void onAttachedToActivity(@NonNull ActivityPluginBinding activityPluginBinding) {
+    final Activity activity = activityPluginBinding.getActivity();
+    webViewHostApi.setContext(activityPluginBinding.getActivity());


Shouldn't this be activity?

stuartmorgan-g · 2021-11-16T20:29:09Z

...er_android/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+  public void onDetachedFromActivity() {
+    final Context context = pluginBinding.getApplicationContext();
+    webViewHostApi.setContext(context);
+    javaScriptChannelHostApi.setPlatformThreadHandler(new Handler(context.getMainLooper()));


This pattern is repeated four times; it seems like changing this (e.g., adding another thing that needs to be adjusted) would be very error-prone. How about extracting a helper that takes the Context, and then each of these can be a one-liner to call that helper with the right context for the scenario.

mvanbeusekom

LGTM

…idget using pigeon (flutter/plugins#4503)

…ing pigeon (flutter#4503)

bparrishMines added 7 commits November 10, 2021 17:51

implementation of webview_widget

3350c71

remove unused files, add navigation delegate to example, add support …

04079cb

…for runjavascript

working example app

bbbbd76

most tests passing

e87b43e

rearrangement

2ac9967

renames

928b5e7

documentation

556b23d

google-cla bot added the cla: yes label Nov 11, 2021

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-android labels Nov 11, 2021

bparrishMines added 3 commits November 11, 2021 14:33

formatting

66a7167

add initialize meethod

7aad502

fix tests

8724ad5

bparrishMines changed the title ~~Webview widget~~ [webview_flutter_android] Implementation of Android WebView widget using pigeon Nov 12, 2021

bparrishMines added 2 commits November 12, 2021 10:53

fix potential race condition

19c46f7

version bump

ed1b199

bparrishMines requested a review from stuartmorgan-g November 12, 2021 19:11

stuartmorgan-g suggested changes Nov 12, 2021

View reviewed changes

stuartmorgan-g requested a review from mvanbeusekom November 12, 2021 19:42

bparrishMines added 10 commits November 12, 2021 14:23

comments, improve readability

588ef0b

set the activity for actvity dependent apis

c8da74f

use application context

79cdc87

dont set context to null

049afce

fix unit tests

3b49d41

change analysis options

514d16e

dart analysis errors

f472535

fix race condition

642454a

fix passing wrong webview

801bf4d

fix not receiving callback on new window

9d088eb

bparrishMines added 4 commits November 14, 2021 14:08

add onCreateWindow unit test without the docs and formatting

388fdeb

documentation and formatting

9aa2b3d

flutter analyze

9938e2f

change from default to enum

7c392a8

bparrishMines mentioned this pull request Nov 15, 2021

[webview_flutter] - Added WebViewSetting option for iOS #4339

Closed

11 tasks

fix analysis options

9ccb29e

mvanbeusekom reviewed Nov 16, 2021

View reviewed changes

bparrishMines added 2 commits November 16, 2021 15:12

add default to switch statement

9154155

use conditional

0cd4f13

stuartmorgan-g approved these changes Nov 16, 2021

View reviewed changes

bparrishMines added 2 commits November 16, 2021 18:00

Merge branch 'master' of github.com:flutter/plugins into webview_widget

0d59bb7

create resuable method and update changelog

acecf65

mvanbeusekom approved these changes Nov 17, 2021

View reviewed changes

bparrishMines added 3 commits November 17, 2021 11:47

remove print and timeout initial resize

c9e744e

return null on timeout

45c7555

forgot to set an actual value

5b9581f

bparrishMines merged commit 8f55fb6 into flutter:master Nov 17, 2021

bparrishMines deleted the webview_widget branch November 17, 2021 18:37

engine-flutter-autoroll mentioned this pull request Nov 17, 2021

Roll Plugins from 4ccc90493235 to 8f55fb68d0e9 (1 revision) flutter/flutter#93802

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 17, 2021

8f55fb6 [webview_flutter_android] Implementation of Android WebView w…

b9996b6

…idget using pigeon (flutter/plugins#4503)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Dec 11, 2021

[webview_flutter_android] Implementation of Android WebView widget us…

2150d1d

…ing pigeon (flutter#4503)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[webview_flutter_android] Implementation of Android WebView widget us…

3895544

…ing pigeon (flutter#4503)

		@@ -1,3 +1,7 @@
		## 2.3.0

		* Replace platform implementation with API built with pigeon.

[webview_flutter_android] Implementation of Android WebView widget using pigeon #4503

[webview_flutter_android] Implementation of Android WebView widget using pigeon #4503

Uh oh!

Conversation

bparrishMines commented Nov 11, 2021 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartmorgan-g commented Nov 12, 2021

Uh oh!

stuartmorgan-g commented Nov 15, 2021

Uh oh!

mvanbeusekom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines commented Nov 16, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvanbeusekom left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

bparrishMines commented Nov 11, 2021 •

edited by stuartmorgan-g

Loading

bparrishMines Nov 12, 2021 •

edited

Loading

bparrishMines Nov 12, 2021 •

edited

Loading