Add Platform View Manager to Windows shell #50598

yaakovschectman · 2024-02-13T17:02:16Z

Create a manager for platform views accessible to the compositor, handle the methods that the framework will send.

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 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.

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

cbracken

Thanks for sending this out! A few initial comments, mostly just on the docs side of things; I haven't given the implementation a pass other than one test.

cbracken · 2024-02-13T17:41:25Z

shell/platform/windows/platform_view_manager.h

+
+namespace flutter {
+
+enum class FocusChangeDirection { kProgrammatic, kForward, kBackward };


Add doc comments to document the enum and each value.

shell/platform/windows/platform_view_manager.h

cbracken · 2024-02-13T17:44:21Z

shell/platform/windows/platform_view_manager.h

+  virtual ~PlatformViewManager();
+
+  virtual void QueuePlatformViewCreation(std::string_view type_name,
+                                         int64_t id);


Consider extracting a using PlatformViewId = int64_t; for clarity in both the API and any local variable declarations.

cbracken · 2024-02-13T17:45:52Z

shell/platform/windows/public/flutter_windows.h

@@ -203,6 +203,28 @@ FLUTTER_EXPORT void FlutterDesktopEngineSetNextFrameCallback(
    VoidCallback callback,
    void* user_data);

+typedef struct {
+  size_t cb_size;


Here and below, use struct_size for consistency with existing code.

cbracken · 2024-02-13T17:46:33Z

shell/platform/windows/public/flutter_windows.h

@@ -203,6 +203,28 @@ FLUTTER_EXPORT void FlutterDesktopEngineSetNextFrameCallback(
    VoidCallback callback,
    void* user_data);

+typedef struct {


Document this struct and its fields.

For the user_data field in particular, we always state that it's arbitrary user data that is opaque to the embedder, but it's often worth documenting what we typically expect the user to pass, where there is a reasonable "typical" value.

cbracken · 2024-02-13T17:48:45Z

shell/platform/windows/public/flutter_windows.h

+  HWND parent_window;
+  const char* platform_view_type;
+  void* user_data;
+  int64_t platform_view_id;


I know we don't do it on other platforms (yet), but I think we should consider extracting a typedef for PlatformView IDs for the same reason as mentioned for our internal APIs.

shell/platform/windows/fixtures/main.dart

cbracken

A few more minor notes.

cbracken · 2024-02-13T18:19:00Z

shell/platform/windows/compositor.h

@@ -32,6 +35,9 @@ class Compositor {

  // Present Flutter content and platform views onto the view.
  virtual bool Present(const FlutterLayer** layers, size_t layers_count) = 0;
+
+ protected:
+  PlatformViewManager* platform_view_manager_;


Consider documenting that this is a weak pointer, and that the PlatformViewManager is owned by FlutterEngine.

Should we make this be a std::shared_ptr to avoid raw pointers? We're not creating tons of these objects so the performance impact should be acceptable.

cbracken · 2024-02-13T18:22:40Z

shell/platform/windows/platform_view_manager.h

+                         FocusChangeDirection direction,
+                         bool focus);
+
+  std::optional<HWND> GetNativeHandleForId(int64_t id) const;


What are we gaining by using a std::optional here over just returning nullptr, since HWND is already a pointer type? In both cases, the caller is left with:

auto platform_view = GetNativeHandleForId(pvid); if (platform_view) { ... }

Just an indicator that this may return null when the id is bad. We can return just HWND and state this in a comment instead.

cbracken · 2024-02-13T18:25:29Z

shell/platform/windows/platform_view_manager.h

+ private:
+  std::unique_ptr<MethodChannel<EncodableValue>> channel_;
+
+  std::map<std::string, FlutterPlatformViewTypeEntry> platform_view_types_;


Here and below, unless ordering is required, prefer std::unordered_map. I realise we're pretty inconsistent in existing code about this.

cbracken · 2024-02-13T18:25:53Z

shell/platform/windows/platform_view_manager.h

+
+  std::map<int64_t, std::function<HWND()>> pending_platform_views_;
+
+  TaskRunner* task_runner_;


Add a comment on ownership.

loic-sharma · 2024-02-13T23:21:53Z

shell/platform/windows/compositor.h

@@ -19,6 +21,7 @@ namespace flutter {
 // Platform views are not yet supported.
 class Compositor {
 public:
+  Compositor(PlatformViewManager* manager) : platform_view_manager_(manager) {}


Compositor is just an interface that knows nothing about the implementation. I'd remove the platform_view_manager_ field from this type and move it to the software & opengl compositors.

... but since these compositors don't use this platform view manager yet, I wouldn't update them to accept this platform view manager yet.

shell/platform/windows/compositor_software.h

shell/platform/windows/fixtures/main.dart

loic-sharma · 2024-02-13T23:31:09Z

shell/platform/windows/fixtures/main.dart

+  // for the implementation of the encoding and magic number identifiers.
+  final List<int> data = <int>[
+    // Method name
+    7, 'create'.length, ...utf8.encode('create'),


For readability, could we create constants for these? Something like:

const int valueNull = 0; const int valueInt32 = 3; const int valueString = 7; const int valueMap = 13;

I like it. Even better:

enum class ValueType { kNull = 0, kInt32 = 3, ... };

@cbracken I think this appears applicable only for C++ code. For the dart file, the const ints may be the best we can do.

Agh yes, not sure where my mind was at there. Dart enhanced enums do provide a mechanism for this, but it's entirely up to you.

shell/platform/windows/flutter_windows.cc

shell/platform/windows/platform_view_manager.h

shell/platform/windows/platform_view_manager.cc

shell/platform/windows/platform_view_manager.h

shell/platform/windows/flutter_windows_engine_unittests.cc

shell/platform/windows/platform_view_plugin.cc

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

loic-sharma · 2024-02-21T18:33:08Z

shell/platform/windows/flutter_windows_engine_unittests.cc

@@ -1169,5 +1170,30 @@ TEST_F(FlutterWindowsEngineTest, ChannelListenedTo) {
  }
 }

+TEST_F(FlutterWindowsEngineTest, ReceivePlatformViewMessage) {


From my understanding, this file is more for engine unit tests. Since this launches & runs a full app, should we move this to the integration tests in flutter_windows_unittests.cc?

I don't feel strongly about this, feel free to keep as is.

It seems to me that this test is quite similar in most regards to the handful of unit tests immediately preceding it in this file. Would you disagree?

I think it's probably fine to land as-is, but I do think we should probably do a followup evaluation of which tests belong where. We're definitely not super consistent and to be honest, I'm not sure we've ever drawn a really clear line (not just in the Windows embedder but also on macOS).

shell/platform/windows/public/flutter_windows.h

shell/platform/windows/platform_view_plugin.h

shell/platform/windows/platform_view_manager.h

shell/platform/windows/flutter_windows_engine_unittests.cc

loic-sharma · 2024-02-21T18:42:31Z

This is looking really close to landing! I left a few nitpicks, but the only one I really care about is this one: #50598 (comment). Let me know if you'd want to chat about that!

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

cbracken

This lgtm -- nice work! I also really like that we now have a mechanism for staging future changes to the API we expose to the runner on Windows.

loic-sharma

Excellent work!!! 🎉

loic-sharma · 2024-02-24T00:37:44Z

shell/platform/windows/fixtures/main.dart

+@pragma('vm:entry-point')
+void sendCreatePlatformViewMethod() async {
+  // The platform view method channel uses the standard method codec.
+  // See https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/services/message_codecs.dart#L262


I would use a permalink here. The line number will be wrong as the code continues to evolve

…50598)

…144147) flutter/engine@7a573c2...34a8b9b 2024-02-26 109111084+yaakovschectman@users.noreply.github.com Add Platform View Manager to Windows shell (flutter/engine#50598) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

@yaakovschectman

_This is the same pull request as #50898. GitHub broke on the previous pull request so I re-created it_ This moves the logic to handle `flutter/accessibility` messages to a new type, `AccessibilityPlugin`. Notable changes: 1. Windows app no longer crashes if it receives accessibility events it does not support 2. Windows app no longer crashes if it receives accessibility events while in headless mode @yaakovschectman After playing around with this, I ended up using a different pattern than what what I suggested on #50598 (comment). This message handler is simple enough that splitting into a child/base types felt like unnecessary boilerplate. The key thing is separating messaging and implementation logic, which was achieved through the `SetUp` method. Let me know what you think, and sorry for all my flip-flopping on this topic! ð�� This is preparation for: flutter/flutter#143765 Sample app for manual testing: flutter/flutter#113059 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

yaakovschectman added 7 commits February 8, 2024 14:20

Define structs, class

500e60c

Define PV type registration runner API function

8dd4543

Bare minimum PlatformViewHandler

ae0d049

Compositor owns platform view manager

5f14af0

Set up for mocking

200f13a

Add unit test

ed90810

Formatting

5d410ff

yaakovschectman added affects: desktop platform-windows labels Feb 13, 2024

yaakovschectman requested review from cbracken, loic-sharma, dkwingsmt and gspencergoog February 13, 2024 17:02

yaakovschectman self-assigned this Feb 13, 2024

cbracken reviewed Feb 13, 2024

View reviewed changes

yaakovschectman added 2 commits February 13, 2024 13:45

License fix, await future

484c8c1

PR Feedback

b8fadd7

loic-sharma reviewed Feb 13, 2024

View reviewed changes

shell/platform/windows/compositor_software.h Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 13, 2024

View reviewed changes

shell/platform/windows/fixtures/main.dart Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 13, 2024

View reviewed changes

shell/platform/windows/flutter_windows.cc Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 13, 2024

View reviewed changes

shell/platform/windows/platform_view_manager.h Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 13, 2024

View reviewed changes

shell/platform/windows/platform_view_manager.cc Show resolved Hide resolved

yaakovschectman added 5 commits February 14, 2024 13:16

_internal header file

1d203f4

Wait to add manager to compositors

b9acc1a

Rename fixture function

35eb688

Reorder parameters

659ebd6

Use constants

db2851b

loic-sharma reviewed Feb 16, 2024

View reviewed changes

shell/platform/windows/platform_view_manager.cc Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 16, 2024

View reviewed changes

shell/platform/windows/platform_view_manager.h Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 16, 2024

View reviewed changes

shell/platform/windows/flutter_windows_engine_unittests.cc Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 16, 2024

View reviewed changes

shell/platform/windows/platform_view_plugin.cc Show resolved Hide resolved

yaakovschectman and others added 2 commits February 16, 2024 16:53

PR Feedback

6e6ef7e

Update shell/platform/windows/flutter_windows_engine_unittests.cc

22a9e46

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

loic-sharma reviewed Feb 21, 2024

View reviewed changes

shell/platform/windows/public/flutter_windows.h Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 21, 2024

View reviewed changes

shell/platform/windows/platform_view_plugin.h Show resolved Hide resolved

loic-sharma reviewed Feb 21, 2024

View reviewed changes

shell/platform/windows/platform_view_manager.h Outdated Show resolved Hide resolved

loic-sharma reviewed Feb 21, 2024

View reviewed changes

shell/platform/windows/flutter_windows_engine_unittests.cc Outdated Show resolved Hide resolved

yaakovschectman and others added 7 commits February 21, 2024 14:51

Update shell/platform/windows/platform_view_manager.h

95a8498

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Update shell/platform/windows/public/flutter_windows.h

1fa97f3

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

Update shell/platform/windows/flutter_windows_engine_unittests.cc

5453b29

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

PR Feedback

dfa6b73

Merge branch 'main' into win_pv_i

cde468e

Include internal header

585ca11

Merge branch 'main' into win_pv_i

54d91a3

cbracken approved these changes Feb 23, 2024

View reviewed changes

loic-sharma approved these changes Feb 23, 2024

View reviewed changes

loic-sharma reviewed Feb 24, 2024

View reviewed changes

loic-sharma mentioned this pull request Feb 24, 2024

[Windows] Introduce an accessibility plugin #50898

Closed

8 tasks

yaakovschectman merged commit 34a8b9b into flutter:main Feb 26, 2024

yaakovschectman deleted the win_pv_i branch February 26, 2024 15:13

engine-flutter-autoroll mentioned this pull request Feb 26, 2024

Roll Flutter Engine from 7a573c21a4ad to 34a8b9bdaaac (1 revision) flutter/flutter#144147

Merged

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

34a8b9bda Add Platform View Manager to Windows shell (flutter/engine#…

f1d325e

…50598)

loic-sharma mentioned this pull request Feb 26, 2024

[Windows] Introduce an accessibility plugin #50975

Merged

8 tasks


		namespace flutter {

		enum class FocusChangeDirection { kProgrammatic, kForward, kBackward };


		std::map<int64_t, std::function<HWND()>> pending_platform_views_;

		TaskRunner* task_runner_;

Add Platform View Manager to Windows shell #50598

Add Platform View Manager to Windows shell #50598

Uh oh!

Conversation

yaakovschectman commented Feb 13, 2024

Pre-launch Checklist

Uh oh!

cbracken left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

loic-sharma Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loic-sharma commented Feb 21, 2024

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

loic-sharma left a comment

Choose a reason for hiding this comment

cbracken left a comment •

edited

Loading

loic-sharma Feb 13, 2024 •

edited

Loading

loic-sharma Feb 13, 2024 •

edited

Loading

cbracken Feb 14, 2024 •

edited

Loading