[webview_flutter] Implementation of the webview_flutter_platform_interface package #4302

mvanbeusekom · 2021-09-02T15:08:21Z

This PR adds the webview_flutter_platform_interface package which can be used by plugin developers to add platform support to the webview_flutter plugin.

flutter/flutter#86286

Pre-launch Checklist

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

stuartmorgan

A few things from a first pass:

This is a big enough PR that I'd rather it not do two things at once; let's make moving to a subfolder its own separate precursor PR. (That's actually an example of something that would fail the new check I'm adding.)
There should be an AUTHORS file for the new package (filed [flutter_plugin_tools] Validate that AUTHORS exists flutter#89680), cloned from the main package as a starting point.
Because it's a copy rather than a move it's hard to know what (if anything) has changed. Could you separate out a first commit that is just a straight copy of all the relevant files to the new location, then do any changes in separate commits? I see that there's some of that, but, e.g., the new CHANGELOG is in that commit so I know it's not a pure copy. I can't easily see what other actual changes are there to review.

stuartmorgan · 2021-09-08T20:11:11Z

packages/webview_flutter/webview_flutter_platform_interface/CHANGELOG.md

@@ -0,0 +1,3 @@
+## 1.0.0
+
+* Initial open-source release.


This description doesn't really make sense; how about "Extracted platform interface from webview_flutter"

Much better, thank you. I will pay more attention to this in the future.

stuartmorgan · 2021-09-08T20:13:34Z

.../webview_flutter_platform_interface/test/src/method_channel/webview_method_channel_test.dart

+void main() {
+  TestWidgetsFlutterBinding.ensureInitialized();
+
+  group('$MethodChannelWebViewPlatform', () {


I don't know why the plugins repo does so much of this; tests are already grouped by the file they are in. I'd rather just not have a group wrapping all tests. (Logic groups of sets of tests within the file are fine, it's just these top-level ones that seem like noise.)

I have updated the description so the grouping makes more sense. At the moment there are 2 groups:

One that tests the API on the plugin.flutter.io/webview_<channel_id> method channel;

A second group that tests the API on the plugins.flutter.io/cookie_manager method channel.

Creates a new `webview_flutter_platform_interface` directory and adds the following meta-data files: - `AUTHORS`: copied from the `webview_flutter` package and added my name; - `CHANGELOG.md`: new file adding description for release 1.0.0; - `LICENSE`: copied from the `webview_flutter` package; - `README.md`: new file adding the standard platform interface description; - `pubspec.yaml`: new file adding package meta-data for the `webview_flutter_platform_interface` package.

This commit contains a pure copy of the `lib/platform_interface.dart` and `lib/src/webview_method_channel.dart` files from the `webview_flutter` package to the `webview_flutter_platform_interface` package. The `auto_media_playback_policy.dart` and `javascript_mode.dart` contain the `AutoMediaPlaybackPolicy` and `JavascriptMode` enums copied directly from the `webview_flutter/lib/webview_flutter.dart` package. The code doesn't contain any modifications.

Moves all types defined in the `./lib/platform_interface.dart` file into their own respective files. Except for adding the required `import` statements no code has been modified.

The original `webview_flutter/lib/src/webview_method_channel.dart` was not covered by unit-tests. This commit adds unit-tests for all methods in the `webview_method_channel.dart` file that is part of the new platform interface package.

Copies the `JavascriptChannel` and `JavascriptMessage` classes from the `webview_flutter/lib/webview_flutter.dart` file into their own files and expose them through the platform interface. Note: the code has not been modified.

Adds the JavascriptChannelRegistry implementation which is a class that helps managing multiple Javascript channels and sends messages arrived from the native webview control to the correct JavascriptChannel instance in Dart.

This commit modifies the existing code to make use of the JavascriptChannelRegistry class.

mvanbeusekom · 2021-09-09T15:26:56Z

A few things from a first pass:

This is a big enough PR that I'd rather it not do two things at once; let's make moving to a subfolder its own separate precursor PR. (That's actually an example of something that would fail the new check I'm adding.)

There should be an AUTHORS file for the new package (filed [flutter_plugin_tools] Validate that AUTHORS exists flutter#89680), cloned from the main package as a starting point.

Because it's a copy rather than a move it's hard to know what (if anything) has changed. Could you separate out a first commit that is just a straight copy of all the relevant files to the new location, then do any changes in separate commits? I see that there's some of that, but, e.g., the new CHANGELOG is in that commit so I know it's not a pure copy. I can't easily see what other actual changes are there to review.

@stuartmorgan I have redone the pull request and divided it into 9 logical commits and added additional explanation with each commit to explain the changes made.

I have made al changes based on copied files I done in commit bc79ecd which should make it easier for you to follow the changes.

One more thing, to not make to many changes I left the code as-is and did not update it according to the latest analysis_options.yaml (meaning at the moment it still relies on the analysis_options_legacy.yaml configuration). If you would like me to update this also as part of this PR please let me know, otherwise I will file an issue and create a new PR later on.

I would appreciate it if you could have another look.

stuartmorgan · 2021-09-09T15:50:25Z

One more thing, to not make to many changes I left the code as-is and did not update it according to the latest analysis_options.yaml (meaning at the moment it still relies on the analysis_options_legacy.yaml configuration).

Yes, that should definitely be separate. Thanks!

stuartmorgan

Thanks for tweaking the commits; much easier to review. LGTM modulo some nits, and getting the format bots green.

stuartmorgan · 2021-09-09T15:52:02Z

packages/webview_flutter/webview_flutter_platform_interface/README.md

+See https://flutter.dev/go/platform-interface-breaking-changes for a discussion
+on why a less-clean interface is preferable to a breaking change.
+
+[1]: ../webview_flutter


This won't work on pub.dev; let's link to https://pub.dev/packages/webview_flutter

stuartmorgan · 2021-09-09T15:54:45Z

...bview_flutter_platform_interface/lib/src/platform_interface/webview_platform_controller.dart

+    throw UnimplementedError(
+        "WebView getScrollY is not implemented on the current platform");
+  }
+}


A bunch of the files in this PR don't have the final newline (which causes churn when people touch them using many editors). Could you do a pass to fix that?

Did a full sweep, thanks for pointing it out.

stuartmorgan · 2021-09-09T15:56:35Z

.../webview_flutter_platform_interface/test/src/method_channel/webview_method_channel_test.dart

+  group('Tests on `plugin.flutter.io/webview_<channel_id>` channel', () {
+    const int channelId = 1;
+    const MethodChannel channel =
+    MethodChannel('plugins.flutter.io/webview_$channelId');


This needs an auto-formatter pass.

Run the auto-formatter so formatting steps should go green soon.

stuartmorgan · 2021-09-09T15:57:56Z

.../webview_flutter_platform_interface/test/src/method_channel/webview_method_channel_test.dart

@@ -0,0 +1,451 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


Why are the test files in test/src/ rather than just test/? That's not a standard practice in this repo (or Flutter in generally AFAIK).

That is my mistake. I try to keep the directory structure the test is in as close as possible to the directory structure to the file containing the code it is testing.

So in this case when I need to write tests for code that is in the lib/src/types/javascript_channel.dart I create a test file at test/src/types/javascript_channel_test.dart. It helps me navigate to the correct test file if I am looking to extend certain tests for an existing file.

Updated links to markdown links as the footnote notation doesn't render correctly on pub.dev.

stuartmorgan · 2021-09-09T19:53:30Z

Oh, I didn't consider this effect of my change to the Windows bots... the two "Windows Plugins" jobs are never going to run because the builders no longer exist for them (I renamed them). You'll need to sync in the last master.

mvanbeusekom · 2021-09-09T19:55:01Z

Oh, I didn't consider this effect of my change to the Windows bots... the two "Windows Plugins" jobs are never going to run because the builders no longer exist for them (I renamed them). You'll need to sync in the last master.

No worries, will do in a few minutes.

…_architecture_part_2

…orm_interface package (flutter/plugins#4302)

* master: [webview_flutter] Add download listener to Android webview (flutter#4322) [video_player] add support for content-uri based videos (flutter#4330) [webview_flutter] Implementation of the webview_flutter_platform_interface package (flutter#4302) [camera] Bump minimum Flutter version and iOS deployment target (flutter#4327) [video_player] interface: add support for content-uri based videos (android only) (flutter#4307) Ignore unnecessary_import in legacy analysis options (flutter#4129) [ci] Enable the new Windows targets (flutter#4325) # Conflicts: # packages/webview_flutter/webview_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterWebView.java # packages/webview_flutter/webview_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewBuilder.java

…rface package (flutter#4302)

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

google-cla bot added the cla: yes label Sep 2, 2021

mvanbeusekom force-pushed the webview/federated_architecture_part_2 branch from eb1bb34 to c3e2fdd Compare September 2, 2021 15:37

github-actions bot removed the platform-android label Sep 2, 2021

stuartmorgan self-requested a review September 2, 2021 20:10

This was referenced Sep 6, 2021

[webview_flutter] Move native implementations to their own packages in preparation for federated architecture #4315

Closed

[webview_flutter] Complete move to federated architecture #4316

Closed

stuartmorgan suggested changes Sep 8, 2021

View reviewed changes

mvanbeusekom added 9 commits September 9, 2021 09:37

Added webview_flutter_platform_interface.dart barrel file.

5198072

Separate types from ./lib/platform_interface.dart

e286d50

Moves all types defined in the `./lib/platform_interface.dart` file into their own respective files. Except for adding the required `import` statements no code has been modified.

Moved method channel implementation to new folder.

16eb3f0

Added tests for the webview_method_channel.dart.

3be55e5

The original `webview_flutter/lib/src/webview_method_channel.dart` was not covered by unit-tests. This commit adds unit-tests for all methods in the `webview_method_channel.dart` file that is part of the new platform interface package.

Copy JavascriptChannel and JavascriptMessage.

86148f7

Copies the `JavascriptChannel` and `JavascriptMessage` classes from the `webview_flutter/lib/webview_flutter.dart` file into their own files and expose them through the platform interface. Note: the code has not been modified.

Added JavascriptChannelRegistry implementation.

f3d0dd9

Adds the JavascriptChannelRegistry implementation which is a class that helps managing multiple Javascript channels and sends messages arrived from the native webview control to the correct JavascriptChannel instance in Dart.

Modify existing code to use JavascriptChannelRegistry.

a7ece38

This commit modifies the existing code to make use of the JavascriptChannelRegistry class.

mvanbeusekom force-pushed the webview/federated_architecture_part_2 branch from e83c6cc to a7ece38 Compare September 9, 2021 15:17

stuartmorgan approved these changes Sep 9, 2021

View reviewed changes

mvanbeusekom added 3 commits September 9, 2021 18:33

Fix links in README.

46ae8ae

Updated links to markdown links as the footnote notation doesn't render correctly on pub.dev.

Make sure all files end with an empty line.

e63d6a5

Format code according to Flutter standards.

94843c3

mvanbeusekom 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 Sep 9, 2021

Merge remote-tracking branch 'upstream/master' into webview/federated…

9b70390

…_architecture_part_2

fluttergithubbot merged commit 334d8e2 into flutter:master Sep 9, 2021

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Plugins from 5306c02db66b to 334d8e2e8db1 (26 revisions) flutter/flutter#89799

Closed

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

334d8e2 [webview_flutter] Implementation of the webview_flutter_platf…

bfea8c6

…orm_interface package (flutter/plugins#4302)

engine-flutter-autoroll mentioned this pull request Sep 9, 2021

Roll Plugins from 5306c02db66b to d5b65742487f (27 revisions) flutter/flutter#89803

Merged

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

334d8e2 [webview_flutter] Implementation of the webview_flutter_platf…

7198ced

…orm_interface package (flutter/plugins#4302)

mvanbeusekom deleted the webview/federated_architecture_part_2 branch September 10, 2021 06:56

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[webview_flutter] Implementation of the webview_flutter_platform_inte…

432034f

…rface package (flutter#4302)

This was referenced Sep 14, 2021

[webview_flutter] Extract Android implementation into a separate package #4343

Merged

[webview_flutter] Extract WKWebView implementation into a separate package #4345

Merged

[webview_flutter] Migrate plugin to federated architecture flutter/flutter#86286

Closed

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[webview_flutter] Implementation of the webview_flutter_platform_inte…

a05250c

…rface package (flutter#4302)

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

[webview_flutter] Implementation of the webview_flutter_platform_inte…

4c21a56

…rface package (flutter#4302)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webview_flutter] Implementation of the webview_flutter_platform_interface package #4302

[webview_flutter] Implementation of the webview_flutter_platform_interface package #4302

mvanbeusekom commented Sep 2, 2021

stuartmorgan left a comment •

edited

stuartmorgan Sep 8, 2021

mvanbeusekom Sep 9, 2021

stuartmorgan Sep 8, 2021

mvanbeusekom Sep 9, 2021

mvanbeusekom commented Sep 9, 2021

stuartmorgan commented Sep 9, 2021

stuartmorgan left a comment

stuartmorgan Sep 9, 2021

mvanbeusekom Sep 9, 2021

stuartmorgan Sep 9, 2021

mvanbeusekom Sep 9, 2021

stuartmorgan Sep 9, 2021

mvanbeusekom Sep 9, 2021

stuartmorgan Sep 9, 2021

mvanbeusekom Sep 9, 2021

stuartmorgan commented Sep 9, 2021

mvanbeusekom commented Sep 9, 2021

		@@ -0,0 +1,451 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

[webview_flutter] Implementation of the webview_flutter_platform_interface package #4302

[webview_flutter] Implementation of the webview_flutter_platform_interface package #4302

Conversation

mvanbeusekom commented Sep 2, 2021

Pre-launch Checklist

stuartmorgan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvanbeusekom commented Sep 9, 2021

stuartmorgan commented Sep 9, 2021

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 9, 2021

mvanbeusekom commented Sep 9, 2021

stuartmorgan left a comment •

edited