[css-grid] Do not recursively call layout during auto repeat computation (issue 2263213002 by svillar@igalia.com)

[svi...@igalia.com]

Reviewers: cbiesinger, Manuel Rego
CL: https://codereview.chromium.org/2263213002/

Message:
Adding reviewers who checked the original patch

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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+30, -22 lines):
M third_party/WebKit/Source/core/layout/LayoutGrid.h
M third_party/WebKit/Source/core/layout/LayoutGrid.cpp


Index: third_party/WebKit/Source/core/layout/LayoutGrid.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
index 784ab711f8449cd0e337ac60d7f8090aba6c38b0..2f0dccccb881997d688f2c3479d3455045263d40 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -441,10 +441,9 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)

// TODO(svillar): we won't need to do this once the intrinsic width computation is isolated
// from the LayoutGrid object state (it should not touch any attribute) (see crbug.com/627812)
- size_t autoRepeatColumnsCount = computeAutoRepeatTracksCount(ForColumns);
- if (m_autoRepeatColumns && m_autoRepeatColumns != autoRepeatColumnsCount)
+ if (m_autoRepeatColumns && m_autoRepeatColumns != computeAutoRepeatTracksCount(ForColumns, TrackSizing))
dirtyGrid();
- placeItemsOnGrid(autoRepeatColumnsCount);
+ placeItemsOnGrid(TrackSizing);

GridSizingData sizingData(gridColumnCount(), gridRowCount());

@@ -575,7 +574,7 @@ LayoutUnit LayoutGrid::guttersSize(GridTrackSizingDirection direction, size_t st

void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
{
- const_cast<LayoutGrid*>(this)->placeItemsOnGrid(styleRef().gridAutoRepeatColumns().size());
+ const_cast<LayoutGrid*>(this)->placeItemsOnGrid(IntrinsicSizeComputation);

GridSizingData sizingData(gridColumnCount(), gridRowCount());
sizingData.freeSpaceForDirection(ForColumns) = LayoutUnit();
@@ -1384,7 +1383,7 @@ void LayoutGrid::insertItemIntoGrid(LayoutBox& child, const GridArea& area)
}
}

