[CSS Grid Layout] Tracks growing beyond limits when they should not

Source/core/rendering/RenderGrid.cpp

Index: Source/core/rendering/RenderGrid.cpp
diff --git a/Source/core/rendering/RenderGrid.cpp b/Source/core/rendering/RenderGrid.cpp
index 70044e7ebab6ac1f41f513b719b82090b0e035e9..df26232d35fc819e60668318c7581e1449e81ead 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -43,6 +43,7 @@ public:
     GridTrack()
         : m_baseSize(0)
         , m_growthLimit(0)
+        , m_plannedIncrease(0)

[Julien - ping for review, 2015/01/19 16:36:24]

TBH this field doesn't seem to belong here. It's used only in a single place in
the algorithm ("distribute extra space") soit could be kept outside of
GridTrack.

We used to have |distributeTrackVector| to store the planned increase outside
GridTrack and it's unclear that this approach was completely wrong, just that
our implementation was not totally right.

[svillar, 2015/01/20 08:23:24]

It isn't because the previous approach was wrong, just that this solution seemed
convenient as it allows us not to care about sorts in the Vector of tracks. 

Also that would simplify the fix for another bug I found, which is that we
should not update the sizes of tracks after processing each item, but after
processing all the items with the same span value.

Anyway I'll try some other thing.

[Julien - ping for review, 2015/01/20 13:47:14]

We discussed this part of the change over IRC and Sergio gave some good
arguments towards keeping that.

We currently keep the planned increase independently but that conflicts with the
different sorting that are done during the algorithm. We could wrap GridTrack
into a class with the planned increase but there would be little benefit to do
so (we don't store GridTrack outside layout so the memory footprint argument is
moot) and that would yield to more complexity.

All in all, we are better off with the field inside GridTrack.

     {
     }
 
@@ -99,6 +100,19 @@ public:
         return (m_growthLimit == infinity) ? m_baseSize : m_growthLimit;
     }
 
+    const LayoutUnit& plannedIncrease() const { return m_plannedIncrease; }
+    void setPlannedIncrease(const LayoutUnit& plannedIncrease)
+    {
+        ASSERT(plannedIncrease >= 0);
+        m_plannedIncrease = plannedIncrease;
+    }
+
+    void growPlannedIncrease(const LayoutUnit& plannedIncrease)
+    {
+        ASSERT(plannedIncrease >= 0);
+        m_plannedIncrease += plannedIncrease;

[Julien - ping for review, 2015/01/19 16:36:24]

This concerns me because we don't enforce that we don't forget to commit this
planned increase.

+    }
+
 private:
     bool isGrowthLimitBiggerThanBaseSize() const { return growthLimitIsInfinite() || m_growthLimit >= m_baseSize; }
 
@@ -110,6 +124,7 @@ private:
 
     LayoutUnit m_baseSize;
     LayoutUnit m_growthLimit;
+    LayoutUnit m_plannedIncrease;
 };
 
 struct GridTrackForNormalization {
@@ -228,10 +243,9 @@ public:
     Vector<size_t> contentSizedTracksIndex;
 
     // Performance optimization: hold onto these Vectors until the end of Layout to avoid repeated malloc / free.
-    Vector<LayoutUnit> distributeTrackVector;
     Vector<GridTrack*> filteredTracks;
     WillBeHeapVector<GridItemWithSpan> itemsSortedByIncreasingSpan;
-    Vector<size_t> growAboveMaxBreadthTrackIndexes;
+    Vector<GridTrack*> growBeyondGrowthLimitsTracks;
 };
 
 RenderGrid::RenderGrid(Element* element)
@@ -438,7 +452,7 @@ void RenderGrid::computeUsedBreadthOfGridTracks(GridTrackSizingDirection directi
         for (size_t i = 0; i < tracksSize; ++i)
             tracksForDistribution[i] = tracks.data() + i;
 
-        distributeSpaceToTracks(tracksForDistribution, 0, &GridTrack::baseSize, &GridTrack::growBaseSize, sizingData, availableLogicalSpace);
+        distributeSpaceToTracks(tracksForDistribution, nullptr, &GridTrack::baseSize, &GridTrack::growBaseSize, sizingData, availableLogicalSpace);
     } else {
         for (auto& track : tracks)
             track.setBaseSize(track.growthLimit());
@@ -753,7 +767,7 @@ void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
     const GridResolvedPosition initialTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
     const GridResolvedPosition finalTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedFinalPosition : coordinate.rows.resolvedFinalPosition;
 
-    sizingData.growAboveMaxBreadthTrackIndexes.shrink(0);
+    sizingData.growBeyondGrowthLimitsTracks.shrink(0);
     sizingData.filteredTracks.shrink(0);
     for (GridResolvedPosition trackPosition = initialTrackPosition; trackPosition <= finalTrackPosition; ++trackPosition) {
         GridTrackSize trackSize = gridTrackSize(direction, trackPosition.toInt());
@@ -763,23 +777,22 @@ void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizing
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPosition.toInt()] : sizingData.rowTracks[trackPosition.toInt()];
         sizingData.filteredTracks.append(&track);
 
-        if (growAboveMaxBreadthFilterFunction && (trackSize.*growAboveMaxBreadthFilterFunction)())
-            sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size() - 1);
+        if (!growAboveMaxBreadthFilterFunction || (trackSize.*growAboveMaxBreadthFilterFunction)())
+            sizingData.growBeyondGrowthLimitsTracks.append(&track);
     }
 
     if (sizingData.filteredTracks.isEmpty())
         return;
 
-    LayoutUnit additionalBreadthSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks);
+    LayoutUnit extraSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks);
     for (GridResolvedPosition trackIndexForSpace = initialTrackPosition; trackIndexForSpace <= finalTrackPosition; ++trackIndexForSpace) {
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndexForSpace.toInt()] : sizingData.rowTracks[trackIndexForSpace.toInt()];
-        additionalBreadthSpace -= (track.*trackGetter)();
+        extraSpace -= (track.*trackGetter)();
     }
 
-    // Specs mandate to floor additionalBreadthSpace (extra-space in specs) to 0. Instead we directly avoid the function
-    // call in those cases as it will be a noop in terms of track sizing.
-    if (additionalBreadthSpace > 0)
-        distributeSpaceToTracks(sizingData.filteredTracks, &sizingData.growAboveMaxBreadthTrackIndexes, trackGetter, trackGrowthFunction, sizingData, additionalBreadthSpace);
+    extraSpace = std::max<LayoutUnit>(0, extraSpace);
+    Vector<GridTrack*>* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
+    distributeSpaceToTracks(sizingData.filteredTracks, tracksToGrowBeyondGrowthLimits, trackGetter, trackGrowthFunction, sizingData, extraSpace);
 }
 
 static bool sortByGridTrackGrowthPotential(const GridTrack* track1, const GridTrack* track2)
@@ -795,44 +808,37 @@ static bool sortByGridTrackGrowthPotential(const GridTrack* track1, const GridTr
     return (track1->growthLimit() - track1->baseSize()) < (track2->growthLimit() - track2->baseSize());
 }
 
-void RenderGrid::distributeSpaceToTracks(Vector<GridTrack*>& tracks, Vector<size_t>* growAboveMaxBreadthTrackIndexes, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, GridSizingData& sizingData, LayoutUnit& availableLogicalSpace)
+void RenderGrid::distributeSpaceToTracks(Vector<GridTrack*>& tracks, const Vector<GridTrack*>* growBeyondGrowthLimitsTracks, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, GridSizingData& sizingData, LayoutUnit& availableLogicalSpace)
 {
-    ASSERT(availableLogicalSpace > 0);

[Julien - ping for review, 2015/01/19 16:36:24]

If you don't have any |availableLogicalSpace|, it seems wasteful to try to
distribute it below.

[svillar, 2015/01/20 08:23:24]

Correct, I removed it by accident, it was part of a previous downstream instance
of the patch. I'll bring it back.

     std::sort(tracks.begin(), tracks.end(), sortByGridTrackGrowthPotential);
 
     size_t tracksSize = tracks.size();
-    sizingData.distributeTrackVector.resize(tracksSize);
-
-    for (size_t i = 0; i < tracksSize; ++i) {
-        GridTrack& track = *tracks[i];
-        LayoutUnit availableLogicalSpaceShare = availableLogicalSpace / (tracksSize - i);
-        const LayoutUnit& trackBreadth = (tracks[i]->*trackGetter)();
-        LayoutUnit growthShare = track.growthLimitIsInfinite() ? availableLogicalSpaceShare : std::min(availableLogicalSpaceShare, track.growthLimit() - trackBreadth);
-        sizingData.distributeTrackVector[i] = trackBreadth;
+    size_t i = 0;
+    for (auto* track : tracks) {
+        LayoutUnit availableLogicalSpaceShare = availableLogicalSpace / (tracksSize - i++);
+        const LayoutUnit& trackBreadth = (track->*trackGetter)();
+        LayoutUnit growthShare = track->growthLimitIsInfinite() ? availableLogicalSpaceShare : std::min(availableLogicalSpaceShare, track->growthLimit() - trackBreadth);
+        track->setPlannedIncrease(0);
         // We should never shrink any grid track or else we can't guarantee we abide by our min-sizing function.
         if (growthShare > 0) {
-            sizingData.distributeTrackVector[i] += growthShare;
+            track->growPlannedIncrease(growthShare);
             availableLogicalSpace -= growthShare;
         }
     }
 
-    if (availableLogicalSpace > 0 && growAboveMaxBreadthTrackIndexes) {
-        size_t indexesSize = growAboveMaxBreadthTrackIndexes->size();
-        size_t tracksGrowingAboveMaxBreadthSize = indexesSize ? indexesSize : tracksSize;
-        // If we have a non-null empty vector of track indexes to grow above max breadth means that we should grow all
-        // affected tracks.
-        for (size_t i = 0; i < tracksGrowingAboveMaxBreadthSize; ++i) {
-            LayoutUnit growthShare = availableLogicalSpace / (tracksGrowingAboveMaxBreadthSize - i);
-            size_t distributeTrackIndex = indexesSize ? growAboveMaxBreadthTrackIndexes->at(i) : i;
-            sizingData.distributeTrackVector[distributeTrackIndex] += growthShare;
+    if (availableLogicalSpace > 0 && growBeyondGrowthLimitsTracks) {
+        size_t tracksGrowingAboveMaxBreadthSize = growBeyondGrowthLimitsTracks->size();
+        size_t i = 0;
+        for (auto* track : *growBeyondGrowthLimitsTracks) {
+            LayoutUnit growthShare = availableLogicalSpace / (tracksGrowingAboveMaxBreadthSize - i++);

[Julien - ping for review, 2015/01/19 16:36:24]

The pattern with the auto-for loop + manual iterator doesn't seem like an
improvement over the old code that was explicit.

[svillar, 2015/01/20 08:23:24]

Yeah, I don't have a strong opinion here, I can rollback this change.

+            track->growPlannedIncrease(growthShare);
             availableLogicalSpace -= growthShare;
         }
     }
 
-    for (size_t i = 0; i < tracksSize; ++i) {
-        LayoutUnit growth = sizingData.distributeTrackVector[i] - (tracks[i]->*trackGetter)();
-        if (growth >= 0)
-            (tracks[i]->*trackGrowthFunction)(growth);
+    for (auto* track: tracks) {
+        if (auto plannedIncrease = track->plannedIncrease())
+            (track->*trackGrowthFunction)(plannedIncrease);
     }
 }