[css-grid] Do not recursively call layout during auto repeat computation

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())

[jfernandez, 2016/08/22 14:44:48]

Where is this case defined in the new logic ? Is it something we don't need to
do now ? why ¿

[svillar, 2016/08/22 16:01:52]

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.

[jfernandez, 2016/08/22 16:20:20]

got it now, thanks.

-                : computeContentLogicalHeight(MaxSize, maxLength, LayoutUnit(-1));
+    LayoutUnit availableSize(-1);

[Manuel Rego, 2016/08/23 07:43:03]

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?

[svillar, 2016/08/25 09:33:10]

There is no intrinsic size computation with fixed widths. The preferred widths
computation is only performed for intrinsic widths.
 
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.
placeItemsOnGrid() is only called from layout() or width intrinsic size
computation. Grid is indeed symmetrical but the rest of the engine is not.

[Manuel Rego, 2016/08/25 09:59:35]

Thanks for the explanations I knew I was missing some points. :-)

+    // 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) {

[Manuel Rego, 2016/08/23 07:43:03]

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 {
    ...
  }

[svillar, 2016/08/25 09:33:10]

Acknowledged.

+            DCHECK_NE(sizingOperation, IntrinsicSizeComputation);

[jfernandez, 2016/08/22 14:44:48]

Wouldn't be clearer if we define the DCHECK like isRowAxis) && sizingOperation
!= IntrinsicSizeComputation outside the if ?

[svillar, 2016/08/22 16:01:52]

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.

[jfernandez, 2016/08/22 16:20:20]

I was mistaken, forget about this suggestion. 

+            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();