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

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Sep 19, 2023

As of right now, there is an indirect link between flutter/platform_views messages and flutterViewEmbedder, expressed by the callback that's being passed to the PlatformViewMessageHandler.

This PR proposed making this relationship clear between flutter/platform_views and implicitView (i.e. the singleton window).

This PR also opens the path for each view to have its own PlatformViewMessageHandler instance.

Depends on #46044

Part of flutter/flutter#134443

@mdebbar mdebbar requested a review from ditman September 19, 2023 15:02
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 19, 2023
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

The data from this call must contain the viewId that is the container of the platform view. I guess some of the "handlePlatformViewCall" will need to be moved to the platform_dispatcher, to know what view is being used (at least).

((We can probably have a generic handler that extracts the viewId from any message, and reuse that, but that's for the future!))

isNotNull,
reason: 'The container has a single child, the created view.',
);
final DomElement platformView = platformViewsContainer.lastElementChild!;
Copy link
Member

Choose a reason for hiding this comment

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

No need to add lastElementChild for this test, right? can this be:

Suggested change
final DomElement platformView = platformViewsContainer.lastElementChild!;
final DomElement platformView = platformViewsContainer.children.single!;

?

Comment on lines -101 to +102
_contentHandler?.call(content);
_platformViewsContainer.append(content);
Copy link
Member

Choose a reason for hiding this comment

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

Much simpler, this is fantastic!

@mdebbar
Copy link
Contributor Author

mdebbar commented Sep 20, 2023

The data from this call must contain the viewId that is the container of the platform view.

Good point. I'm preparing a framework PR that sends flutterViewId when creating a platform view. Then we can start using it in the engine.

I guess some of the "handlePlatformViewCall" will need to be moved to the platform_dispatcher, to know what view is being used (at least).

The way I was thinking of doing it was: PlatformDispatcher extracts flutterViewId from the platform message, and based on that, it picks the right view from the viewData map. The view instance should have its own platformViewMessageHandler (added by this PR) that can handle the platform message.

@ditman
Copy link
Member

ditman commented Sep 21, 2023

PlatformDispatcher extracts flutterViewId from the platform message, and based on that, it picks the right view from the viewData map. The view instance should have its own platformViewMessageHandler (added by this PR) that can handle the platform message.

Agree. Also the bit that extracts the flutterViewId, if we settle in a standard (maybe @goderbauer has an idea for this) can be shared across the whole engine (instead of having an ad-hoc decoder every time we need to extract a viewId from a platform message)

Still LGTM!

@wanjm
Copy link

wanjm commented Oct 6, 2023

when will it be merged?

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 12, 2023
@auto-submit auto-submit bot merged commit 5e50c7c into flutter:main Oct 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 12, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 12, 2023
flutter/engine@cb37ebc...dee90f1

2023-10-12 skia-flutter-autoroll@skia.org Roll Skia from 8b110fd65de9 to bf557aeaaef8 (1 revision) (flutter/engine#46853)
2023-10-12 uysalere@gmail.com [fuchsia] Add fatal error for Vulkan failure (flutter/engine#46831)
2023-10-12 mdebbar@google.com [web] Stop using `flutterViewEmbedder` for platform views (flutter/engine#46046)

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 jsimmons@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
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
As of right now, there is an indirect link between `flutter/platform_views` messages and `flutterViewEmbedder`, expressed by the callback that's being passed to the `PlatformViewMessageHandler`.

This PR proposed making this relationship clear between `flutter/platform_views` and `implicitView` (i.e. the singleton `window`).

This PR also opens the path for each view to have its own `PlatformViewMessageHandler` instance.

Depends on #46044

Part of flutter/flutter#134443
@mdebbar mdebbar deleted the pv_message_handler_multi_view branch December 11, 2023 20:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants