Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Apr 9, 2024

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
Copy link
Contributor Author

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

///
class Delegate {
public:
using AddViewCallback = PlatformView::AddViewCallback;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dkwingsmt dkwingsmt Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

///
class PlatformView {
public:
using AddViewCallback = std::function<void(bool added)>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dkwingsmt dkwingsmt Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dkwingsmt dkwingsmt requested a review from cbracken as a code owner April 10, 2024 21:00
@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Apr 10, 2024

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.

@dkwingsmt dkwingsmt changed the title Move Shell::Add/RemoveView to PlatformView Move Shell::Add/RemoveView to PlatformView and refine embedder API doc Apr 10, 2024
Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for cleaning this up!

@dkwingsmt dkwingsmt merged commit 079a140 into flutter:main Apr 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 11, 2024
…146663)

flutter/engine@16b0cfd...2d85d12

2024-04-11 skia-flutter-autoroll@skia.org Roll Skia from 08940c2e0c44 to 5101cbe5a6bb (1 revision) (flutter/engine#52066)
2024-04-11 dkwingsmt@users.noreply.github.com Move `Shell::Add/RemoveView` to `PlatformView` and refine embedder API doc (flutter/engine#52003)
2024-04-11 chaopeng@chaopeng.me [Fuchsia] Support per app present latency tracing (flutter/engine#51503)
2024-04-11 jason-simmons@users.noreply.github.com 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 chinmaygarde@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
@dkwingsmt dkwingsmt deleted the move-add-remove-view-to-platformview branch April 11, 2024 23:21
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…lutter#146663)

flutter/engine@16b0cfd...2d85d12

2024-04-11 skia-flutter-autoroll@skia.org Roll Skia from 08940c2e0c44 to 5101cbe5a6bb (1 revision) (flutter/engine#52066)
2024-04-11 dkwingsmt@users.noreply.github.com Move `Shell::Add/RemoveView` to `PlatformView` and refine embedder API doc (flutter/engine#52003)
2024-04-11 chaopeng@chaopeng.me [Fuchsia] Support per app present latency tracing (flutter/engine#51503)
2024-04-11 jason-simmons@users.noreply.github.com 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 chinmaygarde@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants