add flag to identify sub-trees with PlatformViewLayers and always paint them #25824
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I should sprinkle a bunch of checks for the flag in the various |
|
I've added a new MockViewEmbedder which does very little work for now, but lets the new test verify the proper flag behaviors when embedding is allegedly happening. |
|
Any chance we could also add a scenario test? https://github.com/flutter/engine/blob/master/testing/scenario_app/README.md#ios-platform-view-tests |
flow/testing/mock_embedder.h
Outdated
| public: | ||
| MockViewEmbedder(SkCanvas* root_canvas); | ||
|
|
||
| ~MockViewEmbedder() = default; |
There was a problem hiding this comment.
Here and elsewhere, please mention the interface which contains the definition being overriden. In this case, // || ExternalViewEmbedder. Also, virtual's need to be OOL. So you could move the = default into the .cc file.
flow/testing/mock_embedder.h
Outdated
| SkCanvas* CompositeEmbeddedView(int view_id) override; | ||
|
|
||
| private: | ||
| SkCanvas* root_canvas_; |
There was a problem hiding this comment.
sk_sp<SkCanvas>? The the ownership of the root canvas is not clear.
There was a problem hiding this comment.
I tried just returning nullptr again and it worked fine for current tests so I'll go with that.
| namespace flutter { | ||
| namespace testing { | ||
|
|
||
| class MockViewEmbedder : public ExternalViewEmbedder { |
There was a problem hiding this comment.
Call this TestViewEmbedder perhaps? This isn't really mocking anything else. It is an actual ExternalViewEmbedder.
There was a problem hiding this comment.
It's in the same space where we define other Mock objects that help with layer_unittests like mock_layer and mock_raster_cache and mock_texture. Over time it can provide more testing insights, but its first need is just to be a standin for having the view_embedder field of the context to be non-null.
…5954) * add flag to identify sub-trees with PlatformViewLayers and always paint them (#25824) * Roll Dart SDK to 2.13.0-211.14.beta * Roll buildroot to pick up github.com/flutter/buildroot/pull/447 * explicitly call generate_winrt_headers.py with python3 * add third_party/expat to DEPS Co-authored-by: Jim Graham <flar@google.com>
This is a more targeted workaround for flutter/flutter#76097
The only down-sides to this workaround is that we will always paint down to a PlatformViewLayer even if it is not visible on the screen. This should have very little impact on performance as the rendering should not result in any work done (in particular, we won't paint leaf nodes like PictureLayer in these subtrees, only the intervening containers and the PVL itself).
This fix is only needed for the iOS embedder, but it will take effect on all platforms as the impact should be negligible. I've filed an issue against the iOS embedder to try to fix the underlying problem there. See flutter/flutter#81419