From 5733891c8d867e1bb56413e2791031fa04de5a14 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Wed, 22 Nov 2023 16:27:56 -0500 Subject: [PATCH] [web] Hook the new JS API to the FlutterViewManager (#48283) - Auto-generate view IDs. - Views don't auto-register/auto-unregister anymore. - Remove `EnginePlatformDispatcher.registerView/unregisterView` methods. - Add `FlutterViewManager.createAndRegisterView/disposeAndUnregisterView/dispose` methods. - Hook the `addView`/`removeView` JS APIs to `FlutterViewManager`. --- lib/web_ui/lib/src/engine/app_bootstrap.dart | 17 ++--- .../lib/src/engine/platform_dispatcher.dart | 21 +----- .../view_embedder/flutter_view_manager.dart | 35 +++++++++- lib/web_ui/lib/src/engine/window.dart | 42 +++++++----- .../platform_dispatcher_test.dart | 37 ++++++----- lib/web_ui/test/engine/routing_test.dart | 18 ++---- .../engine/surface/platform_view_test.dart | 3 +- .../flutter_view_manager_test.dart | 23 +++---- lib/web_ui/test/engine/window_test.dart | 64 +++++++++++++++---- 9 files changed, 150 insertions(+), 110 deletions(-) diff --git a/lib/web_ui/lib/src/engine/app_bootstrap.dart b/lib/web_ui/lib/src/engine/app_bootstrap.dart index 6dc59af80f5ef..ac09fc0c39bfe 100644 --- a/lib/web_ui/lib/src/engine/app_bootstrap.dart +++ b/lib/web_ui/lib/src/engine/app_bootstrap.dart @@ -2,15 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:js_interop' show JSAny; - -import 'package:ui/src/engine/dom.dart'; - import 'configuration.dart'; import 'js_interop/js_app.dart'; import 'js_interop/js_loader.dart'; import 'platform_dispatcher.dart'; +import 'view_embedder/flutter_view_manager.dart'; /// The type of a function that initializes an engine (in Dart). typedef InitEngineFn = Future Function([JsFlutterConfiguration? params]); @@ -66,22 +63,18 @@ class AppBootstrap { }); } + FlutterViewManager get viewManager => EnginePlatformDispatcher.instance.viewManager; + /// Represents the App that was just started, and its JS API. FlutterApp _prepareFlutterApp() { return FlutterApp( addView: (JsFlutterViewOptions options) async { assert(configuration.multiViewEnabled, 'Cannot addView when multiView is not enabled'); - // NEXT: Create a view from JS Options, then register it in - // the viewManager... (Or have a method that creates-and-registers a view) - // final EngineFlutterView view = View.createFromOptions(options); - // return EnginePlatformDispatcher.instance.viewManager.registerView(view).viewId; - domWindow.console.warn('Create view from JS is unimplemented!'); - domWindow.console.warn((options as JSAny).toObjectDeep); - return -1; + return viewManager.createAndRegisterView(options).viewId; }, removeView: (int viewId) async { assert(configuration.multiViewEnabled, 'Cannot removeView when multiView is not enabled'); - return EnginePlatformDispatcher.instance.viewManager.unregisterView(viewId); + return viewManager.disposeAndUnregisterView(viewId); } ); } diff --git a/lib/web_ui/lib/src/engine/platform_dispatcher.dart b/lib/web_ui/lib/src/engine/platform_dispatcher.dart index 5a848e5513931..00782b2e1906a 100644 --- a/lib/web_ui/lib/src/engine/platform_dispatcher.dart +++ b/lib/web_ui/lib/src/engine/platform_dispatcher.dart @@ -104,12 +104,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { _disconnectFontSizeObserver(); _removeLocaleChangedListener(); HighContrastSupport.instance.removeListener(_updateHighContrast); - - // We need to call `toList()` in order to avoid concurrent modification inside - // the loop (`view.dispose()` removes itself from the view map). - for (final EngineFlutterView view in views.toList()) { - view.dispose(); - } + viewManager.dispose(); } /// Receives all events related to platform configuration changes. @@ -136,19 +131,7 @@ class EnginePlatformDispatcher extends ui.PlatformDispatcher { EngineFlutterDisplay.instance, ]; - final FlutterViewManager viewManager = FlutterViewManager(); - - /// Adds [view] to the platform dispatcher's registry of [views]. - void registerView(EngineFlutterView view) { - viewManager.registerView(view); - } - - /// Removes [view] from the platform dispatcher's registry of [views]. - /// - /// Nothing happens if the view is not already registered. - void unregisterView(EngineFlutterView view) { - viewManager.unregisterView(view.viewId); - } + late final FlutterViewManager viewManager = FlutterViewManager(this); /// The current list of windows. @override diff --git a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart index 577644e702ada..0024c059da4ed 100644 --- a/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart +++ b/lib/web_ui/lib/src/engine/view_embedder/flutter_view_manager.dart @@ -6,6 +6,10 @@ import 'package:ui/src/engine.dart'; /// Encapsulates view objects, and their optional metadata indexed by `viewId`. class FlutterViewManager { + FlutterViewManager(this._dispatcher); + + final EnginePlatformDispatcher _dispatcher; + // A map of EngineFlutterViews indexed by their viewId. final Map _viewData = {}; // A map of (optional) JsFlutterViewOptions, indexed by their viewId. @@ -26,6 +30,14 @@ class FlutterViewManager { return _viewData[viewId]; } + EngineFlutterView createAndRegisterView( + JsFlutterViewOptions jsViewOptions, + ) { + final EngineFlutterView view = EngineFlutterView(_dispatcher, jsViewOptions.hostElement); + registerView(view, jsViewOptions: jsViewOptions); + return view; + } + /// Stores a [view] and its (optional) [jsViewOptions], indexed by `viewId`. /// /// Returns the registered [view]. @@ -34,9 +46,7 @@ class FlutterViewManager { JsFlutterViewOptions? jsViewOptions, }) { final int viewId = view.viewId; - // This assert knows of kImplicitViewId, so some tests like test/engine/routing_test.dart - // can pass. The kImplicitViewId exception should be removed! - assert(viewId == kImplicitViewId || !_viewData.containsKey(viewId)); // Adding the same view twice? + assert(!_viewData.containsKey(viewId)); // Adding the same view twice? // Store the view, and the jsViewOptions, if any... _viewData[viewId] = view; @@ -48,6 +58,16 @@ class FlutterViewManager { return view; } + JsFlutterViewOptions? disposeAndUnregisterView(int viewId) { + final EngineFlutterView? view = _viewData[viewId]; + if (view == null) { + return null; + } + final JsFlutterViewOptions? options = unregisterView(viewId); + view.dispose(); + return options; + } + /// Un-registers [viewId]. /// /// Returns its [JsFlutterViewOptions] (if any). @@ -65,4 +85,13 @@ class FlutterViewManager { JsFlutterViewOptions? getOptions(int viewId) { return _jsViewOptions[viewId]; } + + void dispose() { + // We need to call `toList()` in order to avoid concurrent modification + // inside the loop. + _viewData.keys.toList().forEach(disposeAndUnregisterView); + // Let listeners receive the unregistration events from the loop above, then + // close the stream. + _onViewsChangedController.close(); + } } diff --git a/lib/web_ui/lib/src/engine/window.dart b/lib/web_ui/lib/src/engine/window.dart index 6be8d1ba9e9b1..819f9a1aa1339 100644 --- a/lib/web_ui/lib/src/engine/window.dart +++ b/lib/web_ui/lib/src/engine/window.dart @@ -31,6 +31,8 @@ const bool debugPrintPlatformMessages = false; /// The view ID for the implicit flutter view provided by the platform. const int kImplicitViewId = 0; +int _nextViewId = kImplicitViewId + 1; + /// Represents all views in the Flutter Web Engine. /// /// In addition to everything defined in [ui.FlutterView], this class adds @@ -41,7 +43,6 @@ base class EngineFlutterView implements ui.FlutterView { /// The [hostElement] parameter specifies the container in the DOM into which /// the Flutter view will be rendered. factory EngineFlutterView( - int viewId, EnginePlatformDispatcher platformDispatcher, DomElement hostElement, ) = _EngineFlutterViewImpl; @@ -55,13 +56,17 @@ base class EngineFlutterView implements ui.FlutterView { DomElement? hostElement, ) : embeddingStrategy = EmbeddingStrategy.create(hostElement: hostElement), dimensionsProvider = DimensionsProvider.create(hostElement: hostElement) { - platformDispatcher.registerView(this); // The embeddingStrategy will take care of cleaning up the rootElement on // hot restart. embeddingStrategy.attachGlassPane(dom.rootElement); registerHotRestartListener(dispose); } + static EngineFlutterWindow implicit( + EnginePlatformDispatcher platformDispatcher, + DomElement? hostElement, + ) => EngineFlutterWindow._(platformDispatcher, hostElement); + @override final int viewId; @@ -80,8 +85,10 @@ base class EngineFlutterView implements ui.FlutterView { /// tree and any event listeners. @mustCallSuper void dispose() { + if (isDisposed) { + return; + } isDisposed = true; - platformDispatcher.unregisterView(this); dimensionsProvider.close(); dom.rootElement.remove(); // TODO(harryterkelsen): What should we do about this in multi-view? @@ -186,19 +193,17 @@ base class EngineFlutterView implements ui.FlutterView { final class _EngineFlutterViewImpl extends EngineFlutterView { _EngineFlutterViewImpl( - super.viewId, - super.platformDispatcher, - super.hostElement, - ) : super._(); + EnginePlatformDispatcher platformDispatcher, + DomElement hostElement, + ) : super._(_nextViewId++, platformDispatcher, hostElement); } /// The Web implementation of [ui.SingletonFlutterWindow]. final class EngineFlutterWindow extends EngineFlutterView implements ui.SingletonFlutterWindow { - EngineFlutterWindow( - super.viewId, - super.platformDispatcher, - super.hostElement, - ) : super._() { + EngineFlutterWindow._( + EnginePlatformDispatcher platformDispatcher, + DomElement? hostElement, + ) : super._(kImplicitViewId, platformDispatcher, hostElement) { if (ui_web.isCustomUrlStrategySet) { _browserHistory = createHistoryForExistingState(ui_web.urlStrategy); } @@ -616,11 +621,14 @@ EngineFlutterWindow? _window; EngineFlutterWindow ensureImplicitViewInitialized({ DomElement? hostElement, }) { - return _window ??= EngineFlutterWindow( - kImplicitViewId, - EnginePlatformDispatcher.instance, - hostElement, - ); + if (_window == null) { + _window = EngineFlutterView.implicit( + EnginePlatformDispatcher.instance, + hostElement, + ); + EnginePlatformDispatcher.instance.viewManager.registerView(_window!); + } + return _window!; } /// The Web implementation of [ui.ViewPadding]. diff --git a/lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart b/lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart index c228b5e6a03fb..38a3096035588 100644 --- a/lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart +++ b/lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart @@ -203,27 +203,26 @@ void testMain() { test('disposes all its views', () { final EnginePlatformDispatcher dispatcher = EnginePlatformDispatcher(); - final EngineFlutterView view20 = - EngineFlutterView(20, dispatcher, createDomHTMLDivElement()); - final EngineFlutterView view21 = - EngineFlutterView(21, dispatcher, createDomHTMLDivElement()); - final EngineFlutterView view22 = - EngineFlutterView(22, dispatcher, createDomHTMLDivElement()); - - // Add this again when views don't register themselves upon instantiation. - // dispatcher - // ..registerView(view20) - // ..registerView(view21) - // ..registerView(view22); - - expect(view20.isDisposed, isFalse); - expect(view21.isDisposed, isFalse); - expect(view22.isDisposed, isFalse); + final EngineFlutterView view1 = + EngineFlutterView(dispatcher, createDomHTMLDivElement()); + final EngineFlutterView view2 = + EngineFlutterView(dispatcher, createDomHTMLDivElement()); + final EngineFlutterView view3 = + EngineFlutterView(dispatcher, createDomHTMLDivElement()); + + dispatcher.viewManager + ..registerView(view1) + ..registerView(view2) + ..registerView(view3); + + expect(view1.isDisposed, isFalse); + expect(view2.isDisposed, isFalse); + expect(view3.isDisposed, isFalse); dispatcher.dispose(); - expect(view20.isDisposed, isTrue); - expect(view21.isDisposed, isTrue); - expect(view22.isDisposed, isTrue); + expect(view1.isDisposed, isTrue); + expect(view2.isDisposed, isTrue); + expect(view3.isDisposed, isTrue); }); }); } diff --git a/lib/web_ui/test/engine/routing_test.dart b/lib/web_ui/test/engine/routing_test.dart index 68185c40420ee..ddc9669dd8374 100644 --- a/lib/web_ui/test/engine/routing_test.dart +++ b/lib/web_ui/test/engine/routing_test.dart @@ -11,7 +11,6 @@ import 'package:ui/ui.dart' as ui; import 'package:ui/ui_web/src/ui_web.dart' as ui_web; import '../common/matchers.dart'; -import '../common/test_initialization.dart'; import 'history_test.dart'; const MethodCodec codec = JSONMethodCodec(); @@ -28,26 +27,19 @@ void main() { } void testMain() { - EngineFlutterWindow? savedWindow; late EngineFlutterWindow myWindow; - setUpAll(() async { - await bootstrapAndRunApp(); - }); + final EnginePlatformDispatcher dispatcher = EnginePlatformDispatcher.instance; setUp(() { - savedWindow = EnginePlatformDispatcher.instance.implicitView; - myWindow = EngineFlutterWindow(0, EnginePlatformDispatcher.instance, createDomHTMLDivElement()); + myWindow = EngineFlutterView.implicit(dispatcher, createDomHTMLDivElement()); + dispatcher.viewManager.registerView(myWindow); }); tearDown(() async { + dispatcher.viewManager.unregisterView(myWindow.viewId); await myWindow.resetHistory(); - - // Restore the original implicit view. - EnginePlatformDispatcher.instance.unregisterView(myWindow); - if (savedWindow != null) { - EnginePlatformDispatcher.instance.registerView(savedWindow!); - } + myWindow.dispose(); }); // For now, web always has an implicit view provided by the web engine. diff --git a/lib/web_ui/test/engine/surface/platform_view_test.dart b/lib/web_ui/test/engine/surface/platform_view_test.dart index 64dd789d4f698..9cb19a40e5016 100644 --- a/lib/web_ui/test/engine/surface/platform_view_test.dart +++ b/lib/web_ui/test/engine/surface/platform_view_test.dart @@ -14,8 +14,7 @@ import '../../common/matchers.dart'; import '../../common/test_initialization.dart'; const MethodCodec codec = StandardMethodCodec(); -final EngineFlutterWindow window = EngineFlutterWindow( - 0, +final EngineFlutterWindow window = EngineFlutterView.implicit( EnginePlatformDispatcher.instance, createDomHTMLDivElement(), ); diff --git a/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart b/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart index 238aa01cd6b72..f7f154c69a176 100644 --- a/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart +++ b/lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart @@ -17,15 +17,13 @@ void main() { Future doTests() async { group('FlutterViewManager', () { - int nextViewId = 1; // We keep track of this so we don't have to unregister from PlatformDispatcher. - final EnginePlatformDispatcher platformDispatcher = EnginePlatformDispatcher.instance; - final FlutterViewManager viewManager = FlutterViewManager(); + final FlutterViewManager viewManager = FlutterViewManager(platformDispatcher); group('registerView', () { test('can register view', () { - final int viewId = nextViewId++; - final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div')); + final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); + final int viewId = view.viewId; viewManager.registerView(view); @@ -33,8 +31,7 @@ Future doTests() async { }); test('fails if the same viewId is already registered', () { - final int viewId = nextViewId++; - final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div')); + final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); viewManager.registerView(view); @@ -42,8 +39,8 @@ Future doTests() async { }); test('stores JSOptions that getOptions can retrieve', () { - final int viewId = nextViewId++; - final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div')); + final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); + final int viewId = view.viewId; final JsFlutterViewOptions expectedOptions = jsify({ 'hostElement': createDomElement('div'), }) as JsFlutterViewOptions; @@ -57,8 +54,8 @@ Future doTests() async { group('unregisterView', () { test('unregisters a view', () { - final int viewId = nextViewId++; - final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div')); + final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); + final int viewId = view.viewId; viewManager.registerView(view); expect(viewManager[viewId], isNotNull); @@ -78,8 +75,8 @@ Future doTests() async { }); test('on view registered/unregistered - fires event', () async { - final int viewId = nextViewId++; - final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div')); + final EngineFlutterView view = EngineFlutterView(platformDispatcher, createDomElement('div')); + final int viewId = view.viewId; final Future> viewChangeEvents = onViewsChanged.toList(); viewManager.registerView(view); diff --git a/lib/web_ui/test/engine/window_test.dart b/lib/web_ui/test/engine/window_test.dart index 23f2a2ee1fbb9..6138ea2379a0a 100644 --- a/lib/web_ui/test/engine/window_test.dart +++ b/lib/web_ui/test/engine/window_test.dart @@ -13,7 +13,6 @@ import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; import '../common/matchers.dart'; -import '../common/test_initialization.dart'; const int kPhysicalKeyA = 0x00070004; const int kLogicalKeyA = 0x00000000061; @@ -23,17 +22,18 @@ void main() { } Future testMain() async { - await bootstrapAndRunApp(); - late EngineFlutterWindow myWindow; + final EnginePlatformDispatcher dispatcher = EnginePlatformDispatcher.instance; setUp(() { - myWindow = EngineFlutterWindow(99, EnginePlatformDispatcher.instance, createDomHTMLDivElement()); + myWindow = EngineFlutterView.implicit(dispatcher, createDomHTMLDivElement()); + dispatcher.viewManager.registerView(myWindow); }); tearDown(() async { + dispatcher.viewManager.unregisterView(myWindow.viewId); await myWindow.resetHistory(); - EnginePlatformDispatcher.instance.unregisterView(myWindow); + myWindow.dispose(); }); test('onTextScaleFactorChanged preserves the zone', () { @@ -428,8 +428,6 @@ Future testMain() async { localeChangedCount += 1; }; - ensureFlutterViewEmbedderInitialized(); - // We populate the initial list of locales automatically (only test that we // got some locales; some contributors may be in different locales, so we // can't test the exact contents). @@ -465,13 +463,55 @@ Future testMain() async { await expectLater(completer.future, completes); }); + test('auto-view-id', () { + final DomElement host = createDomHTMLDivElement(); + final EngineFlutterView implicit1 = EngineFlutterView.implicit(dispatcher, host); + final EngineFlutterView implicit2 = EngineFlutterView.implicit(dispatcher, host); + + expect(implicit1.viewId, kImplicitViewId); + expect(implicit2.viewId, kImplicitViewId); + + final EngineFlutterView view1 = EngineFlutterView(dispatcher, host); + final EngineFlutterView view2 = EngineFlutterView(dispatcher, host); + final EngineFlutterView view3 = EngineFlutterView(dispatcher, host); + + expect(view1.viewId, isNot(kImplicitViewId)); + expect(view2.viewId, isNot(kImplicitViewId)); + expect(view3.viewId, isNot(kImplicitViewId)); + + expect(view1.viewId, isNot(view2.viewId)); + expect(view2.viewId, isNot(view3.viewId)); + expect(view3.viewId, isNot(view1.viewId)); + + implicit1.dispose(); + implicit2.dispose(); + view1.dispose(); + view2.dispose(); + view3.dispose(); + }); + + test('registration', () { + final DomHTMLDivElement host = createDomHTMLDivElement(); + final EnginePlatformDispatcher dispatcher = EnginePlatformDispatcher(); + expect(dispatcher.viewManager.views, isEmpty); + + // Creating the view shouldn't register it. + final EngineFlutterView view = EngineFlutterView(dispatcher, host); + expect(dispatcher.viewManager.views, isEmpty); + dispatcher.viewManager.registerView(view); + expect(dispatcher.viewManager.views, [view]); + + // Disposing the view shouldn't unregister it. + view.dispose(); + expect(dispatcher.viewManager.views, [view]); + + dispatcher.dispose(); + }); + test('dispose', () { final DomHTMLDivElement host = createDomHTMLDivElement(); - final EngineFlutterView view = EngineFlutterView( - 123, - EnginePlatformDispatcher.instance, - host, - ); + final EngineFlutterView view = + EngineFlutterView(EnginePlatformDispatcher.instance, host); // First, let's make sure the view's root element was inserted into the // host, and the dimensions provider is active.