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

third_party/WebKit/Source/core/layout/LayoutGrid.cpp

 /*
  * Copyright (C) 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 439 matching lines @@
         // may trigger relayout).
         LayoutState state(*this, locationOffset());
 
         LayoutSize previousSize = size();
 
         updateLogicalWidth();
         m_hasDefiniteLogicalHeight = hasDefiniteLogicalHeight();
 
         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());
 
         // 1- First, the track sizing algorithm is used to resolve the sizes of the grid columns.
         // At this point the logical width is always definite as the above call to updateLogicalWidth()
         // properly resolves intrinsic sizes. We cannot do the same for heights though because many code
         // paths inside updateLogicalHeight() require a previous call to setLogicalHeight() to resolve
         // heights properly (like for positioned items for example).
         LayoutUnit availableSpaceForColumns = availableLogicalWidth();
         computeTrackSizesForDefiniteSize(ForColumns, sizingData, availableSpaceForColumns);
@@ skipping 119 matching lines @@
         }
         if (nonEmptyTracksAfterEndLine)
             gapAccumulator += gap;
     }
 
     return gapAccumulator;
 }
 
 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);
 
     LayoutUnit scrollbarWidth = LayoutUnit(scrollbarLogicalWidth());
     minLogicalWidth += scrollbarWidth;
     maxLogicalWidth += scrollbarWidth;
 }
 
 void LayoutGrid::computeTrackSizesForIndefiniteSize(GridTrackSizingDirection direction, GridSizingData& sizingData, LayoutUnit& minIntrinsicSize, LayoutUnit& maxIntrinsicSize) const
@@ skipping 818 matching lines @@
 {
     RELEASE_ASSERT(area.rows.isTranslatedDefinite() && area.columns.isTranslatedDefinite());
     ensureGridSize(area.rows.endLine(), area.columns.endLine());
 
     for (const auto& row : area.rows) {
         for (const auto& column: area.columns)
             m_grid[row][column].append(&child);
     }
 }
 
+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;
     const auto& autoRepeatTracks = isRowAxis ? styleRef().gridAutoRepeatColumns() : styleRef().gridAutoRepeatRows();
     size_t autoRepeatTrackListLength = autoRepeatTracks.size();
 
     if (!autoRepeatTrackListLength)
         return 0;
 
     LayoutUnit availableSize;
@@ skipping 87 matching lines @@
             if (!iterator.nextGridItem()) {
                 if (!emptyTrackIndexes)
                     emptyTrackIndexes = wrapUnique(new OrderedTrackIndexSet);
                 emptyTrackIndexes->add(trackIndex);
             }
         }
     }
     return emptyTrackIndexes;
 }
 
-void LayoutGrid::placeItemsOnGrid(SizingOperation sizingOperation)
+void LayoutGrid::placeItemsOnGrid()
 {
     if (!m_gridIsDirty)
         return;
 
     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.
     m_gridIsDirty = false;
 
     Vector<LayoutBox*> autoMajorAxisAutoGridItems;
     Vector<LayoutBox*> specifiedMajorAxisAutoGridItems;
     m_hasAnyOrthogonalChild = false;
     for (LayoutBox* child = m_orderIterator.first(); child; child = m_orderIterator.next()) {
         if (child->isOutOfFlowPositioned())
@@ skipping 205 matching lines @@
 GridTrackSizingDirection LayoutGrid::autoPlacementMajorAxisDirection() const
 {
     return style()->isGridAutoFlowDirectionColumn() ? ForColumns : ForRows;
 }
 
 GridTrackSizingDirection LayoutGrid::autoPlacementMinorAxisDirection() const
 {
     return style()->isGridAutoFlowDirectionColumn() ? ForRows : ForColumns;
 }
 
-void LayoutGrid::dirtyGrid()
+void LayoutGrid::dirtyGrid() const
 {
     if (m_gridIsDirty)
         return;
 
     m_grid.resize(0);
     m_gridItemArea.clear();
     m_gridItemsOverflowingGridArea.resize(0);
     m_gridItemsIndexesMap.clear();
     m_autoRepeatColumns = 0;
     m_autoRepeatRows = 0;
@@ skipping 870 matching lines @@
         GridPainter(*this).paintChildren(paintInfo, paintOffset);
 }
 
 bool LayoutGrid::cachedHasDefiniteLogicalHeight() const
 {
     SECURITY_DCHECK(m_hasDefiniteLogicalHeight);
     return m_hasDefiniteLogicalHeight.value();
 }
 
 } // namespace blink