Address PR comments.

Author: ditman

lib/web_ui/lib/src/engine/embedder.dart

@@ -6,7 +6,7 @@ import 'dart:async';
 
 import 'package:ui/ui.dart' as ui;
 
-import '../engine.dart' show buildMode, registerHotRestartListener, renderer, window;
+import '../engine.dart' show buildMode, renderer, window;
 import 'browser_detection.dart';
 import 'configuration.dart';
 import 'dom.dart';
@@ -45,7 +45,7 @@ class FlutterViewEmbedder {
   /// The hostElement is abstracted by an [EmbeddingStrategy] instance, which has
   /// different behavior depending on the `hostElement` value:
   ///
-  /// - A `null` `hostElement` will allow Flutter to take over the whole screen.
+  /// - A `null` `hostElement` will cause Flutter to take over the whole page.
   /// - A non-`null` `hostElement` will render flutter inside that element.
   FlutterViewEmbedder({DomElement? hostElement}) {
     // Create an appropriate EmbeddingStrategy using its factory...
@@ -58,16 +58,9 @@ class FlutterViewEmbedder {
     ));
 
     reset();
-
-    assert(() {
-      // _embeddingStrategy needs to clean-up stuff in the page on hot restart.
-      registerHotRestartListener(_embeddingStrategy.onHotRestart);
-      return true;
-    }());
   }
 
-  /// The [_embeddingStrategy] abstracts all the DOM manipulations required to
-  /// embed a Flutter app in the user-supplied `hostElement`.
+  /// Abstracts all the DOM manipulations required to embed a Flutter app in an user-supplied `hostElement`.
   late EmbeddingStrategy _embeddingStrategy;
 
   // The tag name for the root view of the flutter app (glass-pane)
@@ -147,7 +140,7 @@ class FlutterViewEmbedder {
 
     // Initializes the embeddingStrategy so it can host a single-view Flutter app.
     _embeddingStrategy.initialize(
-      embedderMetadata: <String, String>{
+      hostElementAttributes: <String, String>{
         'flt-renderer': '${renderer.rendererTag} ($rendererSelection)',
         'flt-build-mode': buildMode,
         // TODO(mdebbar): Disable spellcheck until changes in the framework and
@@ -247,6 +240,7 @@ class FlutterViewEmbedder {
   /// size if the change is caused by a rotation.
   void _metricsDidChange(ui.Size? newSize) {
     updateSemanticsScreenProperties();
+    // TODO(dit): Do not computePhysicalSize twice, https://github.com/flutter/flutter/issues/117036
     if (isMobile && !window.isRotation() && textEditing.isEditing) {
       window.computeOnScreenKeyboardInsets(true);
       EnginePlatformDispatcher.instance.invokeOnMetricsChanged();

lib/web_ui/lib/src/engine/view_embedder/dimensions_provider/custom_element_dimensions_provider.dart

@@ -10,12 +10,13 @@ import 'package:ui/ui.dart' as ui show Size;
 
 import 'dimensions_provider.dart';
 
-/// This class provides the real-time dimensions of a "hostElement".
+/// This class provides observable, real-time dimensions of a host element.
 ///
-/// Note: all the measurements returned from this class are potentially
-/// *expensive*, and should be cached as needed. Every call to every method on
-/// this class WILL perform actual DOM measurements.
+/// All the measurements returned from this class are potentially *expensive*,
+/// and should be cached as needed. Every call to every method on this class
+/// WILL perform actual DOM measurements.
 class CustomElementDimensionsProvider extends DimensionsProvider {
+  /// Creates a [CustomElementDimensionsProvider] from a [_hostElement].
   CustomElementDimensionsProvider(this._hostElement) {
     // Hook up a resize observer on the hostElement (if supported!).
     _hostElementResizeObserver = createDomResizeObserver((
@@ -39,6 +40,7 @@ class CustomElementDimensionsProvider extends DimensionsProvider {
     _hostElementResizeObserver?.observe(_hostElement);
   }
 
+  // The host element that will be used to retrieve (and observe) app size measurements.
   final DomElement _hostElement;
 
   // Handle resize events

lib/web_ui/lib/src/engine/view_embedder/dimensions_provider/dimensions_provider.dart

@@ -13,14 +13,17 @@ import 'full_page_dimensions_provider.dart';
 
 /// This class provides the dimensions of the "viewport" in which the app is rendered.
 ///
-///
 /// Similarly to the `EmbeddingStrategy`, this class is specialized to handle
 /// different sources of information:
 ///
 /// * [FullPageDimensionsProvider] - The default behavior, uses the VisualViewport
 ///   API to measure, and react to, the dimensions of the full browser window.
 /// * [CustomElementDimensionsProvider] - Uses a custom html Element as the source
 ///   of dimensions, and the ResizeObserver to notify the app of changes.
+///
+/// All the measurements returned from this class are potentially *expensive*,
+/// and should be cached as needed. Every call to every method on this class
+/// WILL perform actual DOM measurements.
 abstract class DimensionsProvider {
   DimensionsProvider();
 
@@ -40,6 +43,9 @@ abstract class DimensionsProvider {
   }
 
   /// Returns the [ui.Size] of the "viewport".
+  ///
+  /// This function is expensive. It triggers browser layout if there are
+  /// pending DOM writes.
   ui.Size computePhysicalSize();
 
   /// Returns the [WindowPadding] of the keyboard insets (if present).

lib/web_ui/lib/src/engine/view_embedder/dimensions_provider/full_page_dimensions_provider.dart

@@ -14,17 +14,22 @@ import 'dimensions_provider.dart';
 
 /// This class provides the real-time dimensions of a "full page" viewport.
 ///
-/// Note: all the measurements returned from this class are potentially
-/// *expensive*, and should be cached as needed. Every call to every method on
-/// this class WILL perform actual DOM measurements.
+/// All the measurements returned from this class are potentially *expensive*,
+/// and should be cached as needed. Every call to every method on this class
+/// WILL perform actual DOM measurements.
 class FullPageDimensionsProvider extends DimensionsProvider {
+  /// Constructs a global [FullPageDimensionsProvider].
+  ///
+  /// Doesn't need any parameters, because all the measurements come from the
+  /// globally available [DomVisualViewport].
   FullPageDimensionsProvider() {
     // Determine what 'resize' event we'll be listening to.
     // This is needed for older browsers (Firefox < 91, Safari < 13)
+    // TODO(dit): Clean this up, https://github.com/flutter/flutter/issues/117105
     final DomEventTarget resizeEventTarget =
         domWindow.visualViewport ?? domWindow;
 
-    // Subscribe to the 'resize' event, and convert it to a ui.Size stream...
+    // Subscribe to the 'resize' event, and convert it to a ui.Size stream.
     _domResizeSubscription = DomSubscription(
       resizeEventTarget,
       'resize',
@@ -37,7 +42,13 @@ class FullPageDimensionsProvider extends DimensionsProvider {
       StreamController<ui.Size?>.broadcast();
 
   void _onVisualViewportResize(DomEvent event) {
-    // This could return [computePhysicalSize]. Is it too costly to compute?
+    // `event` doesn't contain any size information (as opposed to the custom
+    // element resize observer). If it did, we could broadcast the physical
+    // dimensions here and never have to re-measure the app, until the next
+    // resize event triggers.
+    // Would it be too costly to broadcast the computed physical size from here,
+    // and then never re-measure the app?
+    // Related: https://github.com/flutter/flutter/issues/117036
     _onResizeStreamController.add(null);
   }
 

lib/web_ui/lib/src/engine/view_embedder/embedding_strategy/custom_element_embedding_strategy.dart

@@ -6,22 +6,25 @@ import 'package:ui/src/engine/dom.dart';
 
 import 'embedding_strategy.dart';
 
+/// An [EmbeddingStrategy] that renders flutter inside a target host element.
+///
+/// This strategy attempts to minimize DOM modifications outside of the host
+/// element, so it plays "nice" with other web frameworks.
 class CustomElementEmbeddingStrategy extends EmbeddingStrategy {
+  /// Creates a [CustomElementEmbeddingStrategy] to embed a Flutter view into [_hostElement].
   CustomElementEmbeddingStrategy(this._hostElement) {
-    // Clear children...
-    while (_hostElement.firstChild != null) {
-      _hostElement.removeChild(_hostElement.lastChild!);
-    }
+    _hostElement.clearChildren();
   }
 
+  /// The target element in which this strategy will embedd Flutter.
   final DomElement _hostElement;
 
   @override
   void initialize({
-    Map<String, String>? embedderMetadata,
+    Map<String, String>? hostElementAttributes,
   }) {
     // ignore:avoid_function_literals_in_foreach_calls
-    embedderMetadata?.entries.forEach((MapEntry<String, String> entry) {
+    hostElementAttributes?.entries.forEach((MapEntry<String, String> entry) {
       _setHostAttribute(entry.key, entry.value);
     });
     _setHostAttribute('flt-embedding', 'custom-element');

lib/web_ui/lib/src/engine/view_embedder/embedding_strategy/embedding_strategy.dart

@@ -10,21 +10,19 @@ import 'package:ui/src/engine/view_embedder/hot_restart_cache_handler.dart';
 import 'custom_element_embedding_strategy.dart';
 import 'full_page_embedding_strategy.dart';
 
-/// Provides the API that the FlutterViewEmbedder uses to interact with the DOM.
+/// Controls how a Flutter app is placed, sized and measured on the page.
 ///
-/// The base class handles "global" stuff that is shared across implementations,
-/// like handling hot-restart cleanup.
-///
-/// This class is specialized to handle different types of DOM embeddings:
+/// The base class handles general behavior (like hot-restart cleanup), and then
+/// each specialization enables different types of DOM embeddings:
 ///
 /// * [FullPageEmbeddingStrategy] - The default behavior, where flutter takes
-///   control of the whole web page. This is how Flutter Web used to operate.
+///   control of the whole page.
 /// * [CustomElementEmbeddingStrategy] - Flutter is rendered inside a custom host
 ///   element, provided by the web app programmer through the engine
 ///   initialization.
 abstract class EmbeddingStrategy {
   EmbeddingStrategy() {
-    // Prepare some global stuff...
+    // Initialize code to handle hot-restart (debug only).
     assert(() {
       _hotRestartCache = HotRestartCacheHandler();
       return true;
@@ -43,7 +41,7 @@ abstract class EmbeddingStrategy {
   HotRestartCacheHandler? _hotRestartCache;
 
   void initialize({
-    Map<String, String>? embedderMetadata,
+    Map<String, String>? hostElementAttributes,
   });
 
   /// Attaches the glassPane element into the hostElement.
@@ -52,14 +50,6 @@ abstract class EmbeddingStrategy {
   /// Attaches the resourceHost element into the hostElement.
   void attachResourcesHost(DomElement resourceHost, {DomElement? nextTo});
 
-  /// A callback that runs when hot restart is triggered.
-  ///
-  /// This should "clean" up anything handled by the [EmbeddingStrategy] instance.
-  @mustCallSuper
-  void onHotRestart() {
-    // Elements on the [_hotRestartCache] are cleaned up *after* hot-restart.
-  }
-
   /// Registers a [DomElement] to be cleaned up after hot restart.
   @mustCallSuper
   void registerElementForCleanup(DomElement element) {

lib/web_ui/lib/src/engine/view_embedder/embedding_strategy/full_page_embedding_strategy.dart

@@ -7,13 +7,17 @@ import 'package:ui/src/engine/util.dart' show assertionsEnabled, setElementStyle
 
 import 'embedding_strategy.dart';
 
+/// An [EmbeddingStrategy] that takes over the whole web page.
+///
+/// This strategy takes over the <body> element, modifies the viewport meta-tag,
+/// and ensures that the root Flutter view covers the whole screen.
 class FullPageEmbeddingStrategy extends EmbeddingStrategy {
   @override
   void initialize({
-    Map<String, String>? embedderMetadata,
+    Map<String, String>? hostElementAttributes,
   }) {
     // ignore:avoid_function_literals_in_foreach_calls
-    embedderMetadata?.entries.forEach((MapEntry<String, String> entry) {
+    hostElementAttributes?.entries.forEach((MapEntry<String, String> entry) {
       _setHostAttribute(entry.key, entry.value);
     });
     _setHostAttribute('flt-embedding', 'full-page');
@@ -61,8 +65,6 @@ class FullPageEmbeddingStrategy extends EmbeddingStrategy {
     setElementStyle(bodyElement, 'padding', '0');
     setElementStyle(bodyElement, 'margin', '0');
 
-    // TODO(yjbanov): fix this when KVM I/O support is added. Currently scroll
-    //                using drag, and text selection interferes.
     setElementStyle(bodyElement, 'user-select', 'none');
     setElementStyle(bodyElement, '-webkit-user-select', 'none');
 

lib/web_ui/lib/src/engine/view_embedder/hot_restart_cache_handler.dart

@@ -7,7 +7,13 @@ import 'package:meta/meta.dart';
 import '../dom.dart';
 import '../safe_browser_api.dart';
 
-/// Handles elements that need to be cleared after a hot-restart.
+/// Handles [DomElement]s that need to be removed after a hot-restart.
+///
+/// Elements are stored in an [_elements] list, backed by a global JS variable,
+/// named [defaultCacheName].
+///
+/// When the app hot-restarts (and a new instance of this class is created),
+/// everything in [_elements] is removed from the DOM.
 class HotRestartCacheHandler {
   HotRestartCacheHandler() {
     if (_elements.isNotEmpty) {
@@ -16,16 +22,13 @@ class HotRestartCacheHandler {
     }
   }
 
-  /// This is state persistent across hot restarts that indicates what
-  /// to clear.  Delay removal of old visible state to make the
-  /// transition appear smooth.
+  /// The name for the JS global variable backing this cache.
   @visibleForTesting
   static const String defaultCacheName = '__flutter_state';
 
   /// The js-interop layer backing [_elements].
   ///
-  /// They're stored in a js global with name [storeName], and removed from the
-  /// DOM when the app repaints...
+  /// Elements are stored in a JS global array named [defaultCacheName].
   late List<DomElement?>? _jsElements;
 
   /// The elements that need to be cleaned up after hot-restart.
@@ -39,13 +42,15 @@ class HotRestartCacheHandler {
     return _jsElements!;
   }
 
+  /// Removes every element from [_elements] and empties the list.
   void _clearAllElements() {
     for (final DomElement? element in _elements) {
       element?.remove();
     }
     _elements.clear();
   }
 
+  /// Registers a [DomElement] to be removed after hot-restart.
   void registerElement(DomElement element) {
     _elements.add(element);
   }

lib/web_ui/test/engine/view_embedder/embedding_strategy/custom_element_embedding_strategy_test.dart

@@ -30,14 +30,14 @@ void doTests() {
 
     test('Prepares target environment', () {
       strategy.initialize(
-        embedderMetadata: <String, String>{
+        hostElementAttributes: <String, String>{
           'key-for-testing': 'value-for-testing',
         },
       );
 
       expect(target.getAttribute('key-for-testing'), 'value-for-testing',
           reason:
-              'Should add embedderMetadata as key=value into target element.');
+              'Should add attributes as key=value into target element.');
       expect(target.getAttribute('flt-embedding'), 'custom-element',
           reason:
               'Should identify itself as a specific key=value into the target element.');

lib/web_ui/test/engine/view_embedder/embedding_strategy/full_page_embedding_strategy_test.dart

@@ -37,14 +37,14 @@ void doTests() {
       expect(userMeta, isNotNull);
 
       strategy.initialize(
-        embedderMetadata: <String, String>{
+        hostElementAttributes: <String, String>{
           'key-for-testing': 'value-for-testing',
         },
       );
 
       expect(target.getAttribute('key-for-testing'), 'value-for-testing',
           reason:
-              'Should add embedderMetadata as key=value into target element.');
+              'Should add attributes as key=value into target element.');
       expect(target.getAttribute('flt-embedding'), 'full-page',
           reason:
               'Should identify itself as a specific key=value into the target element.');