Extracted PlatformViewsChannel from PlatformViewsController. #7847

matthew-carroll · 2019-02-15T22:00:44Z

This PR extracts all platform view system channel communication into PlatformViewsChannel.

The relationship between PlatformViewsController and PlatformViewsChannel is analogous to multiple other relationships including: TextInputPlugin and TextInputChannel, AndroidKeyProcessor and KeyEventChannel, and AccessibilityBridge and AccessibilityChannel.

The idea behind these relationships is that the channels codify the Android/Flutter communication API, while the other associated class handles any state and handler behavior.

At a technical level, this PR handles communication issues with Exceptions so that the inner workings of method channels are completely hidden from down stream consumers.

amirh · 2019-02-20T17:08:18Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformViewsChannel.java

+
+/**
+ * System channel that sends 2-way communication between Flutter and Android to
+ * display Android Views within Flutter.


This channel and the PlatformViewsController isn't displaying the views, it configures them, and passes input events.

Maybe replace "to display Android Views within Flutter" with "to facilitate embedding of Android Views within a Flutter application".

amirh · 2019-02-20T17:25:49Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformViewsChannel.java

+   * An attempt was made to use platform views on a version of Android that platform
+   * views does not support.
+   */
+  public static class InvalidPlatformVersionException extends IllegalStateException {}


I'd favor just throwing IllegalStateException directly with an error message, these specialized exception types are feeling a little too boiler plate-y to me. There's a more detailed explanation on why it's good to reuse IllegallStateException directly in Effective Java item 72. Sending you the internal best practices link that explains it offline.

amirh · 2019-02-20T17:28:40Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java

@@ -47,11 +43,51 @@
    // The texture registry maintaining the textures into which the embedded views will be rendered.
    private TextureRegistry mTextureRegistry;

-    // The messenger used to communicate with the framework over the platform views channel.
-    private BinaryMessenger mMessenger;
+    // The system channel used to communicate with Flutter about platform views.


I'd keep it saying: to communicate with "the framework", this code is also part of Flutter.

amirh · 2019-02-20T17:30:07Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java


    private final HashMap<Integer, VirtualDisplayController> vdControllers;

