Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some of our hacks around JSPromise now that we have better APIs. #45591

Merged
merged 3 commits into from Sep 11, 2023

Conversation

eyebrowsoffire
Copy link
Contributor

Our JSPromise hackery is causing some issues with the new dart roll. We should just use the JSPromise and JSFunction support to simplify this. Note that I still have to do some hackery to construct JSPromise objects and to invoke JSFunction objects, and eventually we'll probably be able to simplify this even more once those APIs are baked into dart:js_interop

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Sep 8, 2023
lib/web_ui/lib/src/engine/app_bootstrap.dart Outdated Show resolved Hide resolved
});
}

/// Typedef for the function that initializes the flutter engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to you if you want to make this typed a bit better by having a non-external function that takes in the typedefs, does the toJS conversions, and then forwards them to a (private) external factory function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I guess maybe it's better to keep a little bit of type safety here.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 11, 2023

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

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 11, 2023
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@auto-submit auto-submit bot merged commit f192cc9 into flutter:main Sep 11, 2023
26 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2023
eyebrowsoffire added a commit that referenced this pull request Sep 11, 2023
auto-submit bot pushed a commit that referenced this pull request Sep 11, 2023
…tter APIs." (#45660)

Reverts #45591

This is somehow causing some issues with the hot reload tests and blocking engine -> framework rolls. See flutter/flutter#134455
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 11, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 11, 2023
…134462)

flutter/engine@0774ddc...06696e7

2023-09-11 jacksongardner@google.com Revert "Remove some of our hacks around JSPromise now that we have better APIs." (flutter/engine#45660)
2023-09-11 skia-flutter-autoroll@skia.org Roll Skia from 3ed290acb65f to e6225224fb4e (2 revisions) (flutter/engine#45658)
2023-09-11 kjlubick@users.noreply.github.com Update skwasm build to use safer flush call (flutter/engine#45652)
2023-09-11 jacksongardner@google.com Remove some of our hacks around JSPromise now that we have better APIs. (flutter/engine#45591)

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 rmistry@google.com,zra@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine
Projects
None yet
3 participants