[webview_flutter] platform_interface: Use Dart objects for creationParams and webSettings. #1624

amirh · 2019-05-22T19:27:21Z

As a temporary step we passed these objects as maps that were sent across the method channel. This PR replaces the maps with proper Dart objects.

amirh · 2019-05-22T19:28:44Z

@nkoroste

cyanglaz

I like this approach, I was planning to do a similar update to the image_picker as well.
Left some comments.

cyanglaz · 2019-05-22T19:53:11Z

packages/webview_flutter/lib/platform_interface.dart

+    this.debuggingEnabled,
+  });
+
+  final JavascriptMode javascriptMode;


nit: dart doc?

cyanglaz · 2019-05-22T19:54:43Z

packages/webview_flutter/lib/platform_interface.dart

+  final JavascriptMode javascriptMode;
+
+  /// Whether a [NavigationDelegate] should be used for this webview.
+  final bool hasNavigationDelegate;


nit(personal opinion): useNavigationDelegate might be a better name in this case.

cyanglaz · 2019-05-22T20:01:29Z

packages/webview_flutter/lib/src/webview_method_channel.dart

+    // https://github.com/flutter/flutter/issues/26431
+    // ignore: strong_mode_implicit_dynamic_method
+    final Map<String, dynamic>  updatesMap = _webSettingsToMap(settings);
+    if (updatesMap.isEmpty) {


Another option could be assert(!updatesMap.isEmpty)

It's actually legit to send a settings with all null fields to this method.

We could potentially move the short-circuting logic to webview_flutter.dart, but that requires some ugliness or maintaining an operator== for WebSettings which I'm hesitant to do from a forward-compatibility point of view(I can see us adding a list field to WebSettings, at which point naive looking webSettings==webSettings2 might actually be expensive.

cyanglaz · 2019-05-22T20:11:48Z

packages/webview_flutter/lib/webview_flutter.dart

+  JavascriptMode javascriptMode;
+  bool hasNavigationDelegate;
+  bool debuggingEnabled;
+  if (currentValue.javascriptMode != newValue.javascriptMode) {


if the newValue.javascriptMode is null and the currentValue.javascriptMode is non-null, we are setting the old value to null. Same as below. It seems to violate the comment here:

/// Updates the webview settings. /// /// Any non null field in `settings` will be set as the new setting value. /// All null fields in `settings` are ignored.

The fields in currentValue should never be null (the initial value built by _WebSettingFromWidget can never have null values).

I made it clear in the method comment and added assertions.

I was thinking when the fields in newValue is null, the current implementation will set the corresponding fields in the currentValue to null. But the dart doc in the updateSetting in the WebViewSettings class mentioned that the null fields should be ignored.

After offline discussion with @amirh; we confirmed that the fields in both currentValue and newValue could never be null because they are both created by _webSettingsFromWidget which takes the setting from the WebView widget, which should never be null. Can possibly add assertions.

Added more assertions here

Sorry I'm a bit confused about this entire method, what it's purpose? It looks to me that the return type will almost always be a copy of newValue unless the fields of each equal, in which case it will return a new WebSettings object with some null fields - is that really what you want?

On the platform side, at least for the platforms we've been dealing with so far, all these "settings" are updated by imperative calls, e.g on Android we call setJavaScriptEnabled to update the "javascriptMode".
We do not want to call setJavaScriptEnabled every time the widget is rebuilt, so we're keeping track of the previous javascriptMode value we had the last time the widget was built, and only when we see that the value has changed we call setJavaScriptEnabled.

The WebSettings class groups these kind of settings. We track the previous WebSettings value, and when the widget is rebuilt this method(_webSettingsUpdate) computes a WebSettings instance that only includes the values that have changed and has to be updated on the platform side. This value is then passed to WebViewPlatform.updateSettings which ignores the null fields and updates the platform webview with the new values for all non-null fields in WebSettings.

I see, so you're nullfying all the unchanged fields on purpose because WebViewPlatform.updateSettings expects it that way. I guess the method name confused me, it's more like nullfyUnchangedValues which is a horrible name I agree. Anyway, thanks for the detailed explanation.

Renamed to _clearUnchangedWebSettings

nkoroste · 2019-05-22T20:39:59Z

packages/webview_flutter/lib/platform_interface.dart

+  /// Whether to enable the platform's webview content debugging tools.
+  ///
+  /// See also: [WebView.debuggingEnabled].
+  final bool debuggingEnabled;


nit: isDebuggingEnabled

I think it makes sense to keep it consistent with the WebView parameter name, if we want to change both it will be a breaking change at this point.

Note that as far as I know the Flutter style guide doesn't prefer an "is" prefix for flags.

nkoroste · 2019-05-22T20:42:15Z

packages/webview_flutter/lib/platform_interface.dart

+
+  /// The initial set of JavaScript channels that are configured for this webview.
+  ///
+  /// For each value in this list the platform's webview should make sure that a corresponding


nit: "in this set"

nkoroste · 2019-05-22T20:45:57Z

packages/webview_flutter/lib/src/webview_method_channel.dart

+  }
+
+
+  static Map<String, dynamic> creationParamsToMap(CreationParams creationParams) {


add dart doc or should it be private?

nkoroste · 2019-05-22T20:50:06Z

packages/webview_flutter/lib/webview_flutter.dart

+  JavascriptMode javascriptMode;
+  bool hasNavigationDelegate;
+  bool debuggingEnabled;
+  if (currentValue.javascriptMode != newValue.javascriptMode) {


amirh

Thanks! PTAL

amirh · 2019-05-22T21:25:33Z

packages/webview_flutter/lib/webview_flutter.dart

+  JavascriptMode javascriptMode;
+  bool hasNavigationDelegate;
+  bool debuggingEnabled;
+  if (currentValue.javascriptMode != newValue.javascriptMode) {


The fields in currentValue should never be null (the initial value built by _WebSettingFromWidget can never have null values).

I made it clear in the method comment and added assertions.

amirh · 2019-05-22T21:37:54Z

packages/webview_flutter/lib/src/webview_method_channel.dart

+    // https://github.com/flutter/flutter/issues/26431
+    // ignore: strong_mode_implicit_dynamic_method
+    final Map<String, dynamic>  updatesMap = _webSettingsToMap(settings);
+    if (updatesMap.isEmpty) {


It's actually legit to send a settings with all null fields to this method.

We could potentially move the short-circuting logic to webview_flutter.dart, but that requires some ugliness or maintaining an operator== for WebSettings which I'm hesitant to do from a forward-compatibility point of view(I can see us adding a list field to WebSettings, at which point naive looking webSettings==webSettings2 might actually be expensive.

amirh · 2019-05-22T21:40:09Z

packages/webview_flutter/lib/src/webview_method_channel.dart

+  }
+
+
+  static Map<String, dynamic> creationParamsToMap(CreationParams creationParams) {


amirh · 2019-05-22T21:40:31Z

packages/webview_flutter/lib/platform_interface.dart

+
+  /// The initial set of JavaScript channels that are configured for this webview.
+  ///
+  /// For each value in this list the platform's webview should make sure that a corresponding


amirh · 2019-05-22T21:41:51Z

packages/webview_flutter/lib/platform_interface.dart

+  /// Whether to enable the platform's webview content debugging tools.
+  ///
+  /// See also: [WebView.debuggingEnabled].
+  final bool debuggingEnabled;


I think it makes sense to keep it consistent with the WebView parameter name, if we want to change both it will be a breaking change at this point.

Note that as far as I know the Flutter style guide doesn't prefer an "is" prefix for flags.

amirh · 2019-05-22T21:43:04Z

packages/webview_flutter/lib/platform_interface.dart

+    this.debuggingEnabled,
+  });
+
+  final JavascriptMode javascriptMode;


cyanglaz

amirh · 2019-05-22T22:51:26Z

Thanks.
Waiting for @nkoroste's approval before merging.

nkoroste · 2019-05-23T23:25:53Z

packages/webview_flutter/lib/webview_flutter.dart

+  JavascriptMode javascriptMode;
+  bool hasNavigationDelegate;
+  bool debuggingEnabled;
+  if (currentValue.javascriptMode != newValue.javascriptMode) {


I see, so you're nullfying all the unchanged fields on purpose because WebViewPlatform.updateSettings expects it that way. I guess the method name confused me, it's more like nullfyUnchangedValues which is a horrible name I agree. Anyway, thanks for the detailed explanation.

As a temporary step we passed these objects as maps that were sent across the method channel. This PR replaces the maps with proper Dart objects.

…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.

googlebot added the cla: yes label May 22, 2019

amirh requested a review from cyanglaz May 22, 2019 19:28

cyanglaz reviewed May 22, 2019

View reviewed changes

nkoroste reviewed May 22, 2019

View reviewed changes

amirh commented May 22, 2019

View reviewed changes

cyanglaz approved these changes May 22, 2019

View reviewed changes

nkoroste approved these changes May 23, 2019

View reviewed changes

amirh added 5 commits May 28, 2019 15:37

platform_interface: Use Dart objects for creationParams and webSettings.

f7cdea8

As a temporary step we passed these objects as maps that were sent across the method channel. This PR replaces the maps with proper Dart objects.

review comments followup

1b8e20f

format

f18e066

add more non null assertions

3b2bc5f

review comment followup

b05a5c6

amirh force-pushed the multi_platform_webview_2 branch from 9cecb36 to b05a5c6 Compare May 28, 2019 22:38

amirh merged commit 4d69253 into flutter:master May 29, 2019

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

		}


		static Map<String, dynamic> creationParamsToMap(CreationParams creationParams) {

[webview_flutter] platform_interface: Use Dart objects for creationParams and webSettings. #1624

[webview_flutter] platform_interface: Use Dart objects for creationParams and webSettings. #1624

Uh oh!

Conversation

amirh commented May 22, 2019

Uh oh!

amirh commented May 22, 2019

Uh oh!

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

cyanglaz May 22, 2019 • 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!

cyanglaz May 22, 2019 • 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!

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!

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

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

amirh commented May 22, 2019

Uh oh!

Choose a reason for hiding this comment

cyanglaz May 22, 2019 •

edited

Loading

cyanglaz May 22, 2019 •

edited

Loading