+    private final PlatformViewsChannel.PlatformViewsHandler channelHandler = new PlatformViewsChannel.PlatformViewsHandler() {


What's the benefit of having this anonymous class over making PlatformViewsController implement the interface?

The distinction is based on the intended use. Implementing an interface is a public statement that the given instance can be used as the interface. In other words, a PlatformViewsController could be used by client code as a PlatformViewsHandler. But if another area of code used this class as a PlatformViewsHandler, that would very likely break this code. What we really want is for this PlatformViewsController to own both a PlatformViewsChannel, as well as a PlatformViewsHandler. These are internal implementation details of a PlatformViewsController, rather than a public behavior of PlatformViewsController. Therefore, the PlatformViewsHandler is kept private and internal.

I see.

What do you think about keeping the actual method implementations in this anonymous class (e.g inlining PlatformViewsController#createPlatformView here).

We're adding quite a lot of indirection layers here, I'm convinced that all of them are justified but this one(see my note to self in the PR comment).

So the recommendation is to avoid re-declaring each method within the outer class and to simply do the behavior within the anonymous class?

If so, then I think that should be fine. I think the only reason I didn't do that was because I was trying to create a minimal diff against the existing code. I wasn't sure at the time if you were in the middle of evolving this class.

So if you confirm that this change is a good resolution then I'll take care of it.

Yeah I support inlining the implementation if there isn't a good reason to keep the methods separate.

amirh · 2019-02-20T17:37:51Z

shell/platform/android/io/flutter/embedding/engine/systemchannels/PlatformViewsChannel.java

+        return;
+      }
+
+      switch (call.method) {


This method is pretty long which makes it hard for me to follow, lets keep split out the handling of the each different switch case to its own separate methods.

cbracken · 2019-04-08T17:40:39Z

@matthew-carroll did you have plans to address comments from @amirh or should we close this PR?

matthew-carroll · 2019-04-08T19:09:25Z

@cbracken I plan to address them

cbracken · 2019-05-06T17:40:13Z

@matthew-carroll any progress on this?

matthew-carroll · 2019-05-09T18:32:59Z

I'm gonna try to refresh this PR this weekend and hopefully have it reviewable by Monday.

matthew-carroll · 2019-05-13T04:46:31Z

@amirh I believe I've addressed your comments. I had to force push an update so that I could get the latest from master, and also because the first 2 commits had the wrong author info for me.

amirh

Almost LGTM
I just want to be convinced that the separation between the implementations in PlatformViewController and in the anonymous implementation of PlatformViewsHandler worth it or merge these 2 layers(see my other comments)

amirh · 2019-05-17T23:06:26Z

shell/platform/android/io/flutter/plugin/platform/PlatformViewsController.java


    private final HashMap<Integer, VirtualDisplayController> vdControllers;

+    private final PlatformViewsChannel.PlatformViewsHandler channelHandler = new PlatformViewsChannel.PlatformViewsHandler() {


I see.

What do you think about keeping the actual method implementations in this anonymous class (e.g inlining PlatformViewsController#createPlatformView here).

We're adding quite a lot of indirection layers here, I'm convinced that all of them are justified but this one(see my note to self in the PR comment).

amirh · 2019-05-17T23:09:37Z

Coming back to this PR 3 months later it's almost like taking a fresh look:

I like the separation of parsing the method channel message from handling it, I think it makes the code easier to follow when the parsing is complex.

I was a little about the amount of extra indirection layers and the boilerplate that comes with them, but have convinced myself it makes sense, so a note to future-self as for why we're adding all this boilerplate:

If I want to add a new foo method to the method channel with this change I need to:

add a switch case in the MethodCallHandler implementation.
implement PlatformViewsChannel#foo.
add foo to the PlatformViewsHandler.
add a foo implementation in the anonymous implementation of channelHandler.
add a private foo implementation in PlatformViewsController.

The separation between 1<-->2 we want to avoid a huge onMethodCallmethod.
#3 we want to avoid a cyclic dependency between PlatformViewsController and PlatformViewsHandler.
The separation between 4<-->5 I'm not yet convinced we want, I'm asking to merge them in this review iteration.

… Java file, and updated licenses accordingly.

matthew-carroll · 2019-06-03T06:56:05Z

@amirh I rebased to fix some conflicts and then I inlined the channel handler implementation as you suggested. Are we good to merge in at this point?

amirh

LGTM

…lutter/engine#7847)

flutter/engine@f9ce016...008090b git log f9ce016..008090b --no-merges --oneline 008090b Extracted PlatformViewsChannel from PlatformViewsController. (flutter/engine#7847) a4abfb2 Text inline widget LibTxt/dart:ui implementation (flutter/engine#8207) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (cbracken@google.com), and stop the roller if necessary.

This regression was introduced in #7847. The PlatformViewsChannel method call handler was always setting the result to `notImplemented` even after handling a result, this resulted in a "Reply already submitted" exception being thrown. Note that the method channel code is swallowing this exception and logging an error, so we didn't crash instead we were logging an error(this is why the integration test didn't fail). Filed flutter/flutter#33863 to make sure tests fail when such exceptions are thrown. This PR also cleans up an unused `NoSuchPlatformViewException` that was introduced in #7847. flutter/flutter#33866

…#7847)

This regression was introduced in flutter#7847. The PlatformViewsChannel method call handler was always setting the result to `notImplemented` even after handling a result, this resulted in a "Reply already submitted" exception being thrown. Note that the method channel code is swallowing this exception and logging an error, so we didn't crash instead we were logging an error(this is why the integration test didn't fail). Filed flutter/flutter#33863 to make sure tests fail when such exceptions are thrown. This PR also cleans up an unused `NoSuchPlatformViewException` that was introduced in flutter#7847. flutter/flutter#33866

flutter/engine@f9ce016...008090b git log f9ce016..008090b --no-merges --oneline 008090b Extracted PlatformViewsChannel from PlatformViewsController. (flutter/engine#7847) a4abfb2 Text inline widget LibTxt/dart:ui implementation (flutter/engine#8207) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (cbracken@google.com), and stop the roller if necessary.

matthew-carroll requested review from amirh, mklim and jason-simmons February 15, 2019 22:00

googlebot added the cla: yes label Feb 15, 2019

matthew-carroll force-pushed the android_embedding_refactor_pr4_add_platformviews_channel branch from 0b1fa09 to 48c5e9b Compare February 19, 2019 22:53

jason-simmons approved these changes Feb 20, 2019

View reviewed changes

amirh suggested changes Feb 20, 2019

View reviewed changes

matthew-carroll force-pushed the android_embedding_refactor_pr4_add_platformviews_channel branch from 48c5e9b to d450c83 Compare May 13, 2019 04:45

matthew-carroll requested a review from amirh May 13, 2019 04:47

amirh reviewed May 17, 2019

View reviewed changes

matthew-carroll added 4 commits June 2, 2019 21:57

Extracted PlatformViewsChannel from PlatformViewsController.

3d1babc

Added license to PlatformViewChannel, updated BUILD.gn to include new…

b1d2b55

… Java file, and updated licenses accordingly.

PR Updates

ad26dfe

PR Updates

6f0a4a4

matthew-carroll force-pushed the android_embedding_refactor_pr4_add_platformviews_channel branch from 672aad4 to 6f0a4a4 Compare June 3, 2019 04:58

matthew-carroll added 2 commits June 2, 2019 22:03

Inlinned implementation in PlatformViewsController's channelHandler.

a11092b

PR Updates

6328a52

matthew-carroll requested a review from amirh June 3, 2019 06:56

amirh approved these changes Jun 3, 2019

View reviewed changes

matthew-carroll merged commit 008090b into flutter:master Jun 3, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 3, 2019

008090b Extracted PlatformViewsChannel from PlatformViewsController. (f…

d117799

…lutter/engine#7847)

engine-flutter-autoroll mentioned this pull request Jun 3, 2019

Roll engine f9ce01674ed6..008090b8abb7 (2 commits) flutter/flutter#33783

Merged

This was referenced Jun 4, 2019

Fix platform views channel regression #9185

Merged

"Reply already submitted" errors whenever using platform views on Android. flutter/flutter#33866

Closed

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019

Extracted PlatformViewsChannel from PlatformViewsController. (flutter…

30c54f1

…#7847)


		private final HashMap<Integer, VirtualDisplayController> vdControllers;

		private final PlatformViewsChannel.PlatformViewsHandler channelHandler = new PlatformViewsChannel.PlatformViewsHandler() {

Extracted PlatformViewsChannel from PlatformViewsController. #7847

Extracted PlatformViewsChannel from PlatformViewsController. #7847

Uh oh!

Conversation

matthew-carroll commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

cbracken commented Apr 8, 2019

Uh oh!

matthew-carroll commented Apr 8, 2019

Uh oh!

cbracken commented May 6, 2019

Uh oh!

matthew-carroll commented May 9, 2019

Uh oh!

matthew-carroll commented May 13, 2019

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amirh commented May 17, 2019

Uh oh!

matthew-carroll commented Jun 3, 2019

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthew-carroll commented Feb 15, 2019 •

edited

Loading