[css-grid] Remove a duplicated auto repeat computation for intrinsic sizes

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 54c60e85354bec2cff57e707ee32a69218964331..f2b5299893e06d33c0d859e951889e2c1f30917e 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -457,11 +457,8 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
 
         TextAutosizer::LayoutScope textAutosizerLayoutScope(this, &layoutScope);
 
-        // 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)
-        if (m_autoRepeatColumns && m_autoRepeatColumns != computeAutoRepeatTracksCount(ForColumns, TrackSizing))
-            dirtyGrid();
-        placeItemsOnGrid(TrackSizing);
+        updateAutoRepeatTracksAndSetDirtyIfNeeded(TrackSizing);
+        placeItemsOnGrid();
 
         GridSizingData sizingData(gridColumnCount(), gridRowCount());
 
@@ -601,7 +598,8 @@ LayoutUnit LayoutGrid::guttersSize(GridTrackSizingDirection direction, size_t st
 
 void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, LayoutUnit& maxLogicalWidth) const
 {
-    const_cast<LayoutGrid*>(this)->placeItemsOnGrid(IntrinsicSizeComputation);
+    updateAutoRepeatTracksAndSetDirtyIfNeeded(IntrinsicSizeComputation);
+    const_cast<LayoutGrid*>(this)->placeItemsOnGrid();

[cbiesinger, 2016/10/05 22:22:13]

You still have a const_cast here. Instead of making dirtyGrid and
updateAutoRepeatTracksAndSetDirtyIfNeeded const functions even though they
really aren't, why not just use a const_cast for both of these function calls?

[svillar, 2016/10/06 08:17:19]

I personally dislike those const_cast a lot, actually I'm planning to remove the
placeItemsOnGrid() one.

I think those two functions you mention are semantically const even though
technically they modify the object. The data we are clearing/modifying in those
methods is cached data used to improve the performance of the layout, that's why
I made them all mutable, it makes sense to modify them in some const methods.

 
     GridSizingData sizingData(gridColumnCount(), gridRowCount());
     computeTrackSizesForIndefiniteSize(ForColumns, sizingData, minLogicalWidth, maxLogicalWidth);
@@ -1440,6 +1438,18 @@ void LayoutGrid::insertItemIntoGrid(LayoutBox& child, const GridArea& area)
     }
 }
 
+void LayoutGrid::updateAutoRepeatTracksAndSetDirtyIfNeeded(SizingOperation sizingOperation) const
+{
+    size_t newAutoRepeatColumns = computeAutoRepeatTracksCount(ForColumns, sizingOperation);

[cbiesinger, 2016/10/05 22:22:13]

Why are you computing this differently now, if sizingOperation==Intrinsic?

[svillar, 2016/10/06 08:17:19]

Actually I'm going back to the original implementation, we made it different to
fix a recursive call to layout. The computeAutoRepeatTracksCount() already
handles the case of computing repetitions during intrinsic size computation
(i.e. computing sizes with indefinite sizes) so we can use it directly.

For most of the cases both implementations are equivalent, but the new code
(which is the original one as mentioned) properly handles some situations that
were not working correctly right now (like for example indefinite sizes with an
specified minimum size).

+    size_t newAutoRepeatRows = computeAutoRepeatTracksCount(ForRows, sizingOperation);
+
+    if (m_autoRepeatColumns != newAutoRepeatColumns || m_autoRepeatRows != newAutoRepeatRows)
+        dirtyGrid();
+
+    m_autoRepeatColumns = newAutoRepeatColumns;
+    m_autoRepeatRows = newAutoRepeatRows;
+}
+
 size_t LayoutGrid::computeAutoRepeatTracksCount(GridTrackSizingDirection direction, SizingOperation sizingOperation) const
 {
     bool isRowAxis = direction == ForColumns;
@@ -1547,7 +1557,7 @@ std::unique_ptr<LayoutGrid::OrderedTrackIndexSet> LayoutGrid::computeEmptyTracks
     return emptyTrackIndexes;
 }
 
-void LayoutGrid::placeItemsOnGrid(SizingOperation sizingOperation)
+void LayoutGrid::placeItemsOnGrid()
 {
     if (!m_gridIsDirty)
         return;
@@ -1555,12 +1565,6 @@ void LayoutGrid::placeItemsOnGrid(SizingOperation sizingOperation)
     DCHECK(m_gridItemArea.isEmpty());
     DCHECK(m_gridItemsIndexesMap.isEmpty());
 
-    if (sizingOperation == IntrinsicSizeComputation)
-        m_autoRepeatColumns = styleRef().gridAutoRepeatColumns().size();

[cbiesinger, 2016/10/05 22:22:13]

(...as compared to this one)

-    else
-        m_autoRepeatColumns = computeAutoRepeatTracksCount(ForColumns, sizingOperation);
-    m_autoRepeatRows = computeAutoRepeatTracksCount(ForRows, sizingOperation);
-
     populateExplicitGridAndOrderIterator();
 
     // We clear the dirty bit here as the grid sizes have been updated.
@@ -1786,7 +1790,7 @@ GridTrackSizingDirection LayoutGrid::autoPlacementMinorAxisDirection() const
     return style()->isGridAutoFlowDirectionColumn() ? ForRows : ForColumns;
 }
 
-void LayoutGrid::dirtyGrid()
+void LayoutGrid::dirtyGrid() const
 {
     if (m_gridIsDirty)
         return;