[web] Explicit initialization of the implicit view #47921

mdebbar · 2023-11-10T19:10:29Z

An explicit API for initializing the implicit view (aka window).
The explicit API is being called during the engine initialization for now, but we could simply remove that or make it conditional.
Remove direct usages of window in tests:
- Most of the usages were being delegated to PlatformDispatcher anyway (e.g. sendPlatformMessage).
- This makes it clearer which tests depend on the implicit view (there are still hidden/indirect dependencies though).

ditman · 2023-11-10T19:53:05Z

This makes it clearer which tests depend on the implicit view (there are still hidden/indirect dependencies though).

Yes! This is great. There are a couple of tests still failing, but they all seem related to missing a call to ensureImplicitViewInitialized() somewhere in their setup.

mdebbar · 2023-11-10T21:21:02Z

There are a couple of tests still failing, but they all seem related to missing a call to ensureImplicitViewInitialized() somewhere in their setup.

Exactly. These are examples of tests that have a hidden/indirect dependency on the implicit view (2 of them were because the flutter view embedder requires the existence of the implicit view).

yjbanov · 2023-11-10T22:47:02Z

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

-    _flutterViewEmbedder ??=
-        FlutterViewEmbedder(hostElement: configuration.hostElement);
+FlutterViewEmbedder ensureFlutterViewEmbedderInitialized() {
+  // FlutterViewEmbedder needs the implicit view to be initialized.


Updated comment.

yjbanov · 2023-11-10T22:48:25Z

lib/web_ui/lib/src/engine/initialization.dart

  _initializationState = DebugEngineInitializationState.initializingUi;

  RawKeyboard.initialize(onMacOs: operatingSystem == OperatingSystem.macOs);
+  ensureImplicitViewInitialized();


Will initializeEngineUi henceforth be used to initialize in the single-view mode?

I haven't thought that far yet, but the way I see it is initializeEngineUi will be used for all modes. And instead of calling ensureImplicitViewInitialized unconditionally (like I'm doing here), it will be guarded by whatever conditions that dictate the need for the implicit view.

Cool. When/if this changes, we should also update the dartdocs above.

Also, maybe we should delay the call to initializeEngineUi until the first view is added? cc @ditman

Most likely, initializeEngineUi will do nothing, since all it does now is ensureFlutterViewEmbedderInitialized(), attach a global keyboard handler (and change the state of the lifecycle of the engine initialization)! code.

Everything it does seems that will eventually be gobbled up by createView(viewOptions), right?

