From 53392972bea9accb9272abde5b209683d4f0cece Mon Sep 17 00:00:00 2001 From: Harry Terkelsen <1961493+harryterkelsen@users.noreply.github.com> Date: Wed, 20 Dec 2023 11:12:28 -0800 Subject: [PATCH] [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (#48758) Previously, we would do this any time a view was disposed, which would clear ALL fragment programs in the app, not just the ones associated with the view. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- .../src/engine/canvaskit/embedded_views.dart | 5 +- .../lib/src/engine/canvaskit/renderer.dart | 2 + lib/web_ui/lib/src/engine/html/renderer.dart | 1 + .../engine/skwasm/skwasm_impl/renderer.dart | 1 + lib/web_ui/lib/src/engine/window.dart | 105 +++++++++++------- .../test/canvaskit/embedded_views_test.dart | 13 ++- 6 files changed, 81 insertions(+), 46 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart index 343894b1d8477..e42c9a3327793 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/embedded_views.dart @@ -440,8 +440,7 @@ class HtmlViewEmbedder { sceneHost.insertBefore(platformViewRoot, elementToInsertBefore); final RenderCanvas? overlay = _overlays[viewId]; if (overlay != null) { - sceneHost.insertBefore( - overlay.htmlElement, elementToInsertBefore); + sceneHost.insertBefore(overlay.htmlElement, elementToInsertBefore); } } else { final DomElement platformViewRoot = _viewClipChains[viewId]!.root; @@ -651,6 +650,8 @@ class HtmlViewEmbedder { } } _svgClipDefs.clear(); + _svgPathDefs?.remove(); + _svgPathDefs = null; } static void removeElement(DomElement element) { diff --git a/lib/web_ui/lib/src/engine/canvaskit/renderer.dart b/lib/web_ui/lib/src/engine/canvaskit/renderer.dart index deb63b4b159bf..df9ae2fa20b54 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/renderer.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/renderer.dart @@ -85,6 +85,7 @@ class CanvasKitRenderer implements Renderer { viewManager.onViewDisposed.listen(_onViewDisposed); _instance = this; }(); + registerHotRestartListener(dispose); return _initialized; } @@ -451,6 +452,7 @@ class CanvasKitRenderer implements Renderer { rasterizer.dispose(); } _rasterizers.clear(); + clearFragmentProgramCache(); } @override diff --git a/lib/web_ui/lib/src/engine/html/renderer.dart b/lib/web_ui/lib/src/engine/html/renderer.dart index 0cf11314f8d74..afada4fca1896 100644 --- a/lib/web_ui/lib/src/engine/html/renderer.dart +++ b/lib/web_ui/lib/src/engine/html/renderer.dart @@ -31,6 +31,7 @@ class HtmlRenderer implements Renderer { // to make the unpacking happen while we are waiting for network requests. lineLookup; }); + registerHotRestartListener(clearFragmentProgramCache); _instance = this; } diff --git a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart index 292994ec18122..df4cf213a33a3 100644 --- a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart +++ b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/renderer.dart @@ -349,6 +349,7 @@ class SkwasmRenderer implements Renderer { FutureOr initialize() { surface = SkwasmSurface(); sceneView = EngineSceneView(SkwasmPictureRenderer(surface)); + registerHotRestartListener(clearFragmentProgramCache); } @override diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index a57cd1919edeb..6ed35bb9256a2 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -9,7 +9,7 @@ import 'package:meta/meta.dart'; import 'package:ui/ui.dart' as ui; import 'package:ui/ui_web/src/ui_web.dart' as ui_web; -import '../engine.dart' show DimensionsProvider, registerHotRestartListener, renderer; +import '../engine.dart' show DimensionsProvider, registerHotRestartListener; import 'browser_detection.dart'; import 'display.dart'; import 'dom.dart'; @@ -59,7 +59,8 @@ base class EngineFlutterView implements ui.FlutterView { // by the public `EngineFlutterView` constructor). DomElement? hostElement, ) : embeddingStrategy = EmbeddingStrategy.create(hostElement: hostElement), - dimensionsProvider = DimensionsProvider.create(hostElement: hostElement) { + dimensionsProvider = + DimensionsProvider.create(hostElement: hostElement) { // The embeddingStrategy will take care of cleaning up the rootElement on // hot restart. embeddingStrategy.attachViewRoot(dom.rootElement); @@ -71,7 +72,8 @@ base class EngineFlutterView implements ui.FlutterView { static EngineFlutterWindow implicit( EnginePlatformDispatcher platformDispatcher, DomElement? hostElement, - ) => EngineFlutterWindow._(platformDispatcher, hostElement); + ) => + EngineFlutterWindow._(platformDispatcher, hostElement); @override final int viewId; @@ -101,8 +103,6 @@ base class EngineFlutterView implements ui.FlutterView { dimensionsProvider.close(); pointerBinding.dispose(); dom.rootElement.remove(); - // TODO(harryterkelsen): What should we do about this in multi-view? - renderer.clearFragmentProgramCache(); semantics.reset(); } @@ -115,7 +115,8 @@ base class EngineFlutterView implements ui.FlutterView { @override void updateSemantics(ui.SemanticsUpdate update) { - assert(!isDisposed, 'Trying to update semantics on a disposed EngineFlutterView.'); + assert(!isDisposed, + 'Trying to update semantics on a disposed EngineFlutterView.'); semantics.updateSemantics(update); } @@ -128,7 +129,8 @@ base class EngineFlutterView implements ui.FlutterView { late final ContextMenu contextMenu = ContextMenu(dom.rootElement); - late final DomManager dom = DomManager(viewId: viewId, devicePixelRatio: devicePixelRatio); + late final DomManager dom = + DomManager(viewId: viewId, devicePixelRatio: devicePixelRatio); late final PlatformViewMessageHandler platformViewMessageHandler = PlatformViewMessageHandler(platformViewsContainer: dom.platformViewsHost); @@ -137,9 +139,11 @@ base class EngineFlutterView implements ui.FlutterView { // TODO(goderbauer): Provide API to configure constraints. See also TODO in "render". @override - ViewConstraints get physicalConstraints => ViewConstraints.tight(physicalSize); + ViewConstraints get physicalConstraints => + ViewConstraints.tight(physicalSize); - late final EngineSemanticsOwner semantics = EngineSemanticsOwner(dom.semanticsHost); + late final EngineSemanticsOwner semantics = + EngineSemanticsOwner(dom.semanticsHost); @override ui.Size get physicalSize { @@ -188,7 +192,8 @@ base class EngineFlutterView implements ui.FlutterView { ui.GestureSettings get gestureSettings => _viewConfiguration.gestureSettings; @override - List get displayFeatures => _viewConfiguration.displayFeatures; + List get displayFeatures => + _viewConfiguration.displayFeatures; @override EngineFlutterDisplay get display => EngineFlutterDisplay.instance; @@ -244,11 +249,14 @@ base class EngineFlutterView implements ui.FlutterView { // Return false if the previous dimensions are not set. if (_physicalSize != null) { // First confirm both height and width are effected. - if (_physicalSize!.height != newPhysicalSize.height && _physicalSize!.width != newPhysicalSize.width) { + if (_physicalSize!.height != newPhysicalSize.height && + _physicalSize!.width != newPhysicalSize.width) { // If prior to rotation height is bigger than width it should be the // opposite after the rotation and vice versa. - if ((_physicalSize!.height > _physicalSize!.width && newPhysicalSize.height < newPhysicalSize.width) || - (_physicalSize!.width > _physicalSize!.height && newPhysicalSize.width < newPhysicalSize.height)) { + if ((_physicalSize!.height > _physicalSize!.width && + newPhysicalSize.height < newPhysicalSize.width) || + (_physicalSize!.width > _physicalSize!.height && + newPhysicalSize.width < newPhysicalSize.height)) { // Rotation detected return true; } @@ -273,7 +281,8 @@ final class _EngineFlutterViewImpl extends EngineFlutterView { } /// The Web implementation of [ui.SingletonFlutterWindow]. -final class EngineFlutterWindow extends EngineFlutterView implements ui.SingletonFlutterWindow { +final class EngineFlutterWindow extends EngineFlutterView + implements ui.SingletonFlutterWindow { EngineFlutterWindow._( EnginePlatformDispatcher platformDispatcher, DomElement? hostElement, @@ -320,7 +329,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto double get textScaleFactor => platformDispatcher.textScaleFactor; @override - bool get nativeSpellCheckServiceDefined => platformDispatcher.nativeSpellCheckServiceDefined; + bool get nativeSpellCheckServiceDefined => + platformDispatcher.nativeSpellCheckServiceDefined; @override bool get brieflyShowPassword => platformDispatcher.brieflyShowPassword; @@ -329,7 +339,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto bool get alwaysUse24HourFormat => platformDispatcher.alwaysUse24HourFormat; @override - ui.VoidCallback? get onTextScaleFactorChanged => platformDispatcher.onTextScaleFactorChanged; + ui.VoidCallback? get onTextScaleFactorChanged => + platformDispatcher.onTextScaleFactorChanged; @override set onTextScaleFactorChanged(ui.VoidCallback? callback) { platformDispatcher.onTextScaleFactorChanged = callback; @@ -339,7 +350,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto ui.Brightness get platformBrightness => platformDispatcher.platformBrightness; @override - ui.VoidCallback? get onPlatformBrightnessChanged => platformDispatcher.onPlatformBrightnessChanged; + ui.VoidCallback? get onPlatformBrightnessChanged => + platformDispatcher.onPlatformBrightnessChanged; @override set onPlatformBrightnessChanged(ui.VoidCallback? callback) { platformDispatcher.onPlatformBrightnessChanged = callback; @@ -349,7 +361,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto String? get systemFontFamily => platformDispatcher.systemFontFamily; @override - ui.VoidCallback? get onSystemFontFamilyChanged => platformDispatcher.onSystemFontFamilyChanged; + ui.VoidCallback? get onSystemFontFamilyChanged => + platformDispatcher.onSystemFontFamilyChanged; @override set onSystemFontFamilyChanged(ui.VoidCallback? callback) { platformDispatcher.onSystemFontFamilyChanged = callback; @@ -377,7 +390,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto } @override - ui.PointerDataPacketCallback? get onPointerDataPacket => platformDispatcher.onPointerDataPacket; + ui.PointerDataPacketCallback? get onPointerDataPacket => + platformDispatcher.onPointerDataPacket; @override set onPointerDataPacket(ui.PointerDataPacketCallback? callback) { platformDispatcher.onPointerDataPacket = callback; @@ -400,7 +414,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto bool get semanticsEnabled => platformDispatcher.semanticsEnabled; @override - ui.VoidCallback? get onSemanticsEnabledChanged => platformDispatcher.onSemanticsEnabledChanged; + ui.VoidCallback? get onSemanticsEnabledChanged => + platformDispatcher.onSemanticsEnabledChanged; @override set onSemanticsEnabledChanged(ui.VoidCallback? callback) { platformDispatcher.onSemanticsEnabledChanged = callback; @@ -415,7 +430,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto set onFrameDataChanged(ui.VoidCallback? callback) {} @override - ui.AccessibilityFeatures get accessibilityFeatures => platformDispatcher.accessibilityFeatures; + ui.AccessibilityFeatures get accessibilityFeatures => + platformDispatcher.accessibilityFeatures; @override ui.VoidCallback? get onAccessibilityFeaturesChanged => @@ -435,14 +451,16 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto } @override - ui.PlatformMessageCallback? get onPlatformMessage => platformDispatcher.onPlatformMessage; + ui.PlatformMessageCallback? get onPlatformMessage => + platformDispatcher.onPlatformMessage; @override set onPlatformMessage(ui.PlatformMessageCallback? callback) { platformDispatcher.onPlatformMessage = callback; } @override - void setIsolateDebugName(String name) => ui.PlatformDispatcher.instance.setIsolateDebugName(name); + void setIsolateDebugName(String name) => + ui.PlatformDispatcher.instance.setIsolateDebugName(name); /// Handles the browser history integration to allow users to use the back /// button, etc. @@ -548,7 +566,8 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto Future handleNavigationMessage(ByteData? data) async { return _waitInTheLine(() async { final MethodCall decoded = const JSONMethodCodec().decodeMethodCall(data); - final Map? arguments = decoded.arguments as Map?; + final Map? arguments = + decoded.arguments as Map?; switch (decoded.method) { case 'selectMultiEntryHistory': await _useMultiEntryBrowserHistory(); @@ -572,7 +591,9 @@ final class EngineFlutterWindow extends EngineFlutterView implements ui.Singleto path = Uri.decodeComponent( Uri( path: uri.path.isEmpty ? '/' : uri.path, - queryParameters: uri.queryParametersAll.isEmpty ? null : uri.queryParametersAll, + queryParameters: uri.queryParametersAll.isEmpty + ? null + : uri.queryParametersAll, fragment: uri.fragment.isEmpty ? null : uri.fragment, ).toString(), ); @@ -648,6 +669,7 @@ EngineFlutterWindow get window { ); return _window!; } + EngineFlutterWindow? _window; /// Initializes the [window] (aka the implicit view), if it's not already @@ -693,10 +715,10 @@ class ViewConstraints implements ui.ViewConstraints { }); ViewConstraints.tight(ui.Size size) - : minWidth = size.width, - maxWidth = size.width, - minHeight = size.height, - maxHeight = size.height; + : minWidth = size.width, + maxWidth = size.width, + minHeight = size.height, + maxHeight = size.height; @override final double minWidth; @@ -709,15 +731,17 @@ class ViewConstraints implements ui.ViewConstraints { @override bool isSatisfiedBy(ui.Size size) { - return (minWidth <= size.width) && (size.width <= maxWidth) && - (minHeight <= size.height) && (size.height <= maxHeight); + return (minWidth <= size.width) && + (size.width <= maxWidth) && + (minHeight <= size.height) && + (size.height <= maxHeight); } @override bool get isTight => minWidth >= maxWidth && minHeight >= maxHeight; @override - ViewConstraints operator/(double factor) { + ViewConstraints operator /(double factor) { return ViewConstraints( minWidth: minWidth / factor, maxWidth: maxWidth / factor, @@ -734,11 +758,11 @@ class ViewConstraints implements ui.ViewConstraints { if (other.runtimeType != runtimeType) { return false; } - return other is ViewConstraints - && other.minWidth == minWidth - && other.maxWidth == maxWidth - && other.minHeight == minHeight - && other.maxHeight == maxHeight; + return other is ViewConstraints && + other.minWidth == minWidth && + other.maxWidth == maxWidth && + other.minHeight == minHeight && + other.maxHeight == maxHeight; } @override @@ -749,8 +773,10 @@ class ViewConstraints implements ui.ViewConstraints { if (minWidth == double.infinity && minHeight == double.infinity) { return 'ViewConstraints(biggest)'; } - if (minWidth == 0 && maxWidth == double.infinity && - minHeight == 0 && maxHeight == double.infinity) { + if (minWidth == 0 && + maxWidth == double.infinity && + minHeight == 0 && + maxHeight == double.infinity) { return 'ViewConstraints(unconstrained)'; } String describe(double min, double max, String dim) { @@ -759,6 +785,7 @@ class ViewConstraints implements ui.ViewConstraints { } return '${min.toStringAsFixed(1)}<=$dim<=${max.toStringAsFixed(1)}'; } + final String width = describe(minWidth, maxWidth, 'w'); final String height = describe(minHeight, maxHeight, 'h'); return 'ViewConstraints($width, $height)'; diff --git a/lib/web_ui/test/canvaskit/embedded_views_test.dart b/lib/web_ui/test/canvaskit/embedded_views_test.dart index 57ef4a8d74c31..c90a9e00b318b 100644 --- a/lib/web_ui/test/canvaskit/embedded_views_test.dart +++ b/lib/web_ui/test/canvaskit/embedded_views_test.dart @@ -731,12 +731,15 @@ void testMain() { await renderScene(sb.build()); } - final DomNode skPathDefs = sceneHost.querySelector('#sk_path_defs')!; - - expect(skPathDefs.childNodes, hasLength(0)); - await renderTestScene(); - expect(skPathDefs.childNodes, hasLength(1)); + + final DomElement? skPathDefs = sceneHost.querySelector('#sk_path_defs'); + expect( + skPathDefs, + isNotNull, + reason: 'Should have created SVG paths after rendering the scene', + ); + expect(skPathDefs!.childNodes, hasLength(1)); await renderTestScene(); expect(skPathDefs.childNodes, hasLength(1));