[flutter_webview] Migrate to the new embedding #2169

mklim · 2019-10-09T01:01:25Z

Description

This depends on a new bugfix on the engine for text input to work with
the new embedding (flutter/engine#13015).

This also introduces some interesting and strange new failure cases.
With the new embedding, a FlutterView can be attached and detached from
the plugin at any point. It "should never" but technically can happen
that the plugin gets into a state where there is an active proxy adapter
view but no valid FlutterView to use for input proxying. For now I've
mostly added error logs and guards against NPEs detecting whenever we've
gotten into one of this situations. But I think we should probably
rigorously test this in an a2a specific situation to truly vet for a2a
support.

Related Issues

Fixes flutter/flutter#41851.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

mklim · 2019-10-09T01:03:23Z

I haven't decided how to handle the fact that we need a new version of the engine for this work with the new embedding yet. Naively we can try to check the version and then log the error but that's not great. Without the engine patch text input doesn't work on older Android versions at all with the new embedding.

Moncader · 2019-10-09T01:29:03Z

packages/webview_flutter/example/android/gradle.properties

Missing a newline here?

Good catch, thank you.

mklim · 2019-10-14T22:44:36Z

I believe this PR is ready to merge, after review. The latest commit added the new e2e tests and updated the pubspec.yaml.

ETA: On second thought, @amirh we should probably sync up about how to define "minimum version" here. The new embedding is only going to be functional in flutter versions after flutter/engine#13015 lands. Can we get the flutter version at runtime from the Java files? We may want to log an error if the new embedding is used with an older version of flutter.

amirh · 2019-10-14T22:50:01Z

packages/webview_flutter/example/pubspec.yaml

@@ -5,6 +5,7 @@ version: 1.0.1

 environment:
  sdk: ">=2.0.0-dev.68.0 <3.0.0"
+  flutter: ">=1.9.1+hotfix.2 <2.0.0"


I'm curious why you picked hotfix 2? is the intention to bump it to the next stable once it's there?

My local branch is out of date, I just meant to pick the most recent stable. Changed it to .4.

I think so, especially since we'll need the v2 Registrant.

amirh · 2019-10-14T22:50:13Z

packages/webview_flutter/example/android/settings_aar.gradle

@@ -0,0 +1 @@
+include ':app'


What's that?

This was automatically created by the tool at some point. I'll revert it since it's not related to this PR.

amirh · 2019-10-14T22:50:28Z

...example/android/app/src/main/java/io/flutter/plugins/webviewflutterexample/MainActivity.java

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
 package io.flutter.plugins.webviewflutterexample;


nit: add license header

amirh · 2019-10-14T22:54:12Z

...ew_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+public class WebViewFlutterPlugin implements FlutterPlugin {
+  private @Nullable FlutterCookieManager flutterCookieManager;
+
+  /**


I'd think we don't want to direct users to do that. They should rely on the generated registrant (if it's not there it probably means their engine is old enough to not run this plugin properly anyway)

Reworded. I linked to the tracking issue here and removed the reference to MainActivity.

amirh · 2019-10-14T22:57:05Z

...example/android/app/src/main/java/io/flutter/plugins/webviewflutterexample/MainActivity.java

-    super.onCreate(savedInstanceState);
-    GeneratedPluginRegistrant.registerWith(this);
+  public void configureFlutterEngine(FlutterEngine flutterEngine) {
+    flutterEngine.getPlugins().add(new WebViewFlutterPlugin());


Let's add a TODO to remove this

amirh · 2019-10-14T22:58:27Z

...ew_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/WebViewFlutterPlugin.java

+/**
+ * Java platform implementation of the webview_flutter plugin.
+ *
+ * <p>Instantiate this in an add to app scenario to gracefully handle activity and context changes.


Not sure whether we want to include these kind of instructions as users shouldn't really need to do that...

amirh · 2019-10-14T22:59:36Z

...bview_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/InputAwareWebView.java

+
+    Log.e(TAG, "The containerView has changed while the proxyAdapterView exists.");
+    if (containerView != null) {
+      setInputConnectionTarget(proxyAdapterView);


Does it work? (if so should we log an error?)

Yeah, this does work from my testing. Semantically this seems "wrong" to me, because we're changing the view after it's already been launched. I'm worried that there may be other bugs under the hood. But it is spammy to log this without a confirmed error. I downgraded to WARNING.

amirh · 2019-10-14T23:00:12Z

...ew_flutter/android/src/main/java/io/flutter/plugins/webviewflutter/FlutterCookieManager.java

-    // Use FlutterCookieManager#registerWith instead.
+  FlutterCookieManager(BinaryMessenger messenger) {
+    methodChannel = new MethodChannel(messenger, "plugins.flutter.io/cookie_manager");
+    methodChannel.setMethodCallHandler(this);
  }

  static void registerWith(BinaryMessenger messenger) {


I guess we could completely remove this method...

I guess so, done.

amirh

LGTM as it was all nites (modulo nits :) )

This depends on a new bugfix on the engine for text input to work with the new embedding (flutter/engine#13015). This also introduces some interesting and strange new failure cases. With the new embedding, a FlutterView can be attached and detached from the plugin at any point. It "should never" but technically can happen that the plugin gets into a state where there is an active proxy adapter view but no valid FlutterView to use for input proxying. For now I've mostly added error logs and guards against NPEs detecting whenever we've gotten into one of this situations. But I think we should probably rigorously test this in an a2a specific situation to truly vet for a2a support. DO NOT MERGE until we can land tests for this.

- Add the required lifecycle files into the dynamic Gradle inclusion script - Convert the FlutterDriver test to use the e2e package - Update the pubspec and CHANGELOG, including incrementing the minimum SDK version

This depends on a new bugfix on the engine for text input to work with the new embedding (flutter/engine#13015).

mklim requested a review from amirh October 9, 2019 01:01

googlebot added the cla: yes label Oct 9, 2019

Moncader reviewed Oct 9, 2019

View reviewed changes

mklim changed the title ~~Migrate flutter_webview to the new embedding~~ [flutter_webview] Migrate to the new embedding Oct 14, 2019

amirh reviewed Oct 14, 2019

View reviewed changes

amirh approved these changes Oct 14, 2019

View reviewed changes

Michael Klimushyn added 4 commits October 14, 2019 16:49

Last migration fixes

0f24452

- Add the required lifecycle files into the dynamic Gradle inclusion script - Convert the FlutterDriver test to use the e2e package - Update the pubspec and CHANGELOG, including incrementing the minimum SDK version

Review feedback

f73bfd8

Increment pubspec/changelog

6ced266

mklim merged commit 573c9ff into flutter:master Oct 15, 2019

mklim deleted the webview_migrate_embedding branch October 15, 2019 00:13

mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019

[flutter_webview] Migrate to the new embedding (flutter#2169)

01cfa4f

This depends on a new bugfix on the engine for text input to work with the new embedding (flutter/engine#13015).

sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019

[flutter_webview] Migrate to the new embedding (flutter#2169)

6f78be0

This depends on a new bugfix on the engine for text input to work with the new embedding (flutter/engine#13015).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flutter_webview] Migrate to the new embedding #2169

[flutter_webview] Migrate to the new embedding #2169

mklim commented Oct 9, 2019 •

edited

mklim commented Oct 9, 2019

Moncader Oct 9, 2019

mklim Oct 9, 2019

mklim commented Oct 14, 2019 •

edited

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh Oct 14, 2019

mklim Oct 14, 2019

amirh left a comment

[flutter_webview] Migrate to the new embedding #2169

[flutter_webview] Migrate to the new embedding #2169

Conversation

mklim commented Oct 9, 2019 • edited

Description

Related Issues

Checklist

Breaking Change

mklim commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklim commented Oct 14, 2019 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirh left a comment

Choose a reason for hiding this comment

mklim commented Oct 9, 2019 •

edited

mklim commented Oct 14, 2019 •

edited