Move `Shell::Add/RemoveView` to `PlatformView` and refine embedder API doc #52003

dkwingsmt · 2024-04-09T21:02:29Z

This PR moves the methods to add or remove views from Shell to PlatformView. By design, the Shell is supposed to be a messenger that glues classes together, while PlatformView is the operator that embedders that do not use the embedder API should operate on. The current design was made due to lack of knowledge to this design.

This also makes PlatformView aware of views, which might be a prerequisite to #51925.

This PR also adds some details to embedder API AddView and RemoveView.

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.

dkwingsmt · 2024-04-09T22:11:39Z

@chinmaygarde Can you take a look? I'm not sure whether this is a necessary change. I'm proposing this only because I supposed the Shell should have as few methods as possible and the add/removeView methods should have existed on PlatformView to be consistent with other existing methods, such as DispatchPointerDataPacket.

chinmaygarde · 2024-04-10T17:26:17Z

shell/common/platform_view.h

  //----------------------------------------------------------------------------
  /// @brief      Used to forward events from the platform view to interested
  ///             subsystems. This forwarding is done by the shell which sets
  ///             itself up as the delegate of the platform view.
  ///
  class Delegate {
   public:
+    using AddViewCallback = PlatformView::AddViewCallback;


Nit: Is this necessary? You can just use the typedef from the parent.

The compiler requires PlatformView::AddViewCallback (for both the methods in Delegate and Shell) instead of simply AddViewCallback without it, which is definitely not necessary though, and I'm down to remove it. What do you think?

chinmaygarde · 2024-04-10T17:27:11Z

shell/common/platform_view.h

@@ -51,13 +51,17 @@ namespace flutter {
 ///
 class PlatformView {
 public:
+  using AddViewCallback = std::function<void(bool added)>;


What do the added and removed arguments to the callback mean? If added is false, why invoke the AddViewCallback? Perhaps document that?

Both the add and the remove callbacks rely on the result. For example, in the Windows embedder as implemented in #51923

After calling AddView, the embedder blocks until a result is reported, which decides whether the returned FlutterWindowsView is null. (This eager blocking can be optimized in the future, but this is an easier approach for now.)

The embedder also pre-allocate resources after calling AddView before the callback, and if the result turns out a failure, then the resources are deallocated. The pre-allocation ensures that the framework can start operating the view as soon as they're aware of it.

After calling RemoveView, embedder does not deallocate resources until the callback (successful or not). This allows handling any pending operations during this time.

shell/platform/embedder/embedder.h

dkwingsmt · 2024-04-10T23:05:31Z

I kept the statement "The embedder should prepare resources in advance but be ready to clean up on failure." since I think this is a very useful implementation suggestion.

loic-sharma

LGTM, thanks for cleaning this up!

…mbedder API doc (flutter/engine#52003)

…146663) flutter/engine@16b0cfd...2d85d12 2024-04-11 [email protected] Roll Skia from 08940c2e0c44 to 5101cbe5a6bb (1 revision) (flutter/engine#52066) 2024-04-11 [email protected] Move `Shell::Add/RemoveView` to `PlatformView` and refine embedder API doc (flutter/engine#52003) 2024-04-11 [email protected] [Fuchsia] Support per app present latency tracing (flutter/engine#51503) 2024-04-11 [email protected] Save and restore OpenGL bindings that are changed by fl_renderer_render (flutter/engine#51887) 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 [email protected],[email protected],[email protected] 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

…lutter#146663) flutter/engine@16b0cfd...2d85d12 2024-04-11 [email protected] Roll Skia from 08940c2e0c44 to 5101cbe5a6bb (1 revision) (flutter/engine#52066) 2024-04-11 [email protected] Move `Shell::Add/RemoveView` to `PlatformView` and refine embedder API doc (flutter/engine#52003) 2024-04-11 [email protected] [Fuchsia] Support per app present latency tracing (flutter/engine#51503) 2024-04-11 [email protected] Save and restore OpenGL bindings that are changed by fl_renderer_render (flutter/engine#51887) 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 [email protected],[email protected],[email protected] 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

Impl

594f12c

github-actions bot added the platform-ios label Apr 9, 2024

dkwingsmt requested review from loic-sharma and chinmaygarde April 9, 2024 21:23

Fix fuchsia compile

ef707e4

github-actions bot added the platform-fuchsia label Apr 9, 2024

dkwingsmt added 2 commits April 9, 2024 15:35

Tidy

364e855

Fix fuchsia compile

881ec1d

chinmaygarde reviewed Apr 10, 2024

View reviewed changes

embedder API docs

ef8ed56

dkwingsmt requested a review from cbracken as a code owner April 10, 2024 21:00

loic-sharma reviewed Apr 10, 2024

View reviewed changes