Add platform view support for Linux shell #24169

huanghongxun · 2021-02-03T14:42:20Z

This pull request is part of #23985. For framework API, see flutter/flutter#74814.

This pull request adds platform view API to Linux shell.

Code changes:

FlGestureHelper for accepting gesture, redistributing press, release and motion events to platform view. Because only Flutter framework knows about whether we can touch and operate with the platform view by hit test. If Flutter framework accepts a gesture, FlGestureHelper will send following pointer events to GtkWidget. For hover events, If Flutter framework has notified enter and exit event, FlGestureHelper will also send pointer hover event (or GdkEventMotion) to platform view.
FlPlatformView to wrap a GtkWidget, owned by FlPlatformViewsPlugin. This abstraction allows us to owning a FlMethodChannel in FlPlatformView for communication with dart codes.
FlPlatformViewFactory for creating platform views for specific view type. Flutter Framework will call functions of FlPlatformViewFactory when creating a new platform view.
FlPlatformViewsPlugin for implementing SystemChannels.platform_views from Flutter services library.

A working demo with webview: https://github.com/wechat-miniprogram/gtktest.

This pr does not support applying mutations on the platform views yet. I will open a PR later to implement that feature.

This pr is part of platform view implementation on Linux, related issue: flutter/flutter#41724.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

flutter-dashboard · 2021-02-03T14:42:48Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

huanghongxun · 2021-02-03T15:45:39Z

@robert-ancell @wmww
Here's platform view implementation. Clipping mutations are not supported by this PR.

wmww · 2021-02-03T21:45:19Z

shell/platform/linux/fl_gesture_helper.cc

+  button.button.button = event->button;
+  button.button.device = event->device;
+  button.button.x_root = event->x_root;
+  button.button.y_root = event->y_root;


Is there a reason to copy each field individually instead of copying the whole event and making required adjustments? Seems to me like the latter would have less opportunities for subtle errors.

shell/platform/linux/fl_platform_views_plugin.cc

wmww · 2021-02-03T21:57:05Z

shell/platform/linux/fl_platform_views_plugin.cc

