-
Notifications
You must be signed in to change notification settings - Fork 6k
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] Reland - Switch web-render option default to auto #23454
Conversation
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @yjbanov do you know if there might be any tests we forgot. I changed the following:
- web integration tests
- benchmarks
- flutter test (framework unit tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -619,6 +619,8 @@ class TestCommand extends Command<bool> with ArgUtils { | |||
'--enable-experiment=non-nullable', | |||
'--no-sound-null-safety', | |||
if (input.forCanvasKit) '-DFLUTTER_WEB_USE_SKIA=true', | |||
if (!input.forCanvasKit) '-DFLUTTER_WEB_AUTO_DETECT=false', | |||
if (!input.forCanvasKit) '-DFLUTTER_WEB_USE_SKIA=false', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just so it doesn't look like there's a possibility of having FLUTTER_WEB_USE_SKIA
twice we could do:
'-DFLUTTER_WEB_USE_SKIA=${input.forCanvasKit}',
Description
Reland #23090
Switches web-render to default to auto detect. On desktop flutter web will start using canvas kit by default, on mobile html will be used to render. Users can still override by -web-renderer=html or canvaskit.
See public doc https://docs.google.com/document/d/1aY0iU16wf_sdT7nwfpjgT-IatHNfF3slTiYHKmxcIog/edit
After this change if you encounter issues please see https://flutter.dev/docs/development/tools/web-renderers to switch to html (prior default) or canvaskit modes explicitely.
Related Issues
flutter/flutter#72377
Tests
Covered by existing engine/framework tests.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.Reviewer Checklist
Breaking Change
Did any tests fail when you ran them? Please read [handling breaking changes].