[web] EngineFlutterView.dispose() #48183
Conversation
| final class _EngineFlutterViewImpl extends EngineFlutterView { | ||
| _EngineFlutterViewImpl( | ||
| super.viewId, | ||
| super.platformDispatcher, | ||
| super.hostElement, | ||
| ) : super._() { | ||
| registerHotRestartListener(() { | ||
| // TODO(harryterkelsen): What should we do about this in multi-view? | ||
| renderer.clearFragmentProgramCache(); | ||
| _dimensionsProvider.close(); | ||
| }); | ||
| } | ||
| ) : super._(); | ||
| } |
There was a problem hiding this comment.
This extension is unneeded now, right?
There was a problem hiding this comment.
It is still needed to tell the compiler which super constructor I want to use (it's not the default constructor).
| @override | ||
| void render(ui.Scene scene) => platformDispatcher.render(scene, this); | ||
| void render(ui.Scene scene) { | ||
| assert(!isDisposed, 'Trying to render a disposed EngineFlutterView.'); |
There was a problem hiding this comment.
Not 100% sure, but I think this should not be an assert, just a noop.
(Also since this is being called from views[id].render(), it's likely that this assert will only be exercised in test code, right?)
There was a problem hiding this comment.
(Also since this is being called from
views[id].render(), it's likely that this assert will only be exercised in test code, right?)
In theory, yes. In practice, the assert here serves as a guard against bugs in our logic if we somehow try to render to a disposed view.
If the framework tries to render to a disposed view, then the platform dispatcher (or renderer) should do viewData[id]?.render(scene) (which would fail to find the view, and it'll be a noop). I don't think we should allow calling render directly on a disposed view.
| @override | ||
| void updateSemantics(ui.SemanticsUpdate update) => platformDispatcher.updateSemantics(update); | ||
| void updateSemantics(ui.SemanticsUpdate update) { | ||
| assert(!isDisposed, 'Trying to update semantics on a disposed EngineFlutterView.'); |
There was a problem hiding this comment.
(Similar comment to the one in the render method)
| @mustCallSuper | ||
| void dispose() { | ||
| isDisposed = true; | ||
| platformDispatcher.unregisterView(this); |
There was a problem hiding this comment.
Right now the view constructor is automatically doing register, and this needs to be here (but we said that registration wouldn't happen automatically upon creation, so this will go in that case, right?)
There was a problem hiding this comment.
Yep. dispose should undo anything that this class did. If the view is registering itself, then it should also unregister itself (in dispose). If someone else is registering the view, then that someone should handle the unregistering too.
…138739) flutter/engine@1f1e26c...255f967 2023-11-20 mdebbar@google.com [web] EngineFlutterView.dispose() (flutter/engine#48183) 2023-11-20 skia-flutter-autoroll@skia.org Roll Skia from 64d1e14df168 to 795a259c2f56 (2 revisions) (flutter/engine#48227) 2023-11-20 skia-flutter-autoroll@skia.org Roll Skia from a874e29d71c0 to 64d1e14df168 (1 revision) (flutter/engine#48225) 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 jonahwilliams@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
EngineFlutterView.dispose()to cleanup when the view is removed (and in hot restart).EnginePlatformDispatcher.dispose()now disposes of all of its registered views.