[web] Add add/removeView JS methods. #48106

ditman · 2023-11-16T03:54:47Z

This PR adds:

JS-interop types to expose addView/removeView from the running FlutterApp object on JavaScript.
- Also, the options object that can be passed to addView.

Issues:

Fixes: [web:multi-view] Devise a public API for multi-view flutter#137377

Info

Most interesting files:

app_bootstrap.dart -> Adds the implementation for JS add/remove view.
js_app.dart -> Adds the js-interop layer for the FlutterApp object, and the configuration type. (Two options: hostElement and initialData).
flutter_view_manager.dart -> An abstraction over the viewData map that keeps related things together: viewData, js configuration options, register/unregister methods and a Stream of modification events.

The rest of the changes were ""required"" to have a small demo that does anything (currently it lets me "register" views from javascript). I didn't add much in there, because probably it's already being worked on by @mdebbar; just fiddled with the constructor of the EngineFlutterView to create views from JS Config, and added a wrapper around the viewData map (FlutterViewManager) to prevent direct access to the Map.

Usage

This is how I'm currently initializing my Flutter App, so I can "leak" the flutterApp to window and do things asynchronously after flutter loads:

<script>
  window.addEventListener('load', async function(ev) {
    _flutter.loader.loadEntrypoint({
      onEntrypointLoaded: function(engineInitializer) {
        engineInitializer.initializeEngine({
          multiViewEnabled: true,
        }).then(function(appRunner) {
          return appRunner.runApp();
        }).then(onAppRunning);
      }
    });
    // Leak flutterApp to window so we can do async things...
    function onAppRunning(flutterApp) {
      console.log('Running app', flutterApp);
      window.flutterApp = flutterApp;
    }
  });
</script>

This to test:

Go on your JS console and use flutterApp.addView({}) and see how that returns a Promise for the viewId it just added,
Now do flutterApp.removeView(0) (removes the implicitView), and see how everything starts crashing! (as expected)
- Get out of this weird state with a hot reload 🥳

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
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.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

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

yjbanov · 2023-11-16T18:10:40Z

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

+  /// Creates (and stores) a view from its [JsFlutterViewOptions].
+  ///
+  /// Returns a `viewId` that is automatically assigned by this method.
+  int createView({ required JsFlutterViewOptions options }) {


This API feels too much like an SQL database API, with IDs and ID look-up queries. Can this instead return EngineFlutterView? This would remove the need to deal with null checks due to nullability of operator[], and if the call site needs the ID, it can call view.viewId.

Agreed. I would also like for this class to be able to register listeners who want to receive events when views are registered or unregistered

This would remove the need to deal with null checks due to nullability of operator[], and if the call site needs the ID, it can call view.viewId.

Haha yes, this is totally modeled after what I needed in the call site, and not most logical (createView -> return a view)

register listeners who want to receive events when views are registered or unregistered

Good call, I was wondering how I could hook up the notification of a view being registered/unregistered. I think we can add an onViewsChanged broadcast stream or similar, so hooking this up to the framework is as easy as listening to that stream and propagating an event.

@harryterkelsen do you think there's value in having separate events for added/removed views, or is "changed" enough?

lib/web_ui/lib/src/engine/app_bootstrap.dart

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

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

harryterkelsen · 2023-11-16T18:38:19Z

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

+  /// Creates (and stores) a view from its [JsFlutterViewOptions].
+  ///
+  /// Returns a `viewId` that is automatically assigned by this method.
+  int createView({ required JsFlutterViewOptions options }) {


Agreed. I would also like for this class to be able to register listeners who want to receive events when views are registered or unregistered

lib/web_ui/lib/src/engine/js_interop/js_app.dart

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

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

lib/web_ui/lib/src/engine/js_interop/js_app.dart

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

ditman · 2023-11-18T01:59:20Z

Please everyone PTAL, this is a little less disruptive than what it was before. If you like the approach, I'll add a few tests and land this!

lib/web_ui/lib/src/engine/js_interop/js_app.dart

harryterkelsen

LGTM

…message.

ditman · 2023-11-21T04:02:39Z

Addressed comments, added some tests and rebased the branch. This is ready for autosubmit.

auto-submit · 2023-11-21T04:19:26Z

auto label is removed for flutter/engine/48106, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

…_test.

ditman

@mdebbar if you're OK with my latest changes, please apply the autosubmit label!

ditman · 2023-11-21T10:23:05Z

lib/web_ui/test/engine/platform_dispatcher/platform_dispatcher_test.dart

+      // Add this again when views don't register themselves upon instantiation.
+      // dispatcher
+      //   ..registerView(view20)
+      //   ..registerView(view21)
+      //   ..registerView(view22);


/cc @mdebbar I had to remove this temporarily, because of...

Ah yes, thank you! Please don't delete them though. I'll be uncommenting them when I move registration out of the EngineFlutterView constructor.

ditman · 2023-11-21T10:23:32Z

lib/web_ui/test/engine/view_embedder/flutter_view_manager_test.dart

+      test('fails if the same viewId is already registered', () {
+        final int viewId = nextViewId++;
+        final EngineFlutterView view = EngineFlutterView(viewId, platformDispatcher, createDomElement('div'));
+
+        viewManager.registerView(view);
+
+        expect(() { viewManager.registerView(view); }, throwsAssertionError);
+      });


(The assertion thrown when registering duplicated views was breaking the platform_dispatcher_test.dart that does both create views, and then registers them)

I actually had to allow overwriting of the ImplicitView for some tests to continue working as expected, like test/engine/routing_test.dart (see latest commit)!

Let's land this as-is for now. But I'm probably going to change it. Tests that need to override the implicit view can simply unregister the old one before registering the new one.

…a bug.

mdebbar · 2023-11-21T15:30:18Z

@mdebbar if you're OK with my latest changes, please apply the autosubmit label!

Thanks for giving me permission to unblock myself 😃

…sions) (#138817) Manual roll requested by [email protected] flutter/engine@70b1c73...6da31e1 2023-11-21 [email protected] Revert Dart SDK to 3.3.0-152.0.dev (flutter/engine#48272) 2023-11-21 [email protected] [web] Add add/removeView JS methods. (flutter/engine#48106) 2023-11-21 [email protected] Roll Skia from c244996e79d6 to c6d971f87d12 (1 revision) (flutter/engine#48268) 2023-11-21 [email protected] Roll Skia from 516357c42907 to c244996e79d6 (1 revision) (flutter/engine#48266) 2023-11-21 [email protected] Roll Dart SDK from 99d9cf9bd05a to 5e7b22caba8a (1 revision) (flutter/engine#48264) 2023-11-21 [email protected] Roll Dart SDK from 64cbfa6c51e6 to 99d9cf9bd05a (1 revision) (flutter/engine#48261) 2023-11-21 [email protected] Roll Skia from 88a9e6328f93 to 516357c42907 (1 revision) (flutter/engine#48260) 2023-11-21 [email protected] Roll Skia from a26e5a5771c8 to 88a9e6328f93 (1 revision) (flutter/engine#48259) 2023-11-21 [email protected] Roll Skia from f8daeeb7f092 to a26e5a5771c8 (2 revisions) (flutter/engine#48258) 2023-11-21 [email protected] Roll Dart SDK from f1fd14505782 to 64cbfa6c51e6 (1 revision) (flutter/engine#48257) 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

ditman requested review from harryterkelsen and mdebbar November 16, 2023 03:54

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

yjbanov reviewed Nov 16, 2023

View reviewed changes

harryterkelsen reviewed Nov 16, 2023

View reviewed changes

mdebbar reviewed Nov 16, 2023

View reviewed changes

ditman commented Nov 18, 2023

View reviewed changes

lib/web_ui/lib/src/engine/window.dart Outdated Show resolved Hide resolved

ditman requested review from harryterkelsen, mdebbar and yjbanov November 18, 2023 01:58

mdebbar approved these changes Nov 20, 2023

View reviewed changes

lib/web_ui/lib/src/engine/js_interop/js_app.dart Outdated Show resolved Hide resolved

harryterkelsen approved these changes Nov 20, 2023

View reviewed changes

ditman added 4 commits November 20, 2023 16:45

[web] PoC to add/removeView from JS.

730f08d

Allow futureToPromise to return a promise that may resolve as null.

df7b34b

Address PR comments.

b5c8b00

Revert engine/window.dart

ccc544d

ditman force-pushed the multiview-add-js-api branch from 0b81e0b to 7e5d018 Compare November 21, 2023 00:46

ditman added 3 commits November 20, 2023 17:32

Improve hostElement getter to perform a null check with useful error …

eee08f7

…message.

Add basic test for js_app.dart

d2e5ac1

Update licenses

1beb8d5

ditman force-pushed the multiview-add-js-api branch from 7e5d018 to 1beb8d5 Compare November 21, 2023 01:34

Add flutter_view_manager_test.dart

408101b

ditman changed the title ~~[web] PoC to add/removeView from JS.~~ [web] Add add/removeView JS methods. Nov 21, 2023

ditman marked this pull request as ready for review November 21, 2023 04:01

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

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

Temporarily prevent double view registration in a platform_dispatcher…

4fde0ac

…_test.

ditman commented Nov 21, 2023

View reviewed changes

Allow over-writing the implicit view so more tests can pass. This is …

49c9662

…a bug.

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

auto-submit bot merged commit 7cdfc51 into flutter:main Nov 21, 2023

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

Manual roll Flutter Engine from 70b1c7341255 to 6da31e1bff67 (10 revisions) flutter/flutter#138817

Merged

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

7cdfc512f [web] Add add/removeView JS methods. (flutter/engine#48106)

ea01947

ditman deleted the multiview-add-js-api branch November 21, 2023 19:40

[web] Add add/removeView JS methods. #48106

[web] Add add/removeView JS methods. #48106

Uh oh!

Conversation

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

Uh oh!

Issues:

Info

Usage

Pre-launch Checklist

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ditman commented Nov 18, 2023

Uh oh!

Uh oh!

harryterkelsen left a comment

Choose a reason for hiding this comment

Uh oh!

ditman commented Nov 21, 2023

Uh oh!

auto-submit bot commented Nov 21, 2023

Uh oh!

ditman left a comment

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!

ditman Nov 21, 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!

mdebbar commented Nov 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ditman commented Nov 16, 2023 •

edited

Loading

ditman Nov 21, 2023 •

edited

Loading