[Embedder API] Add multi-view present callback #51267
Conversation
loic-sharma
force-pushed
the
multi_view_present
branch
3 times, most recently
from
df8109c to
dfbe75d
Compare
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
loic-sharma
force-pushed
the
multi_view_present
branch
from
dfbe75d to
24d6c3c
Compare
loic-sharma
force-pushed
the
multi_view_present
branch
from
24d6c3c to
0bff957
Compare
| @@ -1022,7 +1090,8 @@ TEST_F(EmbedderTest, NoLayerCreatedForNoOverlayOnTopOfPlatformLayer) { | |||
| auto scene_image = context.GetNextSceneImage(); | |||
|
|
|||
| context.GetCompositor().SetNextPresentCallback( | |||
| [&](const FlutterLayer** layers, size_t layers_count) { | |||
| [&](FlutterViewId view_id, const FlutterLayer** layers, | |||
| size_t layers_count) { | |||
| ASSERT_EQ(layers_count, 2u); | |||
There was a problem hiding this comment.
Should we add ASSERT_EQ(view_id, kImplicitViewId); to all tests like this?
There was a problem hiding this comment.
Although this value is only passed in by EmbedderConfigBuilder, so it shouldn't matter... but that makes the test even more weird. Why is this callback set in such a way, instead of using public API?
There was a problem hiding this comment.
Should we add
ASSERT_EQ(view_id, kImplicitViewId);to all tests like this?
For now, only EmbedderTest.CanRenderImplicitView and EmbedderTest.CanRenderImplicitViewUsingPresentLayersCallback care about the view_id.
The other EmbedderTests only care that the correct content was rendered, they don't care which view it was rendered to. I don't think verifying the view ID is necessary for these tests.
Once we allow multiple views, we will definitely want new tests that verify the view IDs are expected. I expect these to be new tests that are separate from the existing tests.
Why is this callback set in such a way, instead of using public API?
The tests are doing a bunch of setup magic so that they can render stuff using the different backends.
|
@cbracken or @chinmaygarde Would you be able to review the changes in |
|
Taking a look! |
shell/platform/embedder/embedder.h
Outdated
| @@ -1751,13 +1769,22 @@ typedef bool (*FlutterLayersPresentCallback)(const FlutterLayer** layers, | |||
| size_t layers_count, | |||
| void* user_data); | |||
|
|
|||
| /// The callback invoked when the embedder should present | |||
| /// to a view. | |||
There was a problem hiding this comment.
nit: does this actually hit 80 columns?
There was a problem hiding this comment.
Nice catch! I had wrapped this manually for the design doc but forgot to update it here - fixed!
loic-sharma
added
the
autosubmit
…145123) flutter/engine@6710d10...599dbd7 2024-03-14 737941+loic-sharma@users.noreply.github.com [Embedder API] Add multi-view present callback (flutter/engine#51267) 2024-03-14 skia-flutter-autoroll@skia.org Roll Skia from a315e4572f4e to cac09f152d9b (10 revisions) (flutter/engine#51398) 2024-03-14 dkwingsmt@users.noreply.github.com Multi-view View Metrics (flutter/engine#46174) 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 bdero@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
Adds
FlutterCompositor.present_view_callbackto the embedder API. This new present callback adds aview_idmember to allow embedders know which view is being presented.The embedder API does not allow embedders to create multiple views yet. This will be added in a future pull request.
Design doc: https://flutter.dev/go/multi-view-embedder-apis
Pull request that migrates the Windows embedder to this new embedder API: #51293
Part of flutter/flutter#144806
Part of flutter/flutter#142845
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.