[webview_flutter_wkwebview] Start work on new ios implementation #4626

bparrishMines · 2021-12-20T17:42:14Z

Start of iOS side of: flutter/flutter#93732

Starts the Dart side of the platform specific implemention for iOS. Created WebViewCupertinoWidgetand added support for WebSettings.allowsInlineMediaPlayback and AutoMediaPlaybackPolicy. This currently has no effect on the working iOS implementation.

The branch that contained all the changes for Dart came out to about 9300 lines of code. This is the first PR to add all the api piece by piece.

No version change:
This is only the initial commit to flutter/flutter#93732 and doesn't make any changes to the current implementation.

No CHANGELOG change: Incremental unused code doesn't need to be noted in the CHANGELOG.

Pre-launch Checklist

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

bparrishMines · 2021-12-20T21:11:32Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+        break;
+    }
+
+    if (_isGreaterOrEqualVersion(await systemVersion, '10.0')) {


An alternative to having this version check logic would be to only support mediaTypesRequiringUserActionForPlayback and handle calling the other methods with platform code. However, I wanted to try a solution of handling version conflicts in Dart. A future feature may require a more complex solution than a 1 liner like this one.

bparrishMines · 2021-12-20T21:16:40Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+/// cannot change those settings dynamically later.
+class WebViewConfiguration {
+  /// Indicates whether HTML5 videos play inline or use the native full-screen controller.
+  set allowsInlineMediaPlayback(bool allow) {


Relevant code from current implementation https://github.com/flutter/plugins/blob/master/packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m#L548

bparrishMines · 2021-12-20T21:17:33Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+  ///
+  /// Use [AudiovisualMediaType.none] to indicate that no user gestures are
+  /// required to begin playing media.
+  set mediaTypesRequiringUserActionForPlayback(


Relevant code from current implementation: https://github.com/flutter/plugins/blob/master/packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m#L568

packages/webview_flutter/webview_flutter_wkwebview/CHANGELOG.md

stuartmorgan-g · 2022-01-14T21:07:33Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+  /// Deprecated for iOS >= 10.0.
+  ///
+  /// Passing false indicates the videos can play automatically.
+  @Deprecated(


Isn't this code going to be entirely internal to the package? We shouldn't deprecate our internal wrappers when we know we'll be using them.

stuartmorgan-g · 2022-01-14T21:08:48Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+    throw UnimplementedError();
+  }
+
+  /// Deprecated for iOS >= 9.0.


That's all we support now, so this is dead code. We just never cleaned up the implementation after changing the min version (I guess we missed it in the scrub).

stuartmorgan-g · 2022-01-14T21:10:46Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+/// app’s interface. For example, if your UI includes forward and back buttons,
+/// connect those buttons to the [goBack] and [goForward] methods of your web
+/// view to trigger the corresponding web navigation. Use the [canGoBack] and
+/// [canGoForward] fields to determine when to enable or disable your buttons.


We should not be duplicating docs wholesale from Apple code.

I would much prefer we say something like:
"Wraps WKWebView. See insert-link-here for documentation".

I was back and forth about to what extent we should document these classes. I think this is a good pattern moving forward. I think this would also pair well with testing that links are up to date. I created an issue for it: flutter/flutter#96974

stuartmorgan-g · 2022-01-14T21:12:29Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+
+  /// The system version of the iOS device.
+  ///
+  /// If null, this value is retrieved asynchronously.


This documentation seems confusing; AFAICT is is describing what clients of this code might do, not anything this class does.

stuartmorgan-g · 2022-01-14T21:13:58Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+  /// If null, this value is retrieved asynchronously.
+  final FutureOr<String>? systemVersion;
+
+  /// The handler for constructing [web_kit.WebView]s.


If it's for creating webviews, isn't it a Factory rather than a Proxy?

On Android, this class is also used to call static methods. So there is a possibility that it could be used for more than constructing WebViews. I updated the documentation.

stuartmorgan-g · 2022-01-14T21:15:04Z

packages/webview_flutter/webview_flutter_wkwebview/pubspec.yaml

        pluginClass: FLTWebViewFlutterPlugin

 dependencies:
+  device_info_plus: ^3.1.1


I definitely don't want to do this. We should build the specific functionality we need into the plugin.

stuartmorgan-g · 2022-01-14T21:19:25Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+    return _parseVersion(first).compareTo(_parseVersion(second)) >= 0;
+  }
+
+  Version _parseVersion(String versionStr) {


pub_semver is overkill here; you're already writing manual parsing code anyway, and the set of things it needs to support is smaller than pub_semver (there's no iOS 10.0.7+3-dev for instance). You can eliminate the dependency and just parse to a three-element array then compare in a loop.

stuartmorgan-g · 2022-01-14T21:20:42Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+        if (requiresUserAction) web_kit.AudiovisualMediaType.all,
+        if (!requiresUserAction) web_kit.AudiovisualMediaType.none
+      };
+    } else if (_isGreaterOrEqualVersion(await systemVersion, '9.0')) {


This is always true, so should just be the else.

…view_small

bparrishMines · 2022-01-21T04:10:01Z

@stuartmorgan This is ready for another review. I decided to remove the logic for checking the version since it can be easily handled with platform code: https://github.com/flutter/plugins/blob/main/packages/webview_flutter/webview_flutter_wkwebview/ios/Classes/FlutterWebView.m#L568

stuartmorgan-g

Sorry this took forever to re-review :( LGTM with some small nits.

stuartmorgan-g · 2022-02-04T18:51:20Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/web_kit/web_kit.dart

+///
+/// Wraps
+/// [WKWebView](https://developer.apple.com/documentation/webkit/wkwebview?language=objc).
+class WebView {


I wonder if we should actually keep the WK prefix on these? It's not standard Dart, but it would make it very clear that these are just mirroring the corresponding ObjC class.

I could go either way on it, but I thought it was worth raising.

I like this idea. I added WK to all the webkit classes.

stuartmorgan-g · 2022-02-04T18:52:39Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+import 'web_kit/web_kit.dart' as web_kit;
+
+/// A [Widget] that displays a [web_kit.WebView].
+class WebViewCupertinoWidget extends StatefulWidget {


We should probably not call this Cupertino since we'll likely add macOS support to this package at some point, and in Flutter terminology Cupertino is specifically iOS. WKWebViewWidget? Although that could be confusing, especially if we put WK on the other classes. How about WebViewWebKitWidget?

I used Cupertino because it is used as the name of the WebViewPlatform implementation, but I think we should also update that with the new plugin interface. What are your thoughts on WebKitWebViewWidget? I was also, coincidently, updating the Android implementation to AndroidWebKitWebViewWidget.

This also made me consider that this plugin is called webview_flutter_wkwebview, but the Apple library is called WebKit and WKWebView is only a class in the library. However, the Android library could technically be called WebKit too, so I could understand why are plugin naming scheme wouldn't be consistent.

But all in all, I was considering

Platform Platform Implementation Prefix Library File Name

iOS/MacOS WebKit... web_kit.dart

Android AndroidWebKit... android_web_kit.dart

I like WKWebView because it's less ambiguous; webkit doesn't really say much if you don't know its intended to be a library name. UIWebView was also WebKit-based, for instance. While WKWebView is only one class, it's the core class, with all the others being supporting classes.

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

stuartmorgan-g · 2022-02-04T18:54:55Z

packages/webview_flutter/webview_flutter_wkwebview/lib/src/webview_cupertino_widget.dart

+}
+
+/// An implementation of [WebViewPlatformController] with the WebKit api.
+class WebViewCupertinoPlatformController extends WebViewPlatformController {


Same name change here.

conversation continued in #4626 (comment)

…view_small

…ation (flutter/plugins#4626)

start of building new ios implementation

1dac436

github-actions bot added p: webview_flutter Edits files for a webview_flutter plugin platform-ios labels Dec 20, 2021

bparrishMines added 2 commits December 20, 2021 16:05

test and improve version comparison

04a1e61

accidental line

0aa5192

bparrishMines commented Dec 20, 2021

View reviewed changes

fix class name in comment

e9021f6

bparrishMines commented Dec 20, 2021

View reviewed changes

bparrishMines added 2 commits December 20, 2021 16:24

changelong and formatting

f0f9b8b

update changelog

9e9c8b2

bparrishMines changed the title ~~start of building new ios implementation~~ [webview_flutter_wkwebview] Start work on new ios implementationr Dec 22, 2021

bparrishMines changed the title ~~[webview_flutter_wkwebview] Start work on new ios implementationr~~ [webview_flutter_wkwebview] Start work on new ios implementation Dec 22, 2021

bparrishMines added 2 commits December 22, 2021 10:55

move workaround to method

4a942fb

link to issue

5efe4fb

bparrishMines requested a review from stuartmorgan-g December 22, 2021 16:29

remove workaround

a3fd8a5

godofredoc changed the base branch from master to main January 6, 2022 22:40

hpoul mentioned this pull request Jan 10, 2022

[webview_flutter] Support basic auth requests #2420

Closed

13 tasks

stuartmorgan-g suggested changes Jan 14, 2022

View reviewed changes

bparrishMines added 4 commits January 19, 2022 10:27

Merge branch 'main' of github.com:flutter/plugins into dart_start_web…

a83d3d8

…view_small

remove version control and link to docs

45e0ebf

update proxy docs

6821a78

Merge branch 'main' of github.com:flutter/plugins into dart_start_web…

7f0be21

…view_small

stuartmorgan-g approved these changes Feb 4, 2022

View reviewed changes

bparrishMines added 3 commits February 8, 2022 10:13

naming updates

bbc1bb3

Merge branch 'main' of github.com:flutter/plugins into dart_start_web…

67f475a

…view_small

update doc

9f11cbc

bparrishMines 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 Feb 9, 2022

bparrishMines merged commit f2944b5 into flutter:main Feb 9, 2022

bparrishMines deleted the dart_start_webview_small branch February 9, 2022 18:25

engine-flutter-autoroll mentioned this pull request Feb 10, 2022

Roll Plugins from 9d32c83af9af to aa36a8a606e1 (10 revisions) flutter/flutter#98198

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 10, 2022

f2944b58d [webview_flutter_wkwebview] Start work on new ios implement…

91d7c69

…ation (flutter/plugins#4626)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Plugins from 9d32c83af9af to f6495091d470 (18 revisions) flutter/flutter#98261

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

f2944b58d [webview_flutter_wkwebview] Start work on new ios implement…

5bbf896

…ation (flutter/plugins#4626)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Plugins from 9d32c83af9af to f45bf32f9f53 (19 revisions) flutter/flutter#98266

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

f2944b58d [webview_flutter_wkwebview] Start work on new ios implement…

2d932fd

…ation (flutter/plugins#4626)

engine-flutter-autoroll mentioned this pull request Feb 14, 2022

Roll Plugins from 9d32c83af9af to 6b0d4549eaa5 (30 revisions) flutter/flutter#98410

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 14, 2022

f2944b58d [webview_flutter_wkwebview] Start work on new ios implement…

dc3a5ab

…ation (flutter/plugins#4626)

Platform	Platform Implementation Prefix	Library File Name
iOS/MacOS	WebKit...	web_kit.dart
Android	AndroidWebKit...	android_web_kit.dart

[webview_flutter_wkwebview] Start work on new ios implementation #4626

[webview_flutter_wkwebview] Start work on new ios implementation #4626

Uh oh!

Conversation

bparrishMines commented Dec 20, 2021 • edited 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!

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!

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!

bparrishMines commented Jan 21, 2022

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bparrishMines commented Dec 20, 2021 •

edited

Loading