[canvaskit] Rework embedded view manager #34081
Conversation
flutter-dashboard
bot
added
the
platform-web
|
Gold has detected about 1 new digest(s) on patchset 6. |
|
Gold has detected about 1 new digest(s) on patchset 7. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
| /// The set of platform views using the backup surface. | ||
| final Set<int> _viewsUsingBackupSurface = <int>{}; | ||
| /// Whether or not we have seen a visible platform view in this frame yet. | ||
| bool _seenFirstVisibleView = false; |
There was a problem hiding this comment.
It's a little strange to see frame-specific state stored on the HtmlViewEmbedder singleton. Maybe a "context object" (a la PrerollContext or BuildContext) that's transient and short-lived would work better?
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
Many comments, most for my own education, but nothing blocking.
The only thing that concerns me a little is that the view diffing logic used to account for "do not return viewsToRemove that are in viewsToAdd" (the views that needed to move), but it doesn't anymore; it is now a comment in the submitFrame method.
I think this should be handled either by the diffing (or maybe, more semantically by the ViewListDiffResult class), so when the diff needs to be used by the submitFrame function (or the other theoretical places where that Diff might be needed), the behavior is already built-in, rather than in a comment that can be deleted or ignored (and not tested).
(Also: Diffing of view lists is growing/becoming smarter, maybe it's starting to need to be split from the main file, and tested separately?)
Also also: do we have a test-case that covers the issue in #105485, so it doesn't regress?
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
_| _|_|_| _|_|_|_|_| _| _|
_| _| _| _|_| _|_|
_| _| _|_| _| _| _| _|
_| _| _| _| _| _|
_|_|_|_| _|_|_| _| _| _|
| Iterable<DomElement> getElementsByTagName(String tag) => | ||
| createDomListWrapper<DomElement>(js_util | ||
| .callMethod<_DomList>(this, 'getElementsByTagName', <Object>[tag])); | ||
| external DomElement? get activeElement; |
There was a problem hiding this comment.
I wonder why this was marked as nullable before? (Looks like a carry-over from dart:html Node.isConnected, so I guess it's to accomodate older browsers where this property was not even defined, like IE)
|
(This has gotten reverted due to a lint failure) |
4 participants
Reworks embedded view manager to be simpler, more well documented, and fix bugs.
Fixes flutter/flutter#105485
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.