+                                         int64_t id) {
+  gpointer p = g_hash_table_lookup(self->platform_views, GINT_TO_POINTER(id));
+  if (p && FL_IS_PLATFORM_VIEW(p)) {
+    return FL_PLATFORM_VIEW(p);


Is the FL_IS_PLATFORM_VIEW() check necessary? Will it ever be anything other than null or a platform view? Even if it is, the FL_PLATFORM_VIEW() cast should return null in that case (it would just print an error as well).

wmww · 2021-02-03T22:02:27Z

shell/platform/linux/fl_platform_views_plugin.cc

+    return FL_METHOD_RESPONSE(fl_method_error_response_new(
+        kBadArgumentsError, "View type does not exist", nullptr));
+  }
+  FlPlatformViewFactory* factory = FL_PLATFORM_VIEW_FACTORY(factory_pointer);


This probably won't fail, but it can't hurt to move the FL_PLATFORM_VIEW_FACTORY() cast to above the check and then check if factory exists, rather than checking factory_pointer (assuming that factory shouldn't ever be null)

wmww · 2021-02-03T22:04:17Z

shell/platform/linux/fl_platform_views_plugin.cc

+static FlMethodResponse* platform_views_accept_gesture(
+    FlPlatformViewsPlugin* self,
+    FlValue* args) {
+  if (fl_value_get_type(args) != FL_VALUE_TYPE_LIST) {


Perhaps we should also check if the list has the expected number of elements?

shell/platform/linux/fl_platform_views_plugin.cc

wmww · 2021-02-03T22:09:33Z

shell/platform/linux/fl_platform_views_plugin.cc

+    FlPlatformViewsPlugin* self,
+    int view_identifier) {
+  return (FlPlatformView*)g_hash_table_lookup(self->platform_views,
+                                              GINT_TO_POINTER(view_identifier));


Can this be a safe FL_PLATFORM_VIEW() cast?

It's safe, because we have checked its type in platform_views_create before inserting it into platform_views.

wmww · 2021-02-03T22:11:01Z

shell/platform/linux/fl_platform_views_plugin.h

+/**
+ * fl_platform_views_plugin_new:
+ * @messenger: an #FlBinaryMessenger.
+ * @gesture_helper: an #FlGestureHelper for accepting gestures by Dart side.


As I said in the other comment, ownership of gesture_helper is unclear to me.

shell/platform/linux/fl_view.cc

wmww

LGTM

chinmaygarde · 2021-02-11T22:49:02Z

It's unclear who the owner of this PR is. Does this need additional reviews or can we land this?

cc @cbracken.

huanghongxun · 2021-02-12T02:41:52Z

It's unclear who the owner of this PR is. Does this need additional reviews or can we land this?

cc @cbracken.

The native implementation and interfaces are approved by wmww. Because I have added logics to deal with messages from framework side channel SystemChannel.platform_views, we may review framework side patch flutter/flutter#74814 first, and land both of PRs together?

chinmaygarde · 2021-02-18T22:48:13Z

Thanks for the clarification. The framework patch does seem to be being actively reviewed. Once that is good to go, we can land this as well.

chinmaygarde · 2021-03-04T22:46:20Z

A reviewer was added to the framework patch but it hasn't landed yet. @goderbauer Is there anything we can do to expedite that review? This patch has been pending for close to a month.

…latform_views

chinmaygarde · 2021-03-11T22:53:56Z

@gspencergoog This is the engine patch for flutter/flutter#74814. Are we able to find a reviewer for this?

gspencergoog · 2021-03-11T23:05:52Z

@chinmaygarde: working on it. @cbracken is going to take a look, and we think @cyanglaz might be a good candidate to review, if he's up for it.

shell/platform/linux/fl_platform_views_plugin.cc

shell/platform/linux/fl_clipping_view.cc

shell/platform/linux/fl_renderer_gl.cc

huanghongxun · 2021-05-06T15:25:14Z

There are a couple of things that need to be addressed, but I think this PR looks mostly good. Apart from the listed comments, the major ones are:

Can we remove gesture_helper from this PR, and land it as another change set once this PR lands? I noticed that some of the methods there aren't used and wired up right now, also that class needs tests.

Can we please fix the current test failures?

Thanks.

FlGestureHelper is used to bypass touch events from Framework to platform views, which is important.

…latform_views

chinmaygarde · 2021-05-13T21:52:17Z

@iskakaushik and @robert-ancell I believe this is ready for another review.

shell/platform/linux/fl_clipping_view.cc

shell/platform/linux/fl_clipping_view.h

shell/platform/linux/fl_gesture_helper.cc

shell/platform/linux/fl_platform_views_plugin.cc

shell/platform/linux/fl_view_private.h

shell/platform/linux/fl_clipping_view.h

shell/platform/linux/fl_clipping_view.cc

robert-ancell · 2021-05-20T02:06:44Z

@huanghongxun please remove the gesture helper and do in a following PR as requested by @iskakaushik. If there's anything else you can split out, that will help a lot. These PRs are really big and that makes them slow to review. I would much prefer to review 3 smaller PRs in a row than one big one and that will be faster.

…latform_views

huanghongxun · 2021-05-20T14:28:42Z

@robert-ancell I have proposed #26288 with all requested changes resolved.

zanderso · 2021-10-07T20:04:00Z

@huanghongxun Since this PR has not been updated in quite some time, I am going to close it as presumed stale.

sliechti · 2021-10-09T02:54:36Z

I don't know about others but I have been waiting so long for this PR. What's remaining? Address code reviews? Split the pull request? If @huanghongxun doesn't have time, maybe I can help.

In my opinion, getting webview to work on flutter desktop should be a priority for the Flutter team. I'm puzzled why is not?

huanghongxun · 2021-10-09T08:24:20Z

@sliechti We are tracking the platform view implementation in #26288.

sliechti · 2021-10-09T11:57:52Z

Thank you @huanghongxun

googlebot added the cla: yes label Feb 3, 2021

huanghongxun mentioned this pull request Feb 3, 2021

Platform view implementation on Linux #23985

Closed

9 tasks

huanghongxun force-pushed the dev/platform_views branch 2 times, most recently from f30bd85 to d4cdf91 Compare February 3, 2021 15:02

Add platform view support for Linux shell

2bc8aaf

huanghongxun force-pushed the dev/platform_views branch from d4cdf91 to 2bc8aaf Compare February 3, 2021 15:45

wmww reviewed Feb 3, 2021

View reviewed changes

huanghongxun added 3 commits February 4, 2021 15:12

Fix docs

df150a6

Add scroll event propagation

14dc152

Add platform views plugin test

d3c96ee

wmww approved these changes Feb 5, 2021

View reviewed changes

huanghongxun added 2 commits February 7, 2021 19:46

Apply clipping mutators to platform views

92fc86b

Merge branch 'master' into dev/platform_views

4caf889

Update licenses_flutter

f14f743

huanghongxun force-pushed the dev/platform_views branch 3 times, most recently from e92cef5 to 098a052 Compare February 22, 2021 12:58

Support setDirection

d81f161

huanghongxun force-pushed the dev/platform_views branch from 098a052 to d81f161 Compare February 22, 2021 14:06

huanghongxun added 2 commits March 8, 2021 20:42

Merge branch 'master' of https://github.com/flutter/engine into dev/p…

25f61be

…latform_views

fix: transformation

fca32cf

huanghongxun mentioned this pull request Mar 10, 2021

Add platform view supports on Linux flutter/flutter#74814

Closed

8 tasks

robert-ancell reviewed May 6, 2021

View reviewed changes

shell/platform/linux/fl_platform_views_plugin.cc Show resolved Hide resolved

robert-ancell reviewed May 6, 2021

View reviewed changes

shell/platform/linux/fl_clipping_view.cc Outdated Show resolved Hide resolved

robert-ancell reviewed May 6, 2021

View reviewed changes

shell/platform/linux/fl_clipping_view.cc Outdated Show resolved Hide resolved

robert-ancell reviewed May 6, 2021

View reviewed changes

shell/platform/linux/fl_clipping_view.cc Outdated Show resolved Hide resolved

robert-ancell reviewed May 6, 2021

View reviewed changes

shell/platform/linux/fl_renderer_gl.cc Outdated Show resolved Hide resolved

huanghongxun added 2 commits May 6, 2021 23:12

fix: nits

a9a8a22

fix: nits

bed4f49

Merge branch 'master' of https://github.com/flutter/engine into dev/p…

615b93a

…latform_views