Allow changing the webview's platform specific implementation #1618

amirh · 2019-05-20T23:58:32Z

The goal of this PR is to allow separate packages to extend the plugin to work on more platforms.
This is done by abstracting all the platform specific logic behind the WebViewPlatform and WebViewBuilder interfaces(in platform_interface.dart), and adding a static WebView.platformBuilder field that can be set to change the the platform specific code.

For example, in order to extend the webview to support Fuchsia, a fuchsia_webview_flutter plugin can implement WebViewBuilder and WebViewPlatform(reference implementations available in webview_android.dart and webview_cupertino.dart).

When running on Fuchsia the plugin(or the app developer if the plugin doesn't have the right hooks) can then set itself as the implementation for the webview by running:

WebView.platformBuilder = FuchsiaWebViewBuilder();

amirh · 2019-05-21T00:39:19Z

packages/webview_flutter/lib/platform_interface.dart

+/// 
+/// An instance implementing this interface is passed to the `onWebViewCreated` callback that is
+/// passed to [WebViewPlatformInterface#onWebViewCreated].
+abstract class WebViewPlatformControllerInterface {


Actually I think we better make this class non-abstract and have a default implementation(that throws?) for all methods, this way a new implementor doesn't have to implement the full API surface all at once (and when we're adding new methods here it's going to take time for implementors to catch-up).

I'll update the PR to make this non abstract tomorrow.

I think it can stay abstract but you can just add default implementation which others will inherit when extending it

what @nkoroste says

ok, added a default implementation that throws.

If the default implementation throws, it's probably best if the default is abstract (so that the analyzer tells you to implement it). Otherwise you risk crashes at runtime and there's no lint to tell you what you need to implement. If it makes sense to have an actual default that doesn't throw, I'd go with that. That way you can get up and going quickly on a new platform.

Hixie · 2019-05-21T00:45:11Z

Note: This PR is not ready to be merged

FWIW, GitHub has a "create PR as WIP" mode when you create the PR that will make the PR appear grey instead of green until you say it's ready.

nkoroste

Overall LGTM

nkoroste · 2019-05-21T00:41:03Z

packages/webview_flutter/lib/platform_interface.dart

+/// 
+/// An instance implementing this interface is passed to the `onWebViewCreated` callback that is
+/// passed to [WebViewPlatformInterface#onWebViewCreated].
+abstract class WebViewPlatformControllerInterface {


nit: maybe "WebViewControllerPlatformInterface

how about WebViewPlatform and FuchsiaWebViewPlatform/AndroidWebViewPlatform/etc?

Ok I think II'm going with:
WebViewPlatform/AndroidWebViewPlatform and then WebViewBuilder/AndroidWebViewBuilder(instead of WebViewPlatformInterface)

This will mean to set it up for Fuchsia we will do:
WebView.platformBuilder = FuchsiaWebViewBuilder();

Do WebViewBuilder and WebViewPlatform need to be distinct?

We have a single WebViewPlatform per a platform webview instance, while a WebViewBuilder can create zero or more WebViewPlatform instances.

I don't see a nice way to merge them(other than adding a webviewId parameter to all the methods and then using a single instance of that merges WebViewPlatform to serve all of the webview instance).

nkoroste · 2019-05-21T00:41:53Z

packages/webview_flutter/lib/platform_interface.dart

+/// [WebView#implementation] controls the platform interface that is used by [WebView].
+/// [WebViewAndroidImplementation] and [WebViewIosImplementation] are the default implementations
+/// for Android and iOS respectively.
+abstract class WebViewPlatformInterface {


nit: maybe move each interface into it's own file to emphasize their isolation

nkoroste · 2019-05-21T00:58:05Z

packages/webview_flutter/lib/platform_interface.dart

+/// 
+/// An instance implementing this interface is passed to the `onWebViewCreated` callback that is
+/// passed to [WebViewPlatformInterface#onWebViewCreated].
+abstract class WebViewPlatformControllerInterface {


I think it can stay abstract but you can just add default implementation which others will inherit when extending it

Hixie · 2019-05-21T17:15:31Z

packages/webview_flutter/lib/platform_interface.dart

+  Future<void> loadUrl(
+      String url,
+      Map<String, String> headers,
+      );


trivial nit: indenting

Hixie · 2019-05-21T17:16:50Z

packages/webview_flutter/lib/src/webview_ios.dart

+import '../platform_interface.dart';
+import 'webview_method_channel.dart';
+
+class WebViewIosImplementation implements WebViewPlatformInterface {


Avoid using Ios in classes, the correct capitalization is iOS. Use Cupertino if necessary.

Hixie · 2019-05-21T17:17:08Z

packages/webview_flutter/lib/src/webview_ios.dart

+class WebViewIosImplementation implements WebViewPlatformInterface {
+  @override
+  Widget build({BuildContext context, Map<String, dynamic> creationParams, WebViewCreatedCallback onWebViewCreated, Set<Factory<OneSequenceGestureRecognizer>> gestureRecognizers}) {
+    return UiKitView(


assert onWebViewCreated isn't null

Hixie · 2019-05-21T17:17:15Z

packages/webview_flutter/lib/src/webview_method_channel.dart

@@ -0,0 +1,33 @@
+import 'dart:async';


missing copyright statement

Hixie · 2019-05-21T17:18:34Z

packages/webview_flutter/lib/webview_flutter.dart

+      return implementation;
+    }
+
+    if (defaultTargetPlatform == TargetPlatform.android) {


prefer using a switch when comparing on an enum. see style guide.

Hixie · 2019-05-21T17:19:00Z

packages/webview_flutter/lib/webview_flutter.dart

+    }
+
+    throw UnsupportedError("Trying to use the default webview implementation for $defaultTargetPlatform but there isn't a default one");
+  }


did you intend to cache the returned value?

Hixie · 2019-05-21T17:23:56Z

packages/webview_flutter/lib/platform_interface.dart

+  int get id;
+}
+
+typedef WebViewCreatedCallback = void Function(WebViewPlatformControllerInterface);


nit: we like to name the arguments in typedefs. In some places, without a name, the type becomes the name (e.g. void bar(Foo) means void bar(dynamic Foo)) so it's better to be consistent throughout.

harryterkelsen · 2019-05-21T18:32:27Z

LGTM for the overall approach.

I like how you are abstracting the "communicate with the platform side" part of the implementation with WebViewPlatformControllerInterface, but if something similar will be required for all plugins with implementations that wish to avoid platform channel overhead, perhaps this idea should be pushed into the framework itself.

I think the problem is that Flutter assumes that communication with the platform-side must be done by encoding the data into bytes, but with the new platform implementations (Fuchsia, web) we know that is not necessarily the case.

amirh · 2019-05-21T22:14:17Z

Looks like we're generally ok with this approach.

I'm going to start landing it, to make review easier I'd rather land this in pieces, this first piece sets up the plumbing, and moves a single method loadUrl to the Dart interface.

I plan to followup with 2 PRs:

Move the rest of the method channel methods behind the Dart interface.
Replace the creationParams map with Dart objects.

As each of these PRs will be a "breaking change" from the current PR, I'm not going to publish the plugin until everything landed.

amirh · 2019-05-21T22:15:10Z

I think this is ready for review now @cyanglaz @nkoroste

nkoroste · 2019-05-21T23:58:34Z

packages/webview_flutter/lib/webview_flutter.dart

 ///
 /// A [WebViewController] instance can be obtained by setting the [WebView.onWebViewCreated]
 /// callback for a [WebView] widget.
 class WebViewController {


is the long term plan to get rid of this class in favor of WebViewPlatform?

This is the public API we expose to our users(vs the public WebViewPlatform which is the API we expose to platform implementors), while the public API for these 2 currently matches I'm not sure we should require it to match.

Also note that renaming WebViewController right now will be a breaking change, I guess we could eventually make WebViewController extend WebViewPlatform if we want to save one indirection layer, and we extract all the utility code(like the logic in _updateJavaScriptChannels) out of it.

I agree that this should not be part of this PR, but in general I think this controller and WebViewPlatform going to look fairly similar so it would be nice if we can unify them

Let's re-discuss it after we move all the methodchannel stuff out of WebViewController?

I'm actually not sure which option I prefer, I can imagine having a value in separating the interface that users are using to control the webview from the interface that platforms need to provide for the plugin to control the webview (e.g I can see us adding some logic in WebViewController that don't belong in the platform implementation).
But I do see your point about the code duplication.

We should decide at the end of this PR series before publishing to avoid a breaking change.

cyanglaz

LGTM with some minor nits.

cyanglaz · 2019-05-22T00:20:14Z

packages/webview_flutter/lib/platform_interface.dart

+  ///
+  /// `url` must not be null.
+  ///
+  /// Throws an ArgumentError if `url` is not a valid URL string.


How do we enforce the specific platform implementation to honor the parameter constraints mentioned here?

Not sure if we can enforce it. The closet thing we have is the e2e test which we should hope implementors of new platforms will run.

cyanglaz · 2019-05-22T00:29:48Z

packages/webview_flutter/lib/platform_interface.dart

+/// [AndroidWebViewPlatform] and [CupertinoWebViewPlatform] are the default implementations
+/// for Android and iOS respectively.
+abstract class WebViewBuilder {
+  /// Builds a new WebView.


Nit: Should have an extra space above the dart doc?

Should we? I wasn't aware that we enforce something like that, I can't find anything about it in the style guide, are we enforcing this convention?

I asked because it seems other places in this file is doing that. If it is not enforced then I think we are ok. However, I do think it is better to keep the style consistent throughout a file or even a package.

Not sure I understand, how can weI enforce that "onWebViewPlatformCreated will be invoked after the platform specific [WebViewPlatform] implementation is created with the [WebViewPlatform] instance as a parameter."

nkoroste

Overall LGTM

nkoroste · 2019-05-22T17:49:24Z

packages/webview_flutter/lib/platform_interface.dart

+/// [WebView#platformBuilder] controls the builder that is used by [WebView].
+/// [AndroidWebViewPlatform] and [CupertinoWebViewPlatform] are the default implementations
+/// for Android and iOS respectively.
+abstract class WebViewBuilder {


nit: still think you should move this to it's own file

I'd prefer to extract it if this file ever becomes too big, I kind of like the idea of having a single file that we can point implementors of new platforms to where everything they need to implement is specified...

If you feel strongly about it I'll split it now.

nkoroste · 2019-05-22T17:51:39Z

packages/webview_flutter/lib/webview_flutter.dart

 ///
 /// A [WebViewController] instance can be obtained by setting the [WebView.onWebViewCreated]
 /// callback for a [WebView] widget.
 class WebViewController {


I agree that this should not be part of this PR, but in general I think this controller and WebViewPlatform going to look fairly similar so it would be nice if we can unify them

…ce (#1645) This is a followup for #1618 and #1624, and moves all of the plugins.flutter.io/webview method channel calls behind the platform channel, which allows a third party package to provide a new platform implementation for all of these methods. See the description for #1618 for more details. The last remaining part is the plugins.flutter.io/cookie_manager channel which I'm leaving for a followup PR.

This is a followup for #1618, #1624, and #1645, and moves the plugins.flutter.io/cookie_manager method channel behind the platform interface which allows a third party package to provide a new platform implementation the cooke manager. See the description for #1618 for more details. Following this PR all platform specific code can be replaced by an external package.

Allow changing the webview's platform specific implementation

90a8828

googlebot added the cla: yes label May 20, 2019

amirh commented May 21, 2019

View reviewed changes

nkoroste reviewed May 21, 2019

View reviewed changes

Hixie reviewed May 21, 2019

View reviewed changes

amirh added 4 commits May 21, 2019 13:38

initial review comments followup

b78c98f

add unit tests

5eebd7c

update TODO

3cfd04d

add docs

07d892b

amirh force-pushed the multi_platform_webview branch from e0b0ee5 to 07d892b Compare May 21, 2019 22:12

amirh changed the title ~~[WIP] Allow changing the webview's platform specific implementation~~ Allow changing the webview's platform specific implementation May 21, 2019

amirh requested a review from cyanglaz May 21, 2019 22:14

nkoroste reviewed May 21, 2019

View reviewed changes

cyanglaz approved these changes May 22, 2019

View reviewed changes

nkoroste approved these changes May 22, 2019

View reviewed changes

amirh merged commit 0e5933c into flutter:master May 22, 2019

amirh deleted the multi_platform_webview branch May 22, 2019 19:21

amirh mentioned this pull request May 29, 2019

[webview_flutter] Move the method channel behind the platform interface #1645

Merged

amirh mentioned this pull request May 30, 2019

Move clearCookies behind the platform abstraction #1663

Merged

Allow changing the webview's platform specific implementation #1618

Allow changing the webview's platform specific implementation #1618

Uh oh!

Conversation

amirh commented May 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Hixie commented May 21, 2019

Uh oh!

nkoroste 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!

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!

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!

harryterkelsen commented May 21, 2019

Uh oh!

amirh commented May 21, 2019

Uh oh!

amirh commented May 21, 2019

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!

cyanglaz left a comment

Choose a reason for hiding this comment

amirh commented May 20, 2019 •

edited

Loading