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

[web] Better way to detect CanvasKit variant #40154

Merged
merged 5 commits into from Mar 15, 2023

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Mar 8, 2023

Partial revert of #40027

For more info, see: https://skia-review.googlesource.com/c/skia/+/653416

The above CL introduces a new CanvasKit API to tell us whether it needs client ICU data or not. This allows us to do a more robust detection of the CanvasKit variant we are using.

BEFORE MERGING:

  • Make a new CanvasKit release that contains the required changes, and roll to that version.
  • We are now checking for the existence of the method before invoking it.

Fixes flutter/flutter#121905

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Mar 8, 2023
@mdebbar mdebbar marked this pull request as draft March 8, 2023 22:25
@mdebbar mdebbar requested review from ditman and eyebrowsoffire and removed request for ditman March 9, 2023 21:06
@mdebbar mdebbar marked this pull request as ready for review March 9, 2023 21:07
@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 9, 2023

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

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
const bool browserSupportsCanvaskitChromium = false;
// TODO(mdebbar): Uncomment this to enable real detection of browser support.
// final bool browserSupportsCanvaskitChromium = domIntl.v8BreakIterator != null;
final bool browserSupportsCanvaskitChromium = domIntl.v8BreakIterator != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If canvaskit_chromium also removes the wasm image codecs, then this should also add && browserSupportsImageDecoder. Although in practice, I'm not aware of any Chromium-based browser not supporting image decoders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 9, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Mar 9, 2023

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

@mdebbar
Copy link
Contributor Author

mdebbar commented Mar 10, 2023

There's a timeout failure happening in wasm mode. This line is causing a hard wasm failure:

final Object actualMethod = js_util.getProperty(o, method);

because the js_util.getProperty(...) patch in dart2wasm has the wrong signature (it shoud be Function(Object, Object) instead of Function(Object, String)).

This is being fixed in https://dart-review.googlesource.com/c/sdk/+/288161. (Thanks @joshualitt!). Once that Dart CL lands and rolls into the engine, I'll rebase and try again.

Thanks @eyebrowsoffire for looking into this with me!

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 15, 2023
@auto-submit auto-submit bot merged commit e447f20 into flutter:main Mar 15, 2023
36 checks passed
gaaclarke added a commit that referenced this pull request Mar 15, 2023
gaaclarke added a commit that referenced this pull request Mar 15, 2023
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