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

[web] Download CanvasKit chromium when possible #39827

Merged
auto-submit merged 10 commits into from
Feb 28, 2023
Merged

[web] Download CanvasKit chromium when possible #39827

auto-submit merged 10 commits into from
Feb 28, 2023

Conversation

@mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Feb 23, 2023

On Chromium-based browsers, try downloading the Chromium variant of CanvasKit. If not available, fallback to the normal variant.

This PR also introduce a new runtime config to control the filename of the canvaskit.js file. This allows users to force the Chromium of variant of CanvasKit by setting canvasKitJsFileName: "chromium/canvaskit.js" or force the non-Chromium one by doing canvasKitJsFileName: "canvaskit.js" (could be useful in tests as well).

This is still disabled so it should have no effect yet. Once all the pieces are ready, we need to set useClientICU to the correct value based on the browser we are running on.

Part of flutter/flutter#118801

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Feb 23, 2023
eyebrowsoffire
eyebrowsoffire suggested changes Feb 23, 2023
View reviewed changes
ditman
ditman approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Question about the "chromium" mode. Do we want it?

@ditman
Copy link
Member

ditman commented Feb 24, 2023

Also there's a failing test that is probably passing 'null' (string) as a config value, rather than null?

00:00 �[32m+0�[0m: �[1m�[90mloading test/ui/path_test.dart�[0m�[0m                                                                                                                                                               
[CHROME STDERR]:[0223/152338.970262:WARNING:sandbox_linux.cc(393)] InitializeSandbox() called with multiple threads in process gpu-process.
[CHROME STDERR]:
[CHROME STDERR]:DevTools listening on ws://127.0.0.1:12345/devtools/browser/76779f24-94a2-49d6-960f-74fb15daedae

00:00 �[32m+0�[0m: test/ui/path_test.dart: (setUpAll)�[0m                                                                                                                                                           
00:00 �[32m+0�[0m�[31m -1�[0m: test/ui/path_test.dart: (setUpAll) �[1m�[31m[E]�[0m�[0m                                                                                                                                                    
  Invalid argument (canvasKitVariant): Unknown CanvasKit variant: null
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/js_helper.dart 1135:37   Object.wrapException
  ../../../../lib/src/engine/configuration.dart 211:9                       FlutterConfiguration.canvasKitVariant
  ../../../../lib/src/engine/canvaskit/canvaskit_api.dart 2675:25           Object._canvasKitJsFileNames
  ../../../../lib/src/engine/canvaskit/canvaskit_api.dart 2688:10           Object._canvasKitJsUrls
  ../../../../lib/src/engine/canvaskit/canvaskit_api.dart 2698:24           <fn>
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 308:19  _wrapJsFunctionForAsync.closure.$protected
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 333:23  _wrapJsFunctionForAsync.<fn>
  org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart 238:3   Object._asyncStartSync
  ../../../../lib/src/engine/canvaskit/canvaskit_api.dart 2698:41           Object.downloadCanvasKit
  ../../../../lib/src/engine/canvaskit/renderer.dart 76:25                  <fn>

@mdebbar
Copy link
Contributor Author

mdebbar commented Feb 24, 2023

Also there's a failing test that is probably passing 'null' (string) as a config value, rather than null?

Nah. It's a real null. I added a print statement and this is what I'm seeing:

variant: null (runtimeType: Null)

For some reason case null: isn't catching it. I'll keep digging.

@mdebbar
Copy link
Contributor Author

mdebbar commented Feb 24, 2023

It turns out, the value is undefined not null. So Dart fails to match it with case null:.

https://dartpad.dev/66017295266de29cb7bf82d0136c7bb7

@mdebbar mdebbar requested a review from ditman February 24, 2023 16:02
ditman
ditman approved these changes Feb 24, 2023
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Let's go chromium mode! (small change required in the switch though!)

@ditman
Copy link
Member

ditman commented Feb 24, 2023

🚀

@mdebbar
Copy link
Contributor Author

mdebbar commented Feb 24, 2023

@eyebrowsoffire it looks like I need your approval too (to override your previous "Request Changes" review).

eyebrowsoffire
eyebrowsoffire approved these changes Feb 27, 2023
View reviewed changes
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.

Looks great!

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2023
@auto-submit auto-submit bot merged commit 584144d into flutter:main Feb 28, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 28, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 28, 2023
@engine-flutter-autoroll
584144dd4 [web] Download CanvasKit chromium when possible (flutter/en…
a31c848 
…gine#39827)
@ditman
Copy link
Member

ditman commented Mar 1, 2023

Let's goooo!

@mdebbar mdebbar deleted the download_ck_chromium branch June 22, 2023 21:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ditman ditman ditman approved these changes

@eyebrowsoffire eyebrowsoffire eyebrowsoffire approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mdebbar @ditman @eyebrowsoffire