-
Notifications
You must be signed in to change notification settings - Fork 6k
[macOS] Do not block main thread during resizing if there is no content #35409
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 (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
cc @cbracken |
|
I'm not sure that this PR fixes flutter/flutter#92673 because flutter/flutter#92673 has two problems.
At the time of writing, this PR seems to solve one. I do think that we want this change, but I'm not sure if we can keep
in the PR description without solving both issues mentioned in flutter/flutter#92673. |
|
We'll need to author a test for this. I'll give it a shot. |
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.
@a-wallen is writing a test for this. The fix itself lgtm; we can land this once his test has been added.
The red box to immediately be replaced with nothing (a grey background)
Covered by this separate bug: flutter/flutter#76082
|
Writing a unit test for this PR in the engine would be easiest via embedding this flutter application import 'package:flutter/widgets.dart';
void main() async {
runApp(const Container(color: Color(0xffff0000)));
runApp(const Container());
}in a custom entry point for the embedder to consume. However, the engine cannot depend on the framework since the framework depends on the engine. There are two options to test this PR:
|
|
The PR above should address the tests for this PR. |
|
test-exempt: test is separate |
|
@knopp May I land this please? Everything looks good to go. |
|
FYI @chinmaygarde, the test for this isn't done. |
…nt (flutter#35409) * [macOS] Do not block main thread during resizing if there is no content Fixes: flutter/flutter#92673
Fixes: flutter/flutter#92673
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.