[CSSGridLayout] Skip items spanning flex tracks in track sizing algo

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 2550717785c3938577fe2e03984e08f7e720f2bb..4b272b74e82ce2cb555e24a715623b7f7088f672 100644
--- a/Source/core/rendering/RenderGrid.cpp
+++ b/Source/core/rendering/RenderGrid.cpp
@@ -662,8 +662,6 @@ size_t RenderGrid::gridItemSpan(const RenderBox* child, GridTrackSizingDirection
     return childSpan.resolvedFinalPosition.toInt() - childSpan.resolvedInitialPosition.toInt() + 1;
 }
 
-typedef std::pair<RenderBox*, size_t> GridItemWithSpan;
-
 // This function sorts by span (.second in the pair) but also places pointers (.first in the pair) to the same object in
 // consecutive positions so duplicates could be easily removed with std::unique() for example.
 static bool gridItemWithSpanSorter(const GridItemWithSpan& item1, const GridItemWithSpan& item2)
@@ -694,11 +692,10 @@ void RenderGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
         Vector<GridItemWithSpan>::iterator end = std::unique(itemsSortedByIncreasingSpan.begin(), itemsSortedByIncreasingSpan.end(), uniquePointerInPair);
 
         for (Vector<GridItemWithSpan>::iterator it = itemsSortedByIncreasingSpan.begin(); it != end; ++it) {
-            RenderBox* gridItem = it->first;
-            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, gridItem, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::usedBreadth, &GridTrack::growUsedBreadth);
-            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, gridItem, &GridTrackSize::hasMaxContentMinTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::usedBreadth, &GridTrack::growUsedBreadth);
-            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, gridItem, &GridTrackSize::hasMinOrMaxContentMaxTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::maxBreadthIfNotInfinite, &GridTrack::growMaxBreadth);
-            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, gridItem, &GridTrackSize::hasMaxContentMaxTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::maxBreadthIfNotInfinite, &GridTrack::growMaxBreadth);
+            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, *it, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::usedBreadth, &GridTrack::growUsedBreadth);
+            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, *it, &GridTrackSize::hasMaxContentMinTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::usedBreadth, &GridTrack::growUsedBreadth);
+            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, *it, &GridTrackSize::hasMinOrMaxContentMaxTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::maxBreadthIfNotInfinite, &GridTrack::growMaxBreadth);
+            resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, *it, &GridTrackSize::hasMaxContentMaxTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::maxBreadthIfNotInfinite, &GridTrack::growMaxBreadth);
         }
 
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
@@ -707,15 +704,21 @@ void RenderGrid::resolveContentBasedTrackSizingFunctions(GridTrackSizingDirectio
     }
 }
 
-void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, RenderBox* gridItem, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction)
+void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, GridItemWithSpan& gridItemWithSpan, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction)
 {
+    RenderBox* gridItem = gridItemWithSpan.first;
     const GridCoordinate coordinate = cachedGridCoordinate(gridItem);
     const GridResolvedPosition initialTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
     const GridResolvedPosition finalTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedFinalPosition : coordinate.rows.resolvedFinalPosition;
 
     sizingData.filteredTracks.shrink(0);
+    size_t gridItemSpan = gridItemWithSpan.second;
     for (GridResolvedPosition trackPosition = initialTrackPosition; trackPosition <= finalTrackPosition; ++trackPosition) {
         const GridTrackSize& trackSize = gridTrackSize(direction, trackPosition.toInt());
+        // We should not consider items spanning more than one track that span tracks with flexible sizing functions.
+        if (gridItemSpan > 1 && (trackSize.minTrackBreadth().isFlex() || trackSize.maxTrackBreadth().isFlex()))

[Julien - ping for review, 2014/10/10 18:09:16]

While this will work and remove any grid item spanning a flexible track,
wouldn't it be better filter them out? (this code will be called 4 times per
grid item)

I am thinking that aligning our algorithm with what the algorithm's
specification may help us solve all these discrepancies in our implementation
based on the older specification.

[svillar, 2014/10/13 06:52:32]

That's true.
Do you mean replacing it by a new version based on the rewritten algorithm?

[svillar, 2014/10/13 14:38:44]

BTW doing this would involve an extra hash lookup per item as we need the
cachedGridCoordinate to filter items out. BUT if we wait for issue 592803003 to
land (with the std::pair -> class change) this won't be an issue at all. Another
reason to replace the pair with a class :).

[Julien - ping for review, 2014/10/13 19:43:48]

I would go with incremental changes for that as it's going to be difficult to
land a rewritten algorithm. That means probably starting to rework the way we
iterate and some of our datastructure.

What concerns me is how much bugs we will have due to shoe-horning our current
algorithm into the new one's.

[Julien - ping for review, 2014/10/13 19:43:48]

Another argument for the class, I am officially overruled :)

It's LGTM'ed so I am looking forward to rebasing this change on top of it.

[svillar, 2014/10/14 06:29:13]

Yeah but note that this step was already present in the old algorithm (as it
should be as they're supposed to produce identical results) so we aren't at that
point yet, just polishing the final bits of the "old" implementation.

[Julien - ping for review, 2014/10/15 16:04:58]

100% agreed! I was just mentioning that as it may be better to align ourself
sooner rather than later.

+            return;
+
         if (!(trackSize.*filterFunction)())
             continue;