-size_t LayoutGrid::computeAutoRepeatTracksCount(GridTrackSizingDirection direction) const
+size_t LayoutGrid::computeAutoRepeatTracksCount(GridTrackSizingDirection direction, SizingOperation sizingOperation) const
{
bool isRowAxis = direction == ForColumns;
const auto& autoRepeatTracks = isRowAxis ? styleRef().gridAutoRepeatColumns() : styleRef().gridAutoRepeatRows();
@@ -1393,18 +1392,23 @@ size_t LayoutGrid::computeAutoRepeatTracksCount(GridTrackSizingDirection directi
if (!autoRepeatTrackListLength)
return 0;

- LayoutUnit availableSize = isRowAxis ? availableLogicalWidth() : computeContentLogicalHeight(MainOrPreferredSize, styleRef().logicalHeight(), LayoutUnit(-1));
- if (availableSize == -1) {
- const Length& maxLength = isRowAxis ? styleRef().logicalMaxWidth() : styleRef().logicalMaxHeight();
- if (!maxLength.isMaxSizeNone()) {
- availableSize = isRowAxis
- ? computeLogicalWidthUsing(MaxSize, maxLength, containingBlockLogicalWidthForContent(), containingBlock())
- : computeContentLogicalHeight(MaxSize, maxLength, LayoutUnit(-1));
+ LayoutUnit availableSize(-1);
+ // Widths are always definite except during intrinsic size computation. For heights we need to
+ // check whether the computed logical height is -1 or not to determine it.
+ if (sizingOperation != IntrinsicSizeComputation || !isRowAxis) {
+ if (isRowAxis) {
+ DCHECK_NE(sizingOperation, IntrinsicSizeComputation);
+ availableSize = availableLogicalWidth();
+ } else {
+ availableSize = computeContentLogicalHeight(MainOrPreferredSize, styleRef().logicalHeight(), LayoutUnit(-1));
+ if (availableSize == -1) {
+ const Length& maxLength = styleRef().logicalMaxHeight();
+ if (!maxLength.isMaxSizeNone())
+ availableSize = computeContentLogicalHeight(MaxSize, maxLength, LayoutUnit(-1));
+ } else {
+ availableSize = constrainLogicalHeightByMinMax(availableSize, LayoutUnit(-1));
+ }
}
- } else {
- availableSize = isRowAxis
- ? constrainLogicalWidthByMinMax(availableSize, availableLogicalWidth(), containingBlock())
- : constrainLogicalHeightByMinMax(availableSize, LayoutUnit(-1));
}

bool needsToFulfillMinimumSize = false;
@@ -1491,15 +1495,19 @@ std::unique_ptr<LayoutGrid::OrderedTrackIndexSet> LayoutGrid::computeEmptyTracks
return emptyTrackIndexes;
}

-void LayoutGrid::placeItemsOnGrid(size_t autoRepeatColumnsCount)
+void LayoutGrid::placeItemsOnGrid(SizingOperation sizingOperation)
{
if (!m_gridIsDirty)
return;

- ASSERT(m_gridItemArea.isEmpty());
+ DCHECK(m_gridItemArea.isEmpty());
+ DCHECK(m_gridItemsIndexesMap.isEmpty());

- m_autoRepeatColumns = autoRepeatColumnsCount;
- m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows);
+ if (sizingOperation == IntrinsicSizeComputation)
+ m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().size();
+ else
+ m_autoRepeatColumns = computeAutoRepeatTracksCount(ForColumns, sizingOperation);
+ m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows, sizingOperation);

populateExplicitGridAndOrderIterator();

Index: third_party/WebKit/Source/core/layout/LayoutGrid.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutGrid.h b/third_party/WebKit/Source/core/layout/LayoutGrid.h
index 90ebefe8bdd5a178db2ee9dd9e602e58e430ffc2..8588ff69fb9280733ec1e662c407ea85b416edb4 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.h
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.h
@@ -124,7 +124,7 @@ private:
void ensureGridSize(size_t maximumRowSize, size_t maximumColumnSize);
void insertItemIntoGrid(LayoutBox&, const GridArea&);

- size_t computeAutoRepeatTracksCount(GridTrackSizingDirection) const;
+ size_t computeAutoRepeatTracksCount(GridTrackSizingDirection, SizingOperation) const;

typedef ListHashSet<size_t> OrderedTrackIndexSet;
std::unique_ptr<OrderedTrackIndexSet> computeEmptyTracksForAutoRepeat(GridTrackSizingDirection) const;
@@ -132,7 +132,7 @@ private:
bool hasAutoRepeatEmptyTracks(GridTrackSizingDirection) const;
bool isEmptyAutoRepeatTrack(GridTrackSizingDirection, size_t lineNumber) const;

- void placeItemsOnGrid(size_t autoRepeatColumnsCount);
+ void placeItemsOnGrid(SizingOperation);
void populateExplicitGridAndOrderIterator();
std::unique_ptr<GridArea> createEmptyGridAreaAtSpecifiedPositionsOutsideGrid(const LayoutBox&, GridTrackSizingDirection, const GridSpan& specifiedPositions) const;
void placeSpecifiedMajorAxisItemsOnGrid(const Vector<LayoutBox*>&);

[jfern...@igalia.com]

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#oldcode1401
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/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1400
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 ?

https://codereview.chromium.org/2263213002/

[svi...@igalia.com]

Thanks for the quick review!

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#oldcode1401
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/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1400
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/

[jfern...@igalia.com]

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (left):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#oldcode1401
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/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1400
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.

https://codereview.chromium.org/2263213002/

[cbies...@chromium.org]

lgtm as long as Javier is happy :)

https://codereview.chromium.org/2263213002/

[jfern...@igalia.com]

lgtm



https://codereview.chromium.org/2263213002/

[re...@igalia.com]

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1395
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/core/layout/LayoutGrid.cpp#newcode1399
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/

[svi...@igalia.com]

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1395
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/core/layout/LayoutGrid.cpp#newcode1399
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.

https://codereview.chromium.org/2263213002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2263213002/20001


https://codereview.chromium.org/2263213002/

[re...@igalia.com]

LGTM.

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
File third_party/WebKit/Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/2263213002/diff/1/third_party/WebKit/Source/core/layout/LayoutGrid.cpp#newcode1395
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. :-)

https://codereview.chromium.org/2263213002/

[commit-bot@chromium.org via codereview.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_ng/builds/280953)

https://codereview.chromium.org/2263213002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001)

https://codereview.chromium.org/2263213002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 2 (id:??) landed as
https://crrev.com/fd60fc3c2c5ebc47c9fe7e1bbb7fe1bb6dc19ae1
Cr-Commit-Position: refs/heads/master@{#414446}

https://codereview.chromium.org/2263213002/