Massage the JS interop around `didCreateEngineInitializer` #38147

eyebrowsoffire · 2022-12-09T00:43:32Z

Massage JS interop around didCreateEngineInitializer to work better with dart2wasm.

… work better with dart2wasm.

flutter-dashboard · 2022-12-09T00:43:35Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

ditman

Both _flutter and loader may be null and I think that this would crash in that case (because of the @JS('_flutter.loader') bit). The fix is slightly more verbose, but it's the same idea you have here!

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

lib/web_ui/lib/initialization.dart

ditman

LGTM!

Hixie · 2022-12-09T01:37:29Z

test-exempt: change to make things work at all for wasm

joshualitt · 2022-12-09T03:01:00Z

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

@@ -15,16 +16,27 @@ import 'js_promise.dart';
 /// programmer can control the initialization sequence.
 typedef DidCreateEngineInitializerFn = void Function(FlutterEngineInitializer);


Is this used for anything anymore?

joshualitt · 2022-12-09T03:08:21Z

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

-external DidCreateEngineInitializerFn? get didCreateEngineInitializer;
+extension FlutterLoaderExtension on FlutterLoader {
+  external void didCreateEngineInitializer(FlutterEngineInitializer initializer);
+  bool get isAutostart => !js_util.hasProperty(this, 'didCreateEngineInitializer');


autoStart? or rename the bool above to be autostart?

joshualitt · 2022-12-09T03:14:55Z

lib/web_ui/lib/initialization.dart

@@ -91,7 +91,7 @@ Future<void> webOnlyWarmupEngine({
  } else {
    // Yield control of the bootstrap procedure to the user.
    engine.domWindow.console.debug('Flutter Web Bootstrap: Programmatic.');
-    engine.didCreateEngineInitializer!(bootstrap.prepareEngineInitializer());
+    engine.flutter!.loader!.didCreateEngineInitializer(bootstrap.prepareEngineInitializer());


Can we refactor this to make the invariants a bit clearer and reduce the number of ! operators?

FlutterLoader? loader = engine.flutter?.loader; if (loader == null || loader.isAutostart()) { ... autostart ... } else { loader.didCreateEngineInitializer(...) }

auto-submit · 2022-12-09T19:28:15Z

auto label is removed for flutter/engine, pr: 38147, Failed to merge pr#: 38147 with Pull request could not be merged: Pull Request is not mergeable.

…lutter/engine#38147)

…116802) * bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef8585 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21d4 Remove autoninja. (flutter/engine#38136) * 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (#38127)" (flutter/engine#38137) * b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016) * aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

…8147) * Massage the JS interop around `didCreateEngineInitializer` to make it work better with dart2wasm. * Make the whole type hierarchy more explicit. * Address Joshua's comments.

…lutter#116802) * bd8bcf956 Roll Fuchsia Mac SDK from crEcyXdyZ686cAqMV... to pMV6A0ykZQ8aA3NG2... (flutter/engine#38120) * dec8b5221 Preliminary implementation of UIA for A11y on Windows (flutter/engine#37754) * 5545ccf87 Roll Fuchsia Linux SDK from NlJGkMbtZqQ6_BCpu... to xn8ztWtp-zww-jObz... (flutter/engine#38122) * 80a15a419 Create FlutterActivity/FlutterFragment using light weight engine with FlutterEngineGroup (flutter/engine#36963) * 5caef8585 Full implementation of text-input-test (flutter/engine#37986) * 8f6036e58 Reland fix wrong VSYNC event (flutter/engine#37865) * 4101c363c [iOS] Change locale format for spell check (flutter/engine#38080) * 2f5b67e4d [embedder] Ensure destruction called on present (flutter/engine#38078) * 0bddc6045 [Impeller Scene] Depth attachment; baked lighting example (flutter/engine#38118) * 6aa4ccd60 Remove dlCanvasRecorder from flutter::PictureRecorder (flutter/engine#38127) * dbb5284f2 [Windows] Add more cursor plugin tests (flutter/engine#38112) * 6e91204d9 Roll Fuchsia Mac SDK from pMV6A0ykZQ8aA3NG2... to 9SnrQ0vbR8IC7UIoP... (flutter/engine#38135) * 3140ad924 [Impeller] order metal samplers according to declared order and not usage order (flutter/engine#38115) * 84abf21d4 Remove autoninja. (flutter/engine#38136) * 8a113d328 [embedder] Expose metal surface from test context (flutter/engine#38133) * 1ef25b63f Roll Fuchsia Mac SDK from 9SnrQ0vbR8IC7UIoP... to aMW0DjntzFJj4RoR3... (flutter/engine#38139) * 748b3bc15 Revert "Remove dlCanvasRecorder from flutter::PictureRecorder (flutter#38127)" (flutter/engine#38137) * b6daf3d06 [embedder] Consistent naming for GL/Metal tests (flutter/engine#38141) * 339d04baf [web] Trivial fix for non-static interop JS interop class. (flutter/engine#38126) * 1fcbb9c11 [tools] Eliminate version on Obj-C docs (flutter/engine#38145) * 71928b6a4 [Impeller] Use DrawPath instead of Rect geometry when the paint style is stroke (flutter/engine#38146) * 23ce8fdbc Roll Skia from dd3285a80b23 to f84dc9303045 (4 revisions) (flutter/engine#38123) * 366f8663b Roll Skia from f84dc9303045 to 2691cd7b4110 (40 revisions) (flutter/engine#38151) * 447e7013e Roll Skia from 2691cd7b4110 to 711396b81248 (1 revision) (flutter/engine#38152) * cd5d91bf9 Pylint testing/run_tests.py (flutter/engine#38016) * aafac083b Roll Skia from 711396b81248 to b253b10374e7 (7 revisions) (flutter/engine#38157) * 799dc78e8 Roll Fuchsia Linux SDK from xn8ztWtp-zww-jObz... to rRJIjuO-dPNCpCTd9... (flutter/engine#38134) * 3aa3d2a8f Massage the JS interop around `didCreateEngineInitializer` (flutter/engine#38147) * 030950f30 Roll Skia from b253b10374e7 to ec407902999b (3 revisions) (flutter/engine#38158)

Massage the JS interop around didCreateEngineInitializer to make it…

e26575e

… work better with dart2wasm.

eyebrowsoffire requested review from ditman and joshualitt December 9, 2022 00:43

flutter-dashboard bot added the platform-web Code specifically for the web engine label Dec 9, 2022

flutter-dashboard bot added the needs tests label Dec 9, 2022

ditman approved these changes Dec 9, 2022

View reviewed changes

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

lib/web_ui/lib/initialization.dart Outdated Show resolved Hide resolved

Make the whole type hierarchy more explicit.

e3452a2

ditman approved these changes Dec 9, 2022

View reviewed changes

joshualitt reviewed Dec 9, 2022

View reviewed changes

joshualitt approved these changes Dec 9, 2022

View reviewed changes

eyebrowsoffire added 2 commits December 8, 2022 19:50

Address Joshua's comments.

229b71b

Merge branch 'main' into engine_initializer_interop

c878cae

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2022

eyebrowsoffire merged commit 3aa3d2a into flutter:main Dec 9, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 9, 2022

engine-flutter-autoroll mentioned this pull request Dec 9, 2022

Roll Flutter Engine from 8d83b98c55b3 to 030950f3070c (29 revisions) flutter/flutter#116802

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 9, 2022

3aa3d2a8f Massage the JS interop around didCreateEngineInitializer (f…

1a63d39

…lutter/engine#38147)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massage the JS interop around `didCreateEngineInitializer` #38147

Massage the JS interop around `didCreateEngineInitializer` #38147

eyebrowsoffire commented Dec 9, 2022

flutter-dashboard bot commented Dec 9, 2022

ditman left a comment

ditman left a comment

Hixie commented Dec 9, 2022

joshualitt Dec 9, 2022

joshualitt Dec 9, 2022

joshualitt Dec 9, 2022

auto-submit bot commented Dec 9, 2022

		@@ -15,16 +16,27 @@ import 'js_promise.dart';
		/// programmer can control the initialization sequence.
		typedef DidCreateEngineInitializerFn = void Function(FlutterEngineInitializer);

Massage the JS interop around didCreateEngineInitializer #38147

Massage the JS interop around didCreateEngineInitializer #38147

Conversation

eyebrowsoffire commented Dec 9, 2022

flutter-dashboard bot commented Dec 9, 2022

ditman left a comment

Choose a reason for hiding this comment

ditman left a comment

Choose a reason for hiding this comment

Hixie commented Dec 9, 2022

joshualitt Dec 9, 2022

Choose a reason for hiding this comment

joshualitt Dec 9, 2022

Choose a reason for hiding this comment

joshualitt Dec 9, 2022

Choose a reason for hiding this comment

auto-submit bot commented Dec 9, 2022

Massage the JS interop around `didCreateEngineInitializer` #38147

Massage the JS interop around `didCreateEngineInitializer` #38147