Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1930183002:
Refactor scroll updates during flexbox layout. (Closed)
|
Created:
4 years, 7 months ago by szager1 Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src@rtl-scroll-origin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor scroll updates during flexbox layout.
When there are nested flexboxes, the current code does some O(N^2)
work to update scrolling information after flexing has finished. This
change streamlines the process for performing flex layout and updating
scrollbars into three distinct phases, controlled by the highest-level
flexbox in the layout tree:
1. Perform flex layout on descendants; any blocks with overflow:auto
scrollbars may add/remove scrollbars, but they will not run the normal
second-pass layout after changing scrollbars, and they will not clamp
their existing scroll positions.
2. If, during the first pass, any descendant added or removed
scrollbars, run a second flex layout pass, but don't allow any
descendants to add or remove scrollbars.
3. After the second pass, go through and clamp the scroll positions
on all scrolling descendants.
BUG=593209, 600036
Committed: https://crrev.com/aff9f64394945f24400021fdec18dbd70eee3e14
Cr-Commit-Position: refs/heads/master@{#397888}
Patch Set 1 #Patch Set 2 : Fix layout invalidation reason #
Total comments: 23
Patch Set 3 : rebase + nits #
Total comments: 5
Patch Set 4 : nit #Patch Set 5 : rebase #Patch Set 6 : Don't detach scrollbars that need reconstruction #
Total comments: 6
Patch Set 7 : nits #Messages
Total messages: 33 (13 generated)
Description was changed from ========== Refactor scroll updates during flex box layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209 ========== to ========== Refactor scroll updates during flex box layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209 ==========
szager@chromium.org changed reviewers: + cbiesinger@chromium.org, eae@chromium.org, leviw@chromium.org
https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:312: PaintLayerScrollableArea::DelayScrollPositionClampScope delayClampScope; Grumble grumble DeprecatedFlexBox :( We have tests that cover this? I'm not hugely concerned about it, but I wonder. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:323: void updateAfterLayout(); Can you add a TODO or comment about how this is ran *during* layout, and should probably be renamed? https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:324: void clampScrollPositionsAfterLayout(); Is there a reason we can't do this in the layer update pass, *actually* after layout? That would save us the global vector and such.
https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (left): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:637: cancelProgrammaticScrollAnimation(); BTW, why did you remove this? Just because it's not necessary because setScrollPosition does this anyway? https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:628: if (!overflowRect().size().isZero()) { if (!overflowRect().isEmpty()) ? But can you explain / add a comment why this check is necessary? https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1680: PreventRelayoutScope::increment(); Why not just increment()? Also, why do increment() and decrement() even exist? Why not just move that code to the constructor/destructor? I don't see any other callers, and FreezeScrollbarsScope doesn't have them... https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1685: PreventRelayoutScope::decrement(); same here https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1720: m_needsRelayout->clear(); This is for the case that the second pass may have added more objects to this vector? Seems worth adding a comment for that -- at first sight, it looks like ::decrement() should have cleared the vector already in this case. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1732: m_needsClamp = new PersistentHeapVector<Member<PaintLayerScrollableArea>>(); Oilpan confuses me, I trust that this is the right type to use... In PreventRelayoutScope, you lazily create the vector when first calling setNeedsLayout. Why create this one immediately? https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1733: ASSERT(m_count > 0 || m_needsClamp->isEmpty()); By the way, please name all static members as s_foo per the style guide: https://www.chromium.org/blink/coding-style#TOC-Right:18 https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1734: DelayScrollPositionClampScope::increment(); Again, why have these increment/decrement methods instead of doing the work directly in the ctor/dtor?
Description was changed from ========== Refactor scroll updates during flex box layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209 ========== to ========== Refactor scroll updates during flex box layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ==========
https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:312: PaintLayerScrollableArea::DelayScrollPositionClampScope delayClampScope; On 2016/04/28 22:50:04, leviw_travelin_and_unemployed wrote: > Grumble grumble DeprecatedFlexBox :( > > We have tests that cover this? I'm not hugely concerned about it, but I wonder. I have no idea, and I'm not sure I should give a care. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (left): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:637: cancelProgrammaticScrollAnimation(); On 2016/04/29 20:02:50, cbiesinger wrote: > BTW, why did you remove this? Just because it's not necessary because > setScrollPosition does this anyway? It already gets called further down the call stack, I'm just eliminating a redundant call. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:628: if (!overflowRect().size().isZero()) { On 2016/04/29 20:02:50, cbiesinger wrote: > if (!overflowRect().isEmpty()) ? > > But can you explain / add a comment why this check is necessary? This is a hack to prevent this from running before first layout (which will crash for some reason I can't remember). I searched for a more idiomatic way to figure out that this is first layout, but couldn't find anything. Added a comment. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1680: PreventRelayoutScope::increment(); On 2016/04/29 20:02:50, cbiesinger wrote: > Why not just increment()? > > Also, why do increment() and decrement() even exist? Why not just move that code > to the constructor/destructor? I don't see any other callers, and > FreezeScrollbarsScope doesn't have them... Yeah, I had other callers before, but they're gone. Got rid of increment/decrement. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1685: PreventRelayoutScope::decrement(); On 2016/04/29 20:02:50, cbiesinger wrote: > same here Done. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1720: m_needsRelayout->clear(); On 2016/04/29 20:02:50, cbiesinger wrote: > This is for the case that the second pass may have added more objects to this > vector? Seems worth adding a comment for that -- at first sight, it looks like > ::decrement() should have cleared the vector already in this case. Actually, no, this should ASSERT(!m_needsRelayout || m_needsRelayout->isEmpty()). Added that. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1732: m_needsClamp = new PersistentHeapVector<Member<PaintLayerScrollableArea>>(); On 2016/04/29 20:02:50, cbiesinger wrote: > Oilpan confuses me, I trust that this is the right type to use... > > In PreventRelayoutScope, you lazily create the vector when first calling > setNeedsLayout. Why create this one immediately? These are basically static vectors that never get deleted, but oilpan complains if I actually make them non-pointer static. The vector is going to get allocated eventually, it's not really important where it happens. This was just convenient. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1733: ASSERT(m_count > 0 || m_needsClamp->isEmpty()); On 2016/04/29 20:02:50, cbiesinger wrote: > By the way, please name all static members as s_foo per the style guide: > https://www.chromium.org/blink/coding-style#TOC-Right:18 Done. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1734: DelayScrollPositionClampScope::increment(); On 2016/04/29 20:02:50, cbiesinger wrote: > Again, why have these increment/decrement methods instead of doing the work > directly in the ctor/dtor? Got rid of these. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:323: void updateAfterLayout(); On 2016/04/28 22:50:04, leviw_travelin_and_unemployed wrote: > Can you add a TODO or comment about how this is ran *during* layout, and should > probably be renamed? Done. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:324: void clampScrollPositionsAfterLayout(); On 2016/04/28 22:50:04, leviw_travelin_and_unemployed wrote: > Is there a reason we can't do this in the layer update pass, *actually* after > layout? That would save us the global vector and such. It's not totally clear to me that this value isn't needed before the end of layout; for example, there are lots of callers to LayoutBox::scrolledContentOffset, and not just in paint code. Having said that: I agree that it should be possible to do most of the updaterAfterLayout stuff, including scroll offset clamping, after layout has finished, and I'm happy to add another TODO for that :)
> https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:324: void clampScrollPositionsAfterLayout(); > On 2016/04/28 22:50:04, leviw_travelin_and_unemployed wrote: > > Is there a reason we can't do this in the layer update pass, *actually* after > > layout? That would save us the global vector and such. > > It's not totally clear to me that this value isn't needed before the end of layout; for example, there are lots of callers to LayoutBox::scrolledContentOffset, and not just in paint code. > > Having said that: I agree that it should be possible to do most of the updaterAfterLayout stuff, including scroll offset clamping, after layout has finished, and I'm happy to add another TODO for that :) I think you should file a bug that scroll position shouldn't be queried when DocumentLifecycle < LayoutClean and link to that. Adding that ASSERT should make it easy to run down the callers. Just for sanity sake, you may want to add the assert and take a look at the first crash and make sure I'm not glossing over some potentially legitimate case... We certainly *used* to use these during Layout (think: paint invalidation), but I can't come up with why we still would.
Thinking about this a bit more, I think the change to LayoutBox::intrinsicScrollbarLogicalWidth should go in a separate CL; I will create that.
Thanks for making the changes. See below. https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp (right): https://codereview.chromium.org/1930183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutDeprecatedFlexibleBox.cpp:312: PaintLayerScrollableArea::DelayScrollPositionClampScope delayClampScope; On 2016/05/12 20:44:50, szager1 wrote: > On 2016/04/28 22:50:04, leviw_travelin_and_unemployed wrote: > > Grumble grumble DeprecatedFlexBox :( > > > > We have tests that cover this? I'm not hugely concerned about it, but I > wonder. > > I have no idea, and I'm not sure I should give a care. Yeah I wouldn't worry about this https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:796: if (isHorizontalWritingMode() && (style()->overflowY() == OverflowScroll || style()->overflowY() == OverflowAuto)) { Soo, I agree this will be better off in a different CL but I'm not sure I like this change. With the change, what's the difference between this function and scrollbarLogicalWidth()? More broadly, the question of whether preferred widths should take o:auto into account is tricky (what this change would do), in part because I think that adds indeterminism to sizing of layout objects. https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:593: if (!overflowRect().size().isZero()) { Thanks for adding the comment. I'd still change this to overflowRect().isEmpty(), and to an early return.
https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:796: if (isHorizontalWritingMode() && (style()->overflowY() == OverflowScroll || style()->overflowY() == OverflowAuto)) { On 2016/05/13 19:52:48, cbiesinger wrote: > Soo, I agree this will be better off in a different CL but I'm not sure I like > this change. With the change, what's the difference between this function and > scrollbarLogicalWidth()? > > More broadly, the question of whether preferred widths should take o:auto into > account is tricky (what this change would do), in part because I think that adds > indeterminism to sizing of layout objects. The thing that makes this work is the following change to PaintLayerScrollableArea::updateAfterLayout in this CL: if ((verticalScrollbarChanged && box().isHorizontalWritingMode()) || (horizontalScrollbarChanged && !box().isHorizontalWritingMode())) { box().setPreferredLogicalWidthsDirty(); box().updateLogicalWidth(); } As for whether it's a good idea, I would point out that it does improve the appearance of the tests that are updated in this patch, for example here is scrollbars/scrollbars-on-positioned-content.html before and after this patch: before: http://imgur.com/gTworqU after: http://imgur.com/R9OTH7y As you can see, in the 'before' rendering, the horizontal scrollbar is partially covered by the auto vertical scrollbar; in the 'after' rendering, the block is resized to avoid that. The improvement in flexbox-height-with-overflow-auto.html test is even more pronounced: before: http://imgur.com/uWIDjMT after: http://imgur.com/JwtYIEs So, yes, this is a public-facing change, but I would argue it's a good one. https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:593: if (!overflowRect().size().isZero()) { On 2016/05/13 19:52:48, cbiesinger wrote: > Thanks for adding the comment. I'd still change this to > overflowRect().isEmpty(), and to an early return. Done.
https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1930183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:796: if (isHorizontalWritingMode() && (style()->overflowY() == OverflowScroll || style()->overflowY() == OverflowAuto)) { On 2016/05/13 20:52:05, szager1 wrote: > On 2016/05/13 19:52:48, cbiesinger wrote: > > Soo, I agree this will be better off in a different CL but I'm not sure I like > > this change. With the change, what's the difference between this function and > > scrollbarLogicalWidth()? > > > > More broadly, the question of whether preferred widths should take o:auto into > > account is tricky (what this change would do), in part because I think that > adds > > indeterminism to sizing of layout objects. > > The thing that makes this work is the following change to > PaintLayerScrollableArea::updateAfterLayout in this CL: > > if ((verticalScrollbarChanged && box().isHorizontalWritingMode()) > || (horizontalScrollbarChanged && !box().isHorizontalWritingMode())) { > box().setPreferredLogicalWidthsDirty(); > box().updateLogicalWidth(); > } > > As for whether it's a good idea, I would point out that it does improve the > appearance of the tests that are updated in this patch, for example here is > scrollbars/scrollbars-on-positioned-content.html before and after this patch: > > before: http://imgur.com/gTworqU > after: http://imgur.com/R9OTH7y > > As you can see, in the 'before' rendering, the horizontal scrollbar is partially > covered by the auto vertical scrollbar; in the 'after' rendering, the block is > resized to avoid that. The improvement in > flexbox-height-with-overflow-auto.html test is even more pronounced: > > before: http://imgur.com/uWIDjMT > after: http://imgur.com/JwtYIEs > > So, yes, this is a public-facing change, but I would argue it's a good one. Ah, thanks, I missed that part. Still, this now makes intrinsicScrollbarWidth() identical to scrollbarWidth() right? I do think this needs more careful consideration. For example, for the non-flexbox shrinkwrapped case, I think your parent will be sized incorrectly because when we compute the parent's preferred width, we use the non-scrollbar child size, and when we update the child's preferred size, I don't think we recalculate the parent. Is this interoperable with other browsers? Anyway, we should discuss this on a separate CL, as you suggested.
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1930183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
PTAL The add-scrollbar-width-to-intrinsic-width change landed separately, so now this is just a refactor of flex layout.
szager@chromium.org changed reviewers: + skobes@chromium.org
+skobes because scrolling
Description was changed from ========== Refactor scroll updates during flex box layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ========== to ========== Refactor scroll updates during flexbox layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ==========
LGTM w/nits https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:605: box().flipForWritingMode(m_overflowRect); It's a bit weird that InlineBox::flipForWritingMode mutates the argument, most other flipForWritingMode methods take a const reference and returns a flipped copy. Obviously you didn't add this method but we should try to make this more consistent at some point. Would you mind adding a TODO to the definition in InlineBox.h? https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:756: DoublePoint newScrollPosition = clampScrollPosition(scrollPositionDouble()); clampedScrollPosition might be a better name? https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:190: FreezeScrollbarsScope() { s_count++; } Could we add a test or assert or something to guard against pathological growth as that would indicate a failure in the logic. Perhaps an ASSERT if it's more than the max DOM depth?
https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:605: box().flipForWritingMode(m_overflowRect); On 2016/06/02 22:57:25, eae wrote: > It's a bit weird that InlineBox::flipForWritingMode mutates the argument, most > other flipForWritingMode methods take a const reference and returns a flipped > copy. Obviously you didn't add this method but we should try to make this more > consistent at some point. Would you mind adding a TODO to the definition in > InlineBox.h? Done. https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:756: DoublePoint newScrollPosition = clampScrollPosition(scrollPositionDouble()); On 2016/06/02 22:57:25, eae wrote: > clampedScrollPosition might be a better name? Done. https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/1930183002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:190: FreezeScrollbarsScope() { s_count++; } On 2016/06/02 22:57:25, eae wrote: > Could we add a test or assert or something to guard against pathological growth > as that would indicate a failure in the logic. Perhaps an ASSERT if it's more > than the max DOM depth? I made these classes STACK_ALLOCATED, is that good enough?
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1930183002/#ps110001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1930183002/110001
Message was sent while issue was closed.
Description was changed from ========== Refactor scroll updates during flexbox layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ========== to ========== Refactor scroll updates during flexbox layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Refactor scroll updates during flexbox layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 ========== to ========== Refactor scroll updates during flexbox layout. When there are nested flexboxes, the current code does some O(N^2) work to update scrolling information after flexing has finished. This change streamlines the process for performing flex layout and updating scrollbars into three distinct phases, controlled by the highest-level flexbox in the layout tree: 1. Perform flex layout on descendants; any blocks with overflow:auto scrollbars may add/remove scrollbars, but they will not run the normal second-pass layout after changing scrollbars, and they will not clamp their existing scroll positions. 2. If, during the first pass, any descendant added or removed scrollbars, run a second flex layout pass, but don't allow any descendants to add or remove scrollbars. 3. After the second pass, go through and clamp the scroll positions on all scrolling descendants. BUG=593209,600036 Committed: https://crrev.com/aff9f64394945f24400021fdec18dbd70eee3e14 Cr-Commit-Position: refs/heads/master@{#397888} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/aff9f64394945f24400021fdec18dbd70eee3e14 Cr-Commit-Position: refs/heads/master@{#397888} |
