Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263213002:
[css-grid] Do not recursively call layout during auto repeat computation (Closed)
|
Created:
4 years, 4 months ago by svillar Modified:
4 years, 3 months ago CC:
chromium-reviews, jfernandez, szager+layoutwatch_chromium.org, zoltan1, Manuel Rego, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, svillar, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Do not recursively call layout during auto repeat computation
The computation of auto repeat tracks was incorrectly recursively triggering
a layout in order to compute the available size. That was happening whenever
the width was indefinite. In such cases we should treat the width always as
indefinite without having to run any extra code. During the layout phase
we'll have the actual width available.
This partially reverts commit 49bc5ba13c492f09971882e2cb842e3514d19922 which
was a quick fix for a security issue but that was not actually fixing the
real problem behind.
BUG=633474
Committed: https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1
Cr-Commit-Position: refs/heads/master@{#414446}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Patch for landing #
Messages
Total messages: 21 (7 generated)
svillar@igalia.com changed reviewers: + cbiesinger@chromium.org, rego@igalia.com
Adding reviewers who checked the original patch
jfernandez@igalia.com changed reviewers: + jfernandez@igalia.com
https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401: ? computeLogicalWidthUsing(MaxSize, maxLength, containingBlockLogicalWidthForContent(), containingBlock()) Where is this case defined in the new logic ? Is it something we don't need to do now ? why ¿ https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1400: DCHECK_NE(sizingOperation, IntrinsicSizeComputation); Wouldn't be clearer if we define the DCHECK like isRowAxis) && sizingOperation != IntrinsicSizeComputation outside the if ?
Thanks for the quick review! https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401: ? computeLogicalWidthUsing(MaxSize, maxLength, containingBlockLogicalWidthForContent(), containingBlock()) On 2016/08/22 14:44:48, jfernandez wrote: > Where is this case defined in the new logic ? Is it something we don't need to > do now ? why ¿ You can see it defined for rows in the else block of "if(isRowAxis)". For columns we don't need it as I mentioned in the new comment I'm adding, widths are never indefinite during layout, they can only be indefinite when computing intrinsic sizes. https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1400: DCHECK_NE(sizingOperation, IntrinsicSizeComputation); On 2016/08/22 14:44:48, jfernandez wrote: > Wouldn't be clearer if we define the DCHECK like isRowAxis) && sizingOperation > != IntrinsicSizeComputation outside the if ? I really don't get what you mean. Could you rephrase? That DCHECK basically checks that if we have entered the outer if and we're handling columns it could only be the case that we're in the layout phase and never in intrinsic size computation.
https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1401: ? computeLogicalWidthUsing(MaxSize, maxLength, containingBlockLogicalWidthForContent(), containingBlock()) On 2016/08/22 16:01:52, svillar wrote: > On 2016/08/22 14:44:48, jfernandez wrote: > > Where is this case defined in the new logic ? Is it something we don't need to > > do now ? why ¿ > > You can see it defined for rows in the else block of "if(isRowAxis)". For > columns we don't need it as I mentioned in the new comment I'm adding, widths > are never indefinite during layout, they can only be indefinite when computing > intrinsic sizes. got it now, thanks. https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1400: DCHECK_NE(sizingOperation, IntrinsicSizeComputation); On 2016/08/22 16:01:52, svillar wrote: > On 2016/08/22 14:44:48, jfernandez wrote: > > Wouldn't be clearer if we define the DCHECK like isRowAxis) && sizingOperation > > != IntrinsicSizeComputation outside the if ? > > I really don't get what you mean. Could you rephrase? > > That DCHECK basically checks that if we have entered the outer if and we're > handling columns it could only be the case that we're in the layout phase and > never in intrinsic size computation. I was mistaken, forget about this suggestion.
lgtm as long as Javier is happy :)
lgtm
https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1395: LayoutUnit availableSize(-1); Mmm, this means that for columns during intrinsic size computation we always consider an indefinite width. Even when the width might have a fixed size. I guess this is fine, as later during the track sizing layout we're using the right value. I'm wondering if we really need to call computeAutoRepeatTracksCount() during intrinsic size phase or we can simply consider that we've 1 repetition and that the track sizing phase will calculate the right value. I guess this might apply to both columns and rows, not only columns. What do you think? https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1399: if (isRowAxis) { Just in case I was wrong in my previous comment, I think this could be simpler if we use something like: if (isRowAxis) { if sizingOperation != IntrinsicSizeComputation) availableSize = availableLogicalWidth(); } else { ... }
https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1395: LayoutUnit availableSize(-1); On 2016/08/23 07:43:03, Manuel Rego wrote: > Mmm, this means that for columns during intrinsic size computation > we always consider an indefinite width. Even when the width > might have a fixed size. There is no intrinsic size computation with fixed widths. The preferred widths computation is only performed for intrinsic widths. > I'm wondering if we really need to call computeAutoRepeatTracksCount() > during intrinsic size phase or we can simply consider that we've 1 repetition > and that the track sizing phase will calculate the right value. One repetition is one possibility but not the only one, as you can see bellow in the method sometimes you're forced to fulfill a minimum size. > I guess this might apply to both columns and rows, not only columns. > What do you think? placeItemsOnGrid() is only called from layout() or width intrinsic size computation. Grid is indeed symmetrical but the rest of the engine is not. https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1399: if (isRowAxis) { On 2016/08/23 07:43:03, Manuel Rego wrote: > Just in case I was wrong in my previous comment, > I think this could be simpler if we use something like: > if (isRowAxis) { > if sizingOperation != IntrinsicSizeComputation) > availableSize = availableLogicalWidth(); > } else { > ... > } Acknowledged.
The CQ bit was checked by svillar@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org, jfernandez@igalia.com Link to the patchset: https://codereview.chromium.org/2263213002/#ps20001 (title: "Patch for landing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM. https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right): https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutGrid.cpp:1395: LayoutUnit availableSize(-1); On 2016/08/25 09:33:10, svillar wrote: > On 2016/08/23 07:43:03, Manuel Rego wrote: > > Mmm, this means that for columns during intrinsic size computation > > we always consider an indefinite width. Even when the width > > might have a fixed size. > > There is no intrinsic size computation with fixed widths. The preferred widths > computation is only performed for intrinsic widths. > > > I'm wondering if we really need to call computeAutoRepeatTracksCount() > > during intrinsic size phase or we can simply consider that we've 1 repetition > > and that the track sizing phase will calculate the right value. > > One repetition is one possibility but not the only one, as you can see bellow in > the method sometimes you're forced to fulfill a minimum size. > > > I guess this might apply to both columns and rows, not only columns. > > What do you think? > > placeItemsOnGrid() is only called from layout() or width intrinsic size > computation. Grid is indeed symmetrical but the rest of the engine is not. Thanks for the explanations I knew I was missing some points. :-)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by svillar@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Do not recursively call layout during auto repeat computation The computation of auto repeat tracks was incorrectly recursively triggering a layout in order to compute the available size. That was happening whenever the width was indefinite. In such cases we should treat the width always as indefinite without having to run any extra code. During the layout phase we'll have the actual width available. This partially reverts commit 49bc5ba13c492f09971882e2cb842e3514d19922 which was a quick fix for a security issue but that was not actually fixing the real problem behind. BUG=633474 ========== to ========== [css-grid] Do not recursively call layout during auto repeat computation The computation of auto repeat tracks was incorrectly recursively triggering a layout in order to compute the available size. That was happening whenever the width was indefinite. In such cases we should treat the width always as indefinite without having to run any extra code. During the layout phase we'll have the actual width available. This partially reverts commit 49bc5ba13c492f09971882e2cb842e3514d19922 which was a quick fix for a security issue but that was not actually fixing the real problem behind. BUG=633474 Committed: https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1 Cr-Commit-Position: refs/heads/master@{#414446} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1 Cr-Commit-Position: refs/heads/master@{#414446} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
