Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ditman
Copy link
Member

@ditman ditman commented Mar 8, 2024

This makes it possible to more flexibly compose {{flutter_js}} and {{flutter_build_options}} in @eyebrowsoffire's bootstrapping improvements PR.

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.

@ditman ditman requested a review from eyebrowsoffire March 8, 2024 20:46
@flutter-dashboard

This comment was marked as resolved.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Mar 8, 2024
@ditman
Copy link
Member Author

ditman commented Mar 8, 2024

There's several tests covering this gets initialized correctly; not sure if we have a way to test that this is initialized slightly differently though.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ditman
Copy link
Member Author

ditman commented Mar 9, 2024

This is part of our test bootstrap here:

Unfortunately, I cannot add any code that runs before the bootstrap; my idea was to

_flutter = {
  miscellaneous: 'value',
};

then run the rest of the test, but by the time the main method of my test is running, _flutter and _flutter.loader are already defined.

@ditman
Copy link
Member Author

ditman commented Mar 9, 2024

Another way is to have _flutter.miscellaneous = 'value' defined in the bootstrap before flutter.js, and see that nothing breaks, which I'm going to do right now.

@ditman
Copy link
Member Author

ditman commented Mar 9, 2024

By tweaking the test_platform.dart, actually all tests are testing this feature now. Removing missing test warning.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2024
@ditman
Copy link
Member Author

ditman commented Mar 9, 2024

It seems my test to the platform isn't liked by some web tests? Wat?

@ditman ditman removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2024
@ditman
Copy link
Member Author

ditman commented Mar 11, 2024

Reproduced locally, and found JS error in strict mode where my: if (_flutter == null) was throwing an Uncaught ReferenceError: _flutter is not defined error...

So replaced the initialization to the exact same thing we use in flutter.js: if (!window._flutter)

That should do the trick.

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 11, 2024
@auto-submit auto-submit bot merged commit 6771307 into flutter:main Mar 11, 2024
@ditman
Copy link
Member Author

ditman commented Mar 11, 2024

Created an issue while investigating this flutter/flutter#144969

@ditman ditman deleted the flutter-js-polite-init branch March 11, 2024 23:21
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 12, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 12, 2024
…ions) (#145002)

Manual roll requested by [email protected]

flutter/engine@6cefbe1...6771307

2024-03-11 [email protected] [web] Split initialization of the _flutter namespace and loader object. (flutter/engine#51294)
2024-03-11 [email protected] Roll Dart SDK from 09a1a1ede1c4 to 7a2c94fd0a26 (1 revision) (flutter/engine#51338)
2024-03-11 [email protected] Roll Skia from 6f10903e2d28 to 32f1cb50734a (5 revisions) (flutter/engine#51331)
2024-03-11 [email protected] Make scenario app have translucent splash screen so it doesn't affect screenshot colors (flutter/engine#51329)
2024-03-11 [email protected] [gn] Change sdk_rewriter() template to use depfiles (flutter/engine#51314)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants