Skip to content

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Jun 4, 2024

Looks like I did not need this flag since flutter_portal pinned flutter version to 3.10 in their presubmits.

I'll create a g3 fix for this since there're only a handful of scuba failures. Also a g3 test revealed a problem with calling renderObject.markNeedsLayout directly. It has to be deferred so the render tree is clean during the idle phase and the postFrameCallback phase.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 4, 2024
@LongCatIsLooong LongCatIsLooong force-pushed the remove-layoutbuilder-migration-flag branch from 797667e to cd13f32 Compare June 4, 2024 06:46
@LongCatIsLooong LongCatIsLooong force-pushed the remove-layoutbuilder-migration-flag branch from cd13f32 to 70db8f1 Compare June 4, 2024 16:54
@LongCatIsLooong LongCatIsLooong changed the title Remove temporary LayoutBuilder migration flag Remove temporary LayoutBuilder migration flag, defer markNeedsLayout Jun 4, 2024
@LongCatIsLooong
Copy link
Contributor Author

scuba change CL: cl/640025627

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

void _frameCallback(Duration timestamp) {
_deferredCallbackScheduled = false;
// This method is only called when the render tree is stable, if the Element
// is deactivated it will never be reincorporated back to the true.
Copy link
Member

Choose a reason for hiding this comment

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

true -> tree?

@LongCatIsLooong
Copy link
Contributor Author

@goderbauer does it make sense to add an assert that makes sure nobody calls markNeedsLayout during idle / postFrameCallbacks? If markNeedsLayout was called during those two phases it's impossible (I think?) to recover before the next frame as the tree is finalized (and RenderBox.markNeedsLayout even destroys layout cache), and user actions such as hit-testing generally have asserts that ensure the render tree is clean: assert(!debugNeedsLayout).

I don't think we'll be able to just change the markNeedsLayout implementation since there are overrides (RenderBox), and adding the assert will probably mess up the case where we manually mark a render object dirty in tests. But it feels like a reasonable assert to add. Is there a way to somehow distinguish renderObject.markNeedsLayout() calls we manually write in tests?

@goderbauer
Copy link
Member

Hm, I am not sure how we would practically achieve that given the existing (test) code. Also, do you think the assert is still correct for somebody who uses the rendering layer directly?

@LongCatIsLooong
Copy link
Contributor Author

I don't know how people use the rendering layer directly, but I think

  • "user inputs are handled during the idle phase" is the contract SchedulerBinding establishes
  • our existing hitTest implementation does assume the tree is clean, and RenderBox.markNeedsLayout / RenderParagraph.markNeedsLayout destroys the current layout.

Given that mixin RendererBinding on BindingBase, ServicesBinding, SchedulerBinding, GestureBinding, SemanticsBinding, HitTestable it seems the best place to enforce that would be RenererBinding but doing that in the rendering library doesn't sound terribly wrong (as long as SchedulerBinding is being used).

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 5, 2024
@auto-submit auto-submit bot merged commit 8ce3bfb into flutter:master Jun 5, 2024
@LongCatIsLooong LongCatIsLooong deleted the remove-layoutbuilder-migration-flag branch June 5, 2024 20:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 6, 2024
flutter/flutter@27e0656...4608a89

2024-06-06 [email protected] Fix InputDecorator suffixIcon color when in error and hovered (flutter/flutter#149643)
2024-06-06 [email protected] Roll Flutter Engine from 32c3b9b7cbe1 to 92d0cd9370f7 (1 revision) (flutter/flutter#149789)
2024-06-06 [email protected] Roll Flutter Engine from 0edca2e9d3d2 to 32c3b9b7cbe1 (2 revisions) (flutter/flutter#149786)
2024-06-06 [email protected] Roll Flutter Engine from f51e0ad3abbe to 0edca2e9d3d2 (8 revisions) (flutter/flutter#149785)
2024-06-06 [email protected] Roll Flutter Engine from f37733035060 to f51e0ad3abbe (3 revisions) (flutter/flutter#149778)
2024-06-05 [email protected] Remove abi key from local golden file testing (flutter/flutter#149696)
2024-06-05 [email protected] Fixes Router transaction to respect operation order (flutter/flutter#149763)
2024-06-05 [email protected] Send q once (flutter/flutter#149767)
2024-06-05 [email protected] Marks Mac_ios rrect_blur_perf_ios__timeline_summary to be unflaky (flutter/flutter#149729)
2024-06-05 [email protected] Add `contrastLevel` parameter to `ColorScheme.fromSeed` (flutter/flutter#149705)
2024-06-05 [email protected] Roll Flutter Engine from 11a32d43e3f6 to f37733035060 (11 revisions) (flutter/flutter#149770)
2024-06-05 [email protected] Remove unused code from an older test artifact. (flutter/flutter#149746)
2024-06-05 [email protected] Create CarouselView widget - Part 1 (flutter/flutter#148094)
2024-06-05 [email protected] [flutter_tools] Remove additional listener on VM service that simply logged incoming messages (flutter/flutter#149756)
2024-06-05 [email protected] Fix signature for TokenTemplate.updateFile (flutter/flutter#149673)
2024-06-05 [email protected] Remove temporary LayoutBuilder migration flag, defer `markNeedsLayout` (flutter/flutter#149637)
2024-06-05 [email protected] Revert "make output of flutter run web tests verbose (#149694)" (flutter/flutter#149766)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
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 framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants