[css-grid] Cache definite height detection

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 ea4d9ff9a84367692c6ed0672561fcf4c7a4521a..e71428f682b170a4290011ebd334c42d0b5a4571 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -239,9 +239,10 @@ struct LayoutGrid::GridSizingData {
     WTF_MAKE_NONCOPYABLE(GridSizingData);
     STACK_ALLOCATED();
 public:
-    GridSizingData(size_t gridColumnCount, size_t gridRowCount)
+    GridSizingData(size_t gridColumnCount, size_t gridRowCount, bool definiteLogicalHeight)

[jfernandez, 2016/09/14 10:29:37]

better use 'hasDefiniteLogicalHeigth" as variable name.

         : columnTracks(gridColumnCount)
         , rowTracks(gridRowCount)
+        , m_definiteLogicalHeight(definiteLogicalHeight)

[jfernandez, 2016/09/14 10:29:37]

Again, better use the 'has' prefix in the attribute name.

     {
     }
 
@@ -258,6 +259,7 @@ public:
 
     LayoutUnit availableSpace() const { return m_availableSpace; }
     void setAvailableSpace(LayoutUnit availableSpace) { m_availableSpace = availableSpace; }
+    bool hasDefiniteLogicalHeight() const { return m_definiteLogicalHeight; }
 
     SizingOperation sizingOperation { TrackSizing };
     enum SizingState { ColumnSizingFirstIteration, RowSizingFirstIteration, ColumnSizingSecondIteration, RowSizingSecondIteration};
@@ -300,6 +302,8 @@ private:
     // No need to store one per direction as it will be only used for computations during each axis
     // track sizing. It's cached here because we need it to compute relative sizes.
     LayoutUnit m_availableSpace;
+
+    bool m_definiteLogicalHeight;
 };
 
 struct GridItemsSpanGroupRange {
@@ -453,8 +457,6 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
         LayoutSize previousSize = size();
 
         updateLogicalWidth();
-        bool logicalHeightWasIndefinite = !hasDefiniteLogicalHeight();
-
         TextAutosizer::LayoutScope textAutosizerLayoutScope(this, &layoutScope);
 
         // TODO(svillar): we won't need to do this once the intrinsic width computation is isolated
@@ -463,7 +465,7 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
             dirtyGrid();
         placeItemsOnGrid(TrackSizing);
 
-        GridSizingData sizingData(gridColumnCount(), gridRowCount());
+        GridSizingData sizingData(gridColumnCount(), gridRowCount(), hasDefiniteLogicalHeight());

[jfernandez, 2016/09/14 10:29:37]

Are we suer we want to store this information in the GridSizingData structure ?
it seems something that depends more on the grid container box than the
temporary sizing structure data. 

My point is that passing the info as a boolean variable it's a bit strange. Why
not just defining a new class attribute so we can access to it independently of
the sizingData structure ?

[Manuel Rego, 2016/09/15 06:35:28]

Ok, I've moved it to an attribute called m_hasDefiniteLogicalHeight.

 
         // 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()
@@ -475,10 +477,10 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
 
         // 2- Next, the track sizing algorithm resolves the sizes of the grid rows, using the
         // grid column sizes calculated in the previous step.
-        if (logicalHeightWasIndefinite)
-            computeIntrinsicLogicalHeight(sizingData);
-        else
+        if (sizingData.hasDefiniteLogicalHeight())
             computeTrackSizesForDirection(ForRows, sizingData, availableLogicalHeight(ExcludeMarginBorderPadding));
+        else
+            computeIntrinsicLogicalHeight(sizingData);
         setLogicalHeight(computeTrackBasedLogicalHeight(sizingData) + borderAndPaddingLogicalHeight() + scrollbarLogicalHeight());
 
         LayoutUnit oldClientAfterEdge = clientLogicalBottom();
@@ -487,7 +489,7 @@ void LayoutGrid::layoutBlock(bool relayoutChildren)
         // The above call might have changed the grid's logical height depending on min|max height restrictions.
         // Update the sizes of the rows whose size depends on the logical height (also on definite|indefinite sizes).
         LayoutUnit availableSpaceForRows = contentLogicalHeight();
-        if (logicalHeightWasIndefinite)
+        if (!sizingData.hasDefiniteLogicalHeight())
             computeTrackSizesForDirection(ForRows, sizingData, availableSpaceForRows);
 
         // 3- If the min-content contribution of any grid items have changed based on the row
@@ -599,7 +601,7 @@ void LayoutGrid::computeIntrinsicLogicalWidths(LayoutUnit& minLogicalWidth, Layo
 {
     const_cast<LayoutGrid*>(this)->placeItemsOnGrid(IntrinsicSizeComputation);
 
-    GridSizingData sizingData(gridColumnCount(), gridRowCount());
+    GridSizingData sizingData(gridColumnCount(), gridRowCount(), false);
     sizingData.setAvailableSpace(LayoutUnit());
     sizingData.freeSpace(ForColumns) = LayoutUnit();
     sizingData.sizingOperation = IntrinsicSizeComputation;
@@ -670,7 +672,7 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
     // 1. Initialize per Grid track variables.
     for (size_t i = 0; i < tracks.size(); ++i) {
         GridTrack& track = tracks[i];
-        GridTrackSize trackSize = gridTrackSize(direction, i, sizingData.sizingOperation);
+        GridTrackSize trackSize = gridTrackSize(direction, i, sizingData);
 
         track.setBaseSize(computeUsedBreadthOfMinLength(trackSize, maxSize));
         track.setGrowthLimit(computeUsedBreadthOfMaxLength(trackSize, track.baseSize(), maxSize));
@@ -731,10 +733,10 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
     // 4. Grow all Grid tracks having a fraction as the MaxTrackSizingFunction.
     double flexFraction = 0;
     if (hasDefiniteFreeSpace) {
-        flexFraction = findFlexFactorUnitSize(tracks, GridSpan::translatedDefiniteGridSpan(0, tracks.size()), direction, initialFreeSpace);
+        flexFraction = findFlexFactorUnitSize(tracks, GridSpan::translatedDefiniteGridSpan(0, tracks.size()), direction, sizingData, initialFreeSpace);
     } else {
         for (const auto& trackIndex : flexibleSizedTracksIndex)
-            flexFraction = std::max(flexFraction, normalizedFlexFraction(tracks[trackIndex], gridTrackSize(direction, trackIndex).maxTrackBreadth().flex()));
+            flexFraction = std::max(flexFraction, normalizedFlexFraction(tracks[trackIndex], gridTrackSize(direction, trackIndex, sizingData).maxTrackBreadth().flex()));
 
         if (!m_gridItemArea.isEmpty()) {
             for (size_t i = 0; i < flexibleSizedTracksIndex.size(); ++i) {
@@ -746,14 +748,14 @@ void LayoutGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
                     if (i > 0 && span.startLine() <= flexibleSizedTracksIndex[i - 1])
                         continue;
 
-                    flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, maxContentForChild(*gridItem, direction, sizingData)));
+                    flexFraction = std::max(flexFraction, findFlexFactorUnitSize(tracks, span, direction, sizingData, maxContentForChild(*gridItem, direction, sizingData)));
                 }
             }
         }
     }
 
     for (const auto& trackIndex : flexibleSizedTracksIndex) {
-        GridTrackSize trackSize = gridTrackSize(direction, trackIndex);
+        GridTrackSize trackSize = gridTrackSize(direction, trackIndex, sizingData);
 
         LayoutUnit oldBaseSize = tracks[trackIndex].baseSize();
         LayoutUnit baseSize = std::max(oldBaseSize, LayoutUnit(flexFraction * trackSize.maxTrackBreadth().flex()));
@@ -795,7 +797,7 @@ LayoutUnit LayoutGrid::computeUsedBreadthOfMaxLength(const GridTrackSize& trackS
     return LayoutUnit(infinity);
 }
 
-double LayoutGrid::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, GridTrackSizingDirection direction, double flexFactorSum, LayoutUnit& leftOverSpace, const Vector<size_t, 8>& flexibleTracksIndexes, std::unique_ptr<TrackIndexSet> tracksToTreatAsInflexible) const
+double LayoutGrid::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, GridTrackSizingDirection direction, const GridSizingData& sizingData, double flexFactorSum, LayoutUnit& leftOverSpace, const Vector<size_t, 8>& flexibleTracksIndexes, std::unique_ptr<TrackIndexSet> tracksToTreatAsInflexible) const
 {
     // We want to avoid the effect of flex factors sum below 1 making the factor unit size to grow exponentially.
     double hypotheticalFactorUnitSize = leftOverSpace / std::max<double>(1, flexFactorSum);
@@ -807,7 +809,7 @@ double LayoutGrid::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, Gr
         if (additionalTracksToTreatAsInflexible && additionalTracksToTreatAsInflexible->contains(index))
             continue;
         LayoutUnit baseSize = tracks[index].baseSize();
-        double flexFactor = gridTrackSize(direction, index).maxTrackBreadth().flex();
+        double flexFactor = gridTrackSize(direction, index, sizingData).maxTrackBreadth().flex();
         // treating all such tracks as inflexible.
         if (baseSize > hypotheticalFactorUnitSize * flexFactor) {
             leftOverSpace -= baseSize;
@@ -819,11 +821,11 @@ double LayoutGrid::computeFlexFactorUnitSize(const Vector<GridTrack>& tracks, Gr
         }
     }
     if (!validFlexFactorUnit)
-        return computeFlexFactorUnitSize(tracks, direction, flexFactorSum, leftOverSpace, flexibleTracksIndexes, std::move(additionalTracksToTreatAsInflexible));
+        return computeFlexFactorUnitSize(tracks, direction, sizingData, flexFactorSum, leftOverSpace, flexibleTracksIndexes, std::move(additionalTracksToTreatAsInflexible));
     return hypotheticalFactorUnitSize;
 }
 
-double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const GridSpan& tracksSpan, GridTrackSizingDirection direction, LayoutUnit leftOverSpace) const
+double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const GridSpan& tracksSpan, GridTrackSizingDirection direction, const GridSizingData& sizingData, LayoutUnit leftOverSpace) const

[jfernandez, 2016/09/14 10:29:37]

an example of why I don't like a lot the approach you followed is that some
methods, like this one, receive the sizingData argument just to forward it to
the ones actually requiring to know whether the container has a definite logical
height or not.

 {
     if (leftOverSpace <= 0)
         return 0;
@@ -831,7 +833,7 @@ double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const
     double flexFactorSum = 0;
     Vector<size_t, 8> flexibleTracksIndexes;
     for (const auto& trackIndex : tracksSpan) {
-        GridTrackSize trackSize = gridTrackSize(direction, trackIndex);
+        GridTrackSize trackSize = gridTrackSize(direction, trackIndex, sizingData);
         if (!trackSize.maxTrackBreadth().isFlex()) {
             leftOverSpace -= tracks[trackIndex].baseSize();
         } else {
@@ -843,7 +845,7 @@ double LayoutGrid::findFlexFactorUnitSize(const Vector<GridTrack>& tracks, const
     // The function is not called if we don't have <flex> grid tracks
     ASSERT(!flexibleTracksIndexes.isEmpty());
 
-    return computeFlexFactorUnitSize(tracks, direction, flexFactorSum, leftOverSpace, flexibleTracksIndexes);
+    return computeFlexFactorUnitSize(tracks, direction, sizingData, flexFactorSum, leftOverSpace, flexibleTracksIndexes);
 }
 
 static bool hasOverrideContainingBlockContentSizeForChild(const LayoutBox& child, GridTrackSizingDirection direction)
@@ -909,7 +911,7 @@ const GridTrackSize& LayoutGrid::rawGridTrackSize(GridTrackSizingDirection direc
     return trackStyles[untranslatedIndex - autoRepeatTracksCount];
 }
 
-GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t translatedIndex, SizingOperation sizingOperation) const
+GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t translatedIndex, const GridSizingData& sizingData) const
 {
     // Collapse empty auto repeat tracks if auto-fit.
     if (hasAutoRepeatEmptyTracks(direction) && isEmptyAutoRepeatTrack(direction, translatedIndex))
@@ -924,7 +926,7 @@ GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size
     // If the logical width/height of the grid container is indefinite, percentage values are treated as <auto>.
     if (minTrackBreadth.hasPercentage() || maxTrackBreadth.hasPercentage()) {
         // For the inline axis this only happens when we're computing the intrinsic sizes (AvailableSpaceIndefinite).
-        if ((sizingOperation == IntrinsicSizeComputation) || (direction == ForRows && !hasDefiniteLogicalHeight())) {
+        if ((sizingData.sizingOperation == IntrinsicSizeComputation) || (direction == ForRows && !sizingData.hasDefiniteLogicalHeight())) {

[jfernandez, 2016/09/14 10:29:37]

If I understood it correctly, you have passed 'false' as 'definiteLogicalHeight"
argument value when instantiating the SizingData structure for intrinsic size
computation. Since being under intrinsic size computation implies that height is
indefinite, wouldn't be better to just add an assertion here and avoid the
redundant condition ? 

[Manuel Rego, 2016/09/15 06:35:28]

The possibilities are:
* IntrinsicSizeComputation: Both width and height are considered indefinite (the
first condition of the if).
* TrackSizing: Width is always definite in that step. Height can be definite or
indefinite, so we need to check it.

So it's not redundant, if we're in IntrinsicSizeComputation phase, the first
part of the if is true and we consider the size as indefinite. But we need the
other part of the if for the TrackSizing phase.

             if (minTrackBreadth.hasPercentage())
                 minTrackBreadth = Length(Auto);
             if (maxTrackBreadth.hasPercentage())
@@ -1080,10 +1082,10 @@ private:
     GridSpan m_gridSpan;
 };
 
-bool LayoutGrid::spanningItemCrossesFlexibleSizedTracks(const GridSpan& span, GridTrackSizingDirection direction, SizingOperation sizingOperation) const
+bool LayoutGrid::spanningItemCrossesFlexibleSizedTracks(const GridSpan& span, GridTrackSizingDirection direction, const GridSizingData& sizingData) const

[jfernandez, 2016/09/14 10:29:37]

isn't this another example of a method just forwarding the sizingData structure
?

 {
     for (const auto& trackPosition : span) {
-        const GridTrackSize& trackSize = gridTrackSize(direction, trackPosition, sizingOperation);
+        const GridTrackSize& trackSize = gridTrackSize(direction, trackPosition, sizingData);
         if (trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex())
             return true;
     }
@@ -1104,7 +1106,7 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
                     const GridSpan& span = cachedGridSpan(*gridItem, direction);
                     if (span.integerSpan() == 1) {
                         resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, span, *gridItem, track, sizingData);
-                    } else if (!spanningItemCrossesFlexibleSizedTracks(span, direction, sizingData.sizingOperation)) {
+                    } else if (!spanningItemCrossesFlexibleSizedTracks(span, direction, sizingData)) {
                         sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, span));
                     }
                 }
@@ -1135,7 +1137,7 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
 void LayoutGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridSpan& span, LayoutBox& gridItem, GridTrack& track, GridSizingData& sizingData) const
 {
     const size_t trackPosition = span.startLine();
-    GridTrackSize trackSize = gridTrackSize(direction, trackPosition, sizingData.sizingOperation);
+    GridTrackSize trackSize = gridTrackSize(direction, trackPosition, sizingData);
 
     if (trackSize.hasMinContentMinTrackBreadth())
         track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, sizingData)));
@@ -1297,7 +1299,7 @@ void LayoutGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
         sizingData.filteredTracks.shrink(0);
         LayoutUnit spanningTracksSize;
         for (const auto& trackPosition : itemSpan) {
-            GridTrackSize trackSize = gridTrackSize(direction, trackPosition);
+            GridTrackSize trackSize = gridTrackSize(direction, trackPosition, sizingData);
             GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPosition] : sizingData.rowTracks[trackPosition];
             spanningTracksSize += trackSizeForTrackSizeComputationPhase(phase, track, ForbidInfinity);
             if (!shouldProcessTrackForTrackSizeComputationPhase(phase, trackSize))
@@ -1408,7 +1410,7 @@ bool LayoutGrid::tracksAreWiderThanMinTrackBreadth(GridTrackSizingDirection dire
     const Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
     LayoutUnit& maxSize = sizingData.freeSpace(direction);
     for (size_t i = 0; i < tracks.size(); ++i) {
-        GridTrackSize trackSize = gridTrackSize(direction, i, sizingData.sizingOperation);
+        GridTrackSize trackSize = gridTrackSize(direction, i, sizingData);
         if (computeUsedBreadthOfMinLength(trackSize, maxSize) > tracks[i].baseSize())
             return false;
     }
@@ -1868,7 +1870,7 @@ void LayoutGrid::applyStretchAlignmentToTracksIfNeeded(GridTrackSizingDirection
     Vector<GridTrack>& tracks = (direction == ForColumns) ? sizingData.columnTracks : sizingData.rowTracks;
     Vector<unsigned> autoSizedTracksIndex;
     for (unsigned i = 0; i < tracks.size(); ++i) {
-        const GridTrackSize& trackSize = gridTrackSize(direction, i);
+        const GridTrackSize& trackSize = gridTrackSize(direction, i, sizingData);
         if (trackSize.hasAutoMaxTrackBreadth())
             autoSizedTracksIndex.append(i);
     }
@@ -2077,7 +2079,7 @@ GridSpan LayoutGrid::cachedGridSpan(const LayoutBox& gridItem, GridTrackSizingDi
     return direction == ForColumns ? area.columns : area.rows;
 }
 
-LayoutUnit LayoutGrid::assumedRowsSizeForOrthogonalChild(const LayoutBox& child, SizingOperation sizingOperation) const
+LayoutUnit LayoutGrid::assumedRowsSizeForOrthogonalChild(const LayoutBox& child, const GridSizingData& sizingData) const
 {
     DCHECK(isOrthogonalChild(child));
     const GridSpan& span = cachedGridSpan(child, ForRows);
@@ -2085,14 +2087,14 @@ LayoutUnit LayoutGrid::assumedRowsSizeForOrthogonalChild(const LayoutBox& child,
     bool gridAreaIsIndefinite = false;
     LayoutUnit containingBlockAvailableSize = containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);
     for (auto trackPosition : span) {
-        GridLength maxTrackSize = gridTrackSize(ForRows, trackPosition, sizingOperation).maxTrackBreadth();
+        GridLength maxTrackSize = gridTrackSize(ForRows, trackPosition, sizingData).maxTrackBreadth();
         if (maxTrackSize.isContentSized() || maxTrackSize.isFlex())
             gridAreaIsIndefinite = true;
         else
             gridAreaSize += valueForLength(maxTrackSize.length(), containingBlockAvailableSize);
     }
 
-    gridAreaSize += guttersSize(ForRows, span.startLine(), span.integerSpan(), sizingOperation);
+    gridAreaSize += guttersSize(ForRows, span.startLine(), span.integerSpan(), sizingData.sizingOperation);
 
     return gridAreaIsIndefinite ? std::max(child.maxPreferredLogicalWidth(), gridAreaSize) : gridAreaSize;
 }
@@ -2103,7 +2105,7 @@ LayoutUnit LayoutGrid::gridAreaBreadthForChild(const LayoutBox& child, GridTrack
     // may depend on the row track's size. It's possible that the row tracks sizing logic has not been performed yet,
     // so we will need to do an estimation.
     if (direction == ForRows && sizingData.sizingState == GridSizingData::ColumnSizingFirstIteration)
-        return assumedRowsSizeForOrthogonalChild(child, sizingData.sizingOperation);
+        return assumedRowsSizeForOrthogonalChild(child, sizingData);
 
     const Vector<GridTrack>& tracks = direction == ForColumns ? sizingData.columnTracks : sizingData.rowTracks;
     const GridSpan& span = cachedGridSpan(child, direction);