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

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

Merged
amirh merged 5 commits into from
May 29, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7cdea8
platform_interface: Use Dart objects for creationParams and webSettings.
amirh May 22, 2019
1b8e20f
review comments followup
amirh May 22, 2019
f18e066
format
amirh May 22, 2019
3b2bc5f
add more non null assertions
amirh May 22, 2019
b05a5c6
review comment followup
amirh May 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 74 additions & 1 deletion packages/webview_flutter/lib/platform_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import 'package:flutter/foundation.dart';
import 'package:flutter/gestures.dart';
import 'package:flutter/widgets.dart';

import 'webview_flutter.dart';

/// Interface for talking to the webview's platform implementation.
///
/// An instance implementing this interface is passed to the `onWebViewPlatformCreated` callback that is
Expand All @@ -29,6 +31,15 @@ abstract class WebViewPlatform {
"WebView loadUrl is not implemented on the current platform");
}

/// 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.
Future<void> updateSettings(WebSettings setting) {
throw UnimplementedError(
"WebView updateSettings is not implemented on the current platform");
}

// As the PR currently focus about the wiring I've only moved loadUrl to the new way, so
// the discussion is more focused.
// In this temporary state WebViewController still uses a method channel directly for all other
Expand All @@ -37,6 +48,68 @@ abstract class WebViewPlatform {
int get id;
}

/// Settings for configuring a WebViewPlatform.
///
/// Initial settings are passed as part of [CreationParams], settings updates are sent with
/// [WebViewPlatform#updateSettings].
class WebSettings {
WebSettings({
this.javascriptMode,
this.hasNavigationDelegate,
this.debuggingEnabled,
});

/// The JavaScript execution mode to be used by the webview.
final JavascriptMode javascriptMode;
Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019

Choose a reason for hiding this comment

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

nit: dart doc?

Copy link
Contributor Author

@amirh amirh May 22, 2019

Choose a reason for hiding this comment

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

done


/// Whether the [WebView] has a [NavigationDelegate] set.
final bool hasNavigationDelegate;
Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019

Choose a reason for hiding this comment

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

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


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

@nkoroste nkoroste May 22, 2019

Choose a reason for hiding this comment

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

nit: isDebuggingEnabled

Copy link
Contributor Author

@amirh amirh May 22, 2019

Choose a reason for hiding this comment

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

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.


@override
String toString() {
return 'WebSettings(javascriptMode: $javascriptMode, hasNavigationDelegate: $hasNavigationDelegate, debuggingEnabled: $debuggingEnabled)';
}
}

/// Configuration to use when creating a new [WebViewPlatform].
class CreationParams {
CreationParams(
{this.initialUrl, this.webSettings, this.javascriptChannelNames});

/// The initialUrl to load in the webview.
///
/// When null the webview will be created without loading any page.
final String initialUrl;

/// The initial [WebSettings] for the new webview.
///
/// This can later be updated with [WebViewPlatform.updateSettings].
final WebSettings webSettings;

/// The initial set of JavaScript channels that are configured for this webview.
///
/// For each value in this set the platform's webview should make sure that a corresponding
/// property with a postMessage method is set on `window`. For example for a JavaScript channel
/// named `Foo` it should be possible for JavaScript code executing in the webview to do
///
/// ```javascript
/// Foo.postMessage('hello');
/// ```
// TODO(amirh): describe what should happen when postMessage is called once that code is migrated
// to PlatformWebView.
final Set<String> javascriptChannelNames;

@override
String toString() {
return '$runtimeType(initialUrl: $initialUrl, settings: $webSettings, javascriptChannelNames: $javascriptChannelNames)';
}
}

typedef WebViewPlatformCreatedCallback = void Function(
WebViewPlatform webViewPlatform);

Expand Down Expand Up @@ -67,7 +140,7 @@ abstract class WebViewBuilder {
// TODO(amirh): convert this to be the actual parameters.
// I'm starting without it as the PR is starting to become pretty big.
// I'll followup with the conversion PR.
Map<String, dynamic> creationParams,
CreationParams creationParams,
WebViewPlatformCreatedCallback onWebViewPlatformCreated,
Set<Factory<OneSequenceGestureRecognizer>> gestureRecognizers,
});
Expand Down
5 changes: 3 additions & 2 deletions packages/webview_flutter/lib/src/webview_android.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class AndroidWebViewBuilder implements WebViewBuilder {
@override
Widget build({
BuildContext context,
Map<String, dynamic> creationParams,
CreationParams creationParams,
WebViewPlatformCreatedCallback onWebViewPlatformCreated,
Set<Factory<OneSequenceGestureRecognizer>> gestureRecognizers,
}) {
Expand All @@ -46,7 +46,8 @@ class AndroidWebViewBuilder implements WebViewBuilder {
// we explicitly set it here so that the widget doesn't require an ambient
// directionality.
layoutDirection: TextDirection.rtl,
creationParams: creationParams,
creationParams:
MethodChannelWebViewPlatform.creationParamsToMap(creationParams),
creationParamsCodec: const StandardMessageCodec(),
),
);
Expand Down
5 changes: 3 additions & 2 deletions packages/webview_flutter/lib/src/webview_cupertino.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class CupertinoWebViewBuilder implements WebViewBuilder {
@override
Widget build({
BuildContext context,
Map<String, dynamic> creationParams,
CreationParams creationParams,
WebViewPlatformCreatedCallback onWebViewPlatformCreated,
Set<Factory<OneSequenceGestureRecognizer>> gestureRecognizers,
}) {
Expand All @@ -32,7 +32,8 @@ class CupertinoWebViewBuilder implements WebViewBuilder {
onWebViewPlatformCreated(MethodChannelWebViewPlatform(id));
},
gestureRecognizers: gestureRecognizers,
creationParams: creationParams,
creationParams:
MethodChannelWebViewPlatform.creationParamsToMap(creationParams),
creationParamsCodec: const StandardMessageCodec(),
);
}
Expand Down
40 changes: 40 additions & 0 deletions packages/webview_flutter/lib/src/webview_method_channel.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,46 @@ class MethodChannelWebViewPlatform implements WebViewPlatform {
});
}