We can leave the method here for the "implicitView" modes, (that's where we can "createView" for the implicitView), but in multiview mode this can just let the state machine of the engine advance, and noop the rest.

Most likely, initializeEngineUi will do nothing, since all it does now is ensureFlutterViewEmbedderInitialized(), attach a global keyboard handler

Well, there are many things hidden behind that ensureFlutterViewEmbedderInitialized(). E.g. keyboard, pointers, resize, a11y, etc.

I think we can use initializeEngineUi for initializing the global stuff (e.g. a11y announcements).

yjbanov · 2023-11-10T22:48:55Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+  ///
+  /// Nothing happens if the view is not already registered.
+  void unregisterView(EngineFlutterView view) {
+    viewData.remove(view.viewId);


Do we need to call dispose on the view?

There's no dispose on view (yet). But if there was a FlutterView.dispose method, then here's how it would look like:

class EngineFlutterView implements ui.FlutterView { EngineFlutterView(...) { // This already exists. platformDispatcher.registerView(this); } void dispose() { // This doesn't exists yet, but this is what `dispose` would look like if we ever add one (I think we will). platformDispatcher.unregisterView(this); } }

I guess what I'm saying is: if we ever have a FlutterView.dispose, then FlutterView.dispose should call PlatformDispatcher.unregisterView and not vice versa.

Ha-ha. Gotcha.

yjbanov · 2023-11-10T22:51:03Z

lib/web_ui/lib/src/engine/window.dart

-    EngineFlutterWindow(kImplicitViewId, EnginePlatformDispatcher.instance);
+EngineFlutterWindow get window {
+  if (_window == null) {
+    throw StateError('Trying to access the implicit flutter view, but it has not been initialized.');


Is there a scenario where in multi-view mode we also have an implicit view? If not, maybe the error message should talk along the lines of "Implicit Flutter view is not available. Flutter is running in multi-view mode."

nit: s/flutter/Flutter/ (it's a proper name)

Is there a scenario where in multi-view mode we also have an implicit view?

I'm trying to avoid this, and only use the implicit view in the modes that exist right now (single view)

Updated error message.

yjbanov · 2023-11-13T21:16:50Z

lib/web_ui/lib/src/engine/initialization.dart

  _initializationState = DebugEngineInitializationState.initializingUi;

  RawKeyboard.initialize(onMacOs: operatingSystem == OperatingSystem.macOs);
+  ensureImplicitViewInitialized();


Cool. When/if this changes, we should also update the dartdocs above.

yjbanov · 2023-11-13T21:17:13Z

lib/web_ui/lib/src/engine/platform_dispatcher.dart

+  ///
+  /// Nothing happens if the view is not already registered.
+  void unregisterView(EngineFlutterView view) {
+    viewData.remove(view.viewId);


Ha-ha. Gotcha.

yjbanov · 2023-11-13T21:17:58Z

lib/web_ui/lib/src/engine/window.dart

+    throw StateError(
+      'Trying to access the implicit FlutterView, but it is not available.\n'
+      'Note: the implicit FlutterView is not available in multi-view mode.',
+    );


Since the expression below needs a ! anyway, can we make it an assert so the error message can be tree shaken off?

auto-submit · 2023-11-13T22:43:56Z

auto label is removed for flutter/engine/47921, due to Pull request flutter/engine/47921 is not in a mergeable state.

…/engine#47921)

…138420) flutter/engine@1b3fd80...777dcb9 2023-11-14 [email protected] [web] Explicit initialization of the implicit view (flutter/engine#47921) 2023-11-14 [email protected] Roll Skia from b7d581997f2b to a811132be321 (1 revision) (flutter/engine#48019) 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 [email protected],[email protected],[email protected] 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

[web] Explicit initialization of the implicit view

4c5cd28

mdebbar requested review from ditman and yjbanov November 10, 2023 19:10

github-actions bot added the platform-web Code specifically for the web engine label Nov 10, 2023

fix tests

65de3b3

yjbanov reviewed Nov 10, 2023

View reviewed changes

mdebbar added 3 commits November 13, 2023 09:33

Merge branch 'main' into init_implicit_view

7d6b4f6

update comment and error message

dd5a74c

fix more tests

011cd9b

mdebbar force-pushed the init_implicit_view branch from fc8a6cb to 011cd9b Compare November 13, 2023 16:31

mdebbar requested a review from yjbanov November 13, 2023 18:46

mdebbar added 2 commits November 13, 2023 14:37

better fix for tests

30217be

Merge branch 'main' into init_implicit_view

2a2d724

yjbanov approved these changes Nov 13, 2023

View reviewed changes

assert

9ae8bd3

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 13, 2023

Merge branch 'main' into init_implicit_view

c8b7690

mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 14, 2023

auto-submit bot merged commit 777dcb9 into flutter:main Nov 14, 2023

mdebbar deleted the init_implicit_view branch November 14, 2023 16:15

engine-flutter-autoroll mentioned this pull request Nov 14, 2023

Roll Flutter Engine from 1b3fd80812c3 to 777dcb99f6e0 (2 revisions) flutter/flutter#138420

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 14, 2023

777dcb99f [web] Explicit initialization of the implicit view (flutter…

f71b8d7

…/engine#47921)

[web] Explicit initialization of the implicit view #47921

[web] Explicit initialization of the implicit view #47921

Uh oh!

Conversation

mdebbar commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ditman commented Nov 10, 2023

Uh oh!

mdebbar commented Nov 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdebbar Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdebbar commented Nov 10, 2023 •

edited

Loading

mdebbar Nov 13, 2023 •

edited

Loading