fix widget built twice during warm up frame by chunhtai · Pull Request #39079 · flutter/flutter

chunhtai · 2019-08-22T20:05:18Z

Description

The runapp work as follow
1, build rendering tree
2, schedule warm up frame
...(async gap)
3, run async draw frame

There is a chance engine will flush user setting in the middle of 2 and 3 which will dirty rendering tree. and draw frame will have to rebuild the rendering tree

With this pr the runapp work as follow
1, schedule build rendering tree
2, schedule warm up frame
...(async gap)
3, run async build rendering tree
4, run async draw frame

In this case, it does not matter whether something try to dirty render tree in the middle of 2 and 3 as there is no rendering tree to dirty.

This will however affect flutter test since it override schedulewarmup frame to be synchronized function to make sure runapp run synchronously. To addjust that, I implements a private runapp that flush timer after the regular runapp and removed the schedulewarmup override.

I do not use the callback solution we talked about @Hixie @goderbauer since that will create code duplication in flutter test binding and widgetflutterbinding. However, this approach might be a bigger breaking change for people who work directly with binding.

Related Issues

#31195

Tests

I added the following tests:

'WidgetBinding build rendering tree and warm up frame back to back'

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.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

chunhtai · 2019-08-22T20:06:51Z

packages/flutter/lib/src/widgets/binding.dart

I might as well create an api to just schedule attachRootWidget and flutter test can override this api. This might make it more clean, but I am not sure if it worth to expose an api for that

chunhtai · 2019-08-22T20:08:43Z

packages/flutter/test/widgets/nested_scroll_view_async_test.dart

since it is no longer possible to dirty thing in the middle of warm up frame, I have to change the way to reproduce double builds. if we are ok to remove the null check in nestedscrollview, we can remove this test as well

chunhtai · 2019-08-22T20:10:00Z

packages/flutter/lib/src/widgets/nested_scroll_view.dart

I intend to keep this check as there might be other corner case that hit this, but i cannot think of any now.

Turn these into asserts then?

Hixie · 2019-08-22T22:24:18Z

packages/flutter/test/widgets/binding_test.dart

testerbinding is no longer overriding scheduleWarmUpFrame, there is no point to test it?

reminding myself, we can add this back since I decide to keep the override

Hixie · 2019-08-22T22:25:17Z

packages/flutter/test/widgets/run_app_async_test.dart

nit: extraneous blank line

packages/flutter_test/lib/src/binding.dart

Hixie · 2019-08-22T22:26:50Z

packages/flutter_test/lib/src/binding.dart

this shouldn't be private, and it should be documented (and @Protected)

Hixie · 2019-08-22T22:27:47Z

This will definitely have to go through the breaking change announcement process. Might be worth trying to run it through the internal Google test suite and see how many things we break there, too, to get an idea of how much breakage this will cause.

goderbauer

Great debugging to figure this out!

Can you throw the "breaking change" label on here and sent out an announcement?

goderbauer · 2019-08-22T22:32:39Z

packages/flutter/lib/src/widgets/nested_scroll_view.dart

Turn these into asserts then?

goderbauer · 2019-08-22T22:56:56Z

Looks like there are some tests on cirrus that were supposed to fail, but they are now passing.

goderbauer

This looks good, but we should announce it as a breaking change and as Hixie pointed out, it may be good to run this through Google's internal test suite to see what breaks there.

goderbauer · 2019-08-26T17:39:59Z

packages/flutter/lib/src/widgets/binding.dart

... attaches the root widget...

goderbauer · 2019-08-28T21:43:12Z

@kpsroka FYI

chunhtai · 2019-08-29T22:05:24Z

internal test suite passed, breaking change email sent.

goderbauer

LGTM

Probably good to wait until next week with submission to give people some time to respond to the breaking change.

goderbauer · 2019-08-30T19:58:49Z

packages/flutter/lib/src/widgets/binding.dart

nit: I would probably remove the see also here since the sentence above just said the same thing.

tvolkert · 2019-12-05T20:31:16Z

Was there a breaking change announcement for this change? If so, @chunhtai will you please link it here?

chunhtai · 2019-12-05T20:51:19Z

Was there a breaking change announcement for this change? If so, @chunhtai will you please link it here?

this is the one https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!searchin/flutter-announce/chun%7Csort:date/flutter-announce/m6ewfoO64mI/ziD4VmwUBQAJ

tvolkert · 2019-12-05T20:55:40Z

Thanks!

chunhtai requested review from Hixie and goderbauer August 22, 2019 20:05

googlebot added the cla: yes label Aug 22, 2019

chunhtai commented Aug 22, 2019

View reviewed changes

dnfield added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Aug 22, 2019

Hixie reviewed Aug 22, 2019

View reviewed changes

packages/flutter/test/widgets/run_app_async_test.dart Outdated

Copy link

Contributor

Hixie Aug 22, 2019

Choose a reason for hiding this comment

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

nit: extraneous blank line

Hixie reviewed Aug 22, 2019

View reviewed changes

packages/flutter_test/lib/src/binding.dart Outdated Show resolved Hide resolved

Hixie reviewed Aug 22, 2019

View reviewed changes

packages/flutter_test/lib/src/binding.dart Outdated

Copy link

Contributor

Hixie Aug 22, 2019

Choose a reason for hiding this comment

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

this shouldn't be private, and it should be documented (and @Protected)

goderbauer reviewed Aug 22, 2019

View reviewed changes

packages/flutter/lib/src/widgets/nested_scroll_view.dart Outdated

Copy link

Member

goderbauer Aug 22, 2019

Choose a reason for hiding this comment

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

Turn these into asserts then?

goderbauer reviewed Aug 26, 2019

View reviewed changes

packages/flutter/lib/src/widgets/binding.dart Outdated

Copy link

Member

goderbauer Aug 26, 2019

Choose a reason for hiding this comment

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

... attaches the root widget...

goderbauer approved these changes Aug 30, 2019

View reviewed changes

chunhtai force-pushed the issues/31195-fix-warmup branch from 2f77a48 to 53a3a7f Compare August 30, 2019 20:21

Piinks added the c: API break Backwards-incompatible API changes label Sep 3, 2019

goderbauer added the waiting for tree to go green label Sep 4, 2019

fix widget built twice during warm up frame

a8aebfc

chunhtai force-pushed the issues/31195-fix-warmup branch from 53a3a7f to a8aebfc Compare September 5, 2019 16:35

chunhtai merged commit 57d714e into flutter:master Sep 6, 2019

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Conversation

chunhtai commented Aug 22, 2019 • edited by digiter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Aug 22, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Aug 22, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Aug 28, 2019

Uh oh!

chunhtai commented Aug 29, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Dec 5, 2019

Uh oh!

chunhtai commented Dec 5, 2019

Uh oh!

tvolkert commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chunhtai commented Aug 22, 2019 •

edited by digiter

Loading