@override
Future<void> updateSettings(WebSettings settings) {
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
final Map<String, dynamic> updatesMap = _webSettingsToMap(settings);
if (updatesMap.isEmpty) {
Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019

Choose a reason for hiding this comment

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

Another option could be assert(!updatesMap.isEmpty)

Copy link
Contributor Author

@amirh amirh May 22, 2019

Choose a reason for hiding this comment

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

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.

return null;
}
return _channel.invokeMethod('updateSettings', updatesMap);
}

static Map<String, dynamic> _webSettingsToMap(WebSettings settings) {
final Map<String, dynamic> map = <String, dynamic>{};
void _addIfNonNull(String key, dynamic value) {
if (value == null) {
return;
}
map[key] = value;
}

_addIfNonNull('jsMode', settings.javascriptMode?.index);
_addIfNonNull('hasNavigationDelegate', settings.hasNavigationDelegate);
_addIfNonNull('debuggingEnabled', settings.debuggingEnabled);
return map;
}

/// Converts a [CreationParams] object to a map as expected by `platform_views` channel.
///
/// This is used for the `creationParams` argument of the platform views created by
/// [AndroidWebViewBuilder] and [CupertinoWebViewBuilder].
static Map<String, dynamic> creationParamsToMap(
CreationParams creationParams) {
return <String, dynamic>{
'initialUrl': creationParams.initialUrl,
'settings': _webSettingsToMap(creationParams.webSettings),
'javascriptChannelNames': creationParams.javascriptChannelNames.toList(),
};
}

@override
int get id => _id;
}
137 changes: 52 additions & 85 deletions packages/webview_flutter/lib/webview_flutter.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,9 @@ class _WebViewState extends State<WebView> {
Widget build(BuildContext context) {
return WebView.platformBuilder.build(
context: context,
creationParams: _CreationParams.fromWidget(widget).toMap(),
onWebViewPlatformCreated: _onWebViewPlatformCreated,
gestureRecognizers: widget.gestureRecognizers,
creationParams: _creationParamsfromWidget(widget),
);
}

Expand Down Expand Up @@ -307,85 +307,57 @@ class _WebViewState extends State<WebView> {
}
}

Set<String> _extractChannelNames(Set<JavascriptChannel> channels) {
final Set<String> channelNames = channels == null
// TODO(iskakaushik): Remove this when collection literals makes it to stable.
// ignore: prefer_collection_literals
? Set<String>()
: channels.map((JavascriptChannel channel) => channel.name).toSet();
return channelNames;
CreationParams _creationParamsfromWidget(WebView widget) {
return CreationParams(
initialUrl: widget.initialUrl,
webSettings: _webSettingsFromWidget(widget),
javascriptChannelNames: _extractChannelNames(widget.javascriptChannels),
);
}

class _CreationParams {
_CreationParams(
{this.initialUrl, this.settings, this.javascriptChannelNames});

static _CreationParams fromWidget(WebView widget) {
return _CreationParams(
initialUrl: widget.initialUrl,
settings: _WebSettings.fromWidget(widget),
javascriptChannelNames:
_extractChannelNames(widget.javascriptChannels).toList(),
);
}

final String initialUrl;

final _WebSettings settings;

final List<String> javascriptChannelNames;

Map<String, dynamic> toMap() {
return <String, dynamic>{
'initialUrl': initialUrl,
'settings': settings.toMap(),
'javascriptChannelNames': javascriptChannelNames,
};
}
WebSettings _webSettingsFromWidget(WebView widget) {
return WebSettings(
javascriptMode: widget.javascriptMode,
hasNavigationDelegate: widget.navigationDelegate != null,
debuggingEnabled: widget.debuggingEnabled,
);
}

class _WebSettings {
_WebSettings({
this.javascriptMode,
this.hasNavigationDelegate,
this.debuggingEnabled,
});

static _WebSettings fromWidget(WebView widget) {
return _WebSettings(
javascriptMode: widget.javascriptMode,
hasNavigationDelegate: widget.navigationDelegate != null,
debuggingEnabled: widget.debuggingEnabled,
);
// This method assumes that no fields in `currentValue` are null.
WebSettings _clearUnchangedWebSettings(
WebSettings currentValue, WebSettings newValue) {
assert(currentValue.javascriptMode != null);
assert(currentValue.hasNavigationDelegate != null);
assert(currentValue.debuggingEnabled != null);
assert(newValue.javascriptMode != null);
assert(newValue.hasNavigationDelegate != null);
assert(newValue.debuggingEnabled != null);
JavascriptMode javascriptMode;
bool hasNavigationDelegate;
bool debuggingEnabled;
if (currentValue.javascriptMode != newValue.javascriptMode) {
Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link

@nkoroste nkoroste May 22, 2019

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

@amirh amirh May 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cyanglaz cyanglaz May 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@amirh amirh May 22, 2019

Choose a reason for hiding this comment

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

Added more assertions here

Copy link

@nkoroste nkoroste May 23, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@amirh amirh May 23, 2019

Choose a reason for hiding this comment

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

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.

Copy link

@nkoroste nkoroste May 23, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@amirh amirh May 28, 2019

Choose a reason for hiding this comment

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

Renamed to _clearUnchangedWebSettings

javascriptMode = newValue.javascriptMode;
}

final JavascriptMode javascriptMode;
final bool hasNavigationDelegate;
final bool debuggingEnabled;

Map<String, dynamic> toMap() {
return <String, dynamic>{
'jsMode': javascriptMode.index,
'hasNavigationDelegate': hasNavigationDelegate,
'debuggingEnabled': debuggingEnabled,
};
if (currentValue.hasNavigationDelegate != newValue.hasNavigationDelegate) {
hasNavigationDelegate = newValue.hasNavigationDelegate;
}
if (currentValue.debuggingEnabled != newValue.debuggingEnabled) {
debuggingEnabled = newValue.debuggingEnabled;
}

Map<String, dynamic> updatesMap(_WebSettings newSettings) {
final Map<String, dynamic> updates = <String, dynamic>{};
if (javascriptMode != newSettings.javascriptMode) {
updates['jsMode'] = newSettings.javascriptMode.index;
}
if (hasNavigationDelegate != newSettings.hasNavigationDelegate) {
updates['hasNavigationDelegate'] = newSettings.hasNavigationDelegate;
}

if (debuggingEnabled != newSettings.debuggingEnabled) {
updates['debuggingEnabled'] = newSettings.debuggingEnabled;
}
return WebSettings(
javascriptMode: javascriptMode,
hasNavigationDelegate: hasNavigationDelegate,
debuggingEnabled: debuggingEnabled);
}

return updates;
}
Set<String> _extractChannelNames(Set<JavascriptChannel> channels) {
final Set<String> channelNames = channels == null
// TODO(iskakaushik): Remove this when collection literals makes it to stable.
// ignore: prefer_collection_literals
? Set<String>()
: channels.map((JavascriptChannel channel) => channel.name).toSet();
return channelNames;
}

/// Controls a [WebView].
Expand All @@ -398,7 +370,7 @@ class WebViewController {
this._platformInterface,
this._widget,
) : _channel = MethodChannel('plugins.flutter.io/webview_$id') {
_settings = _WebSettings.fromWidget(_widget);
_settings = _webSettingsFromWidget(_widget);
_updateJavascriptChannelsFromSet(_widget.javascriptChannels);
_channel.setMethodCallHandler(_onMethodCall);
}
Expand All @@ -407,7 +379,7 @@ class WebViewController {

final WebViewPlatform _platformInterface;

_WebSettings _settings;
WebSettings _settings;

WebView _widget;

Expand Down Expand Up @@ -549,20 +521,15 @@ class WebViewController {

Future<void> _updateWidget(WebView widget) async {
_widget = widget;
await _updateSettings(_WebSettings.fromWidget(widget));
await _updateSettings(_webSettingsFromWidget(widget));
await _updateJavascriptChannels(widget.javascriptChannels);
}

Future<void> _updateSettings(_WebSettings setting) async {
final Map<String, dynamic> updateMap = _settings.updatesMap(setting);
if (updateMap == null || updateMap.isEmpty) {
return null;
}
_settings = setting;
// TODO(amirh): remove this on when the invokeMethod update makes it to stable Flutter.
// https://github.com/flutter/flutter/issues/26431
// ignore: strong_mode_implicit_dynamic_method
return _channel.invokeMethod('updateSettings', updateMap);
Future<void> _updateSettings(WebSettings newSettings) {
final WebSettings update =
_clearUnchangedWebSettings(_settings, newSettings);
_settings = newSettings;
return _platformInterface.updateSettings(update);
}

Future<void> _updateJavascriptChannels(
Expand Down
Loading