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

Source/core/rendering/RenderGrid.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 644 matching lines @@
 }
 
 size_t RenderGrid::gridItemSpan(const RenderBox* child, GridTrackSizingDirection direction)
 {
     GridCoordinate childCoordinate = cachedGridCoordinate(child);
     GridSpan childSpan = (direction == ForRows) ? childCoordinate.rows : childCoordinate.columns;
 
     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)
 {
     if (item1.second != item2.second)
         return item1.second < item2.second;
 
     return item1.first < item2.first;
 }
 
@@ skipping 10 matching lines @@
         size_t trackIndex = sizingData.contentSizedTracksIndex[i];
         GridIterator iterator(m_grid, direction, trackIndex);
         Vector<GridItemWithSpan> itemsSortedByIncreasingSpan;
 
         while (RenderBox* gridItem = iterator.nextGridItem())
             itemsSortedByIncreasingSpan.append(std::make_pair(gridItem, gridItemSpan(gridItem, direction)));
         std::stable_sort(itemsSortedByIncreasingSpan.begin(), itemsSortedByIncreasingSpan.end(), gridItemWithSpanSorter);
         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];
         if (track.m_maxBreadth == infinity)
             track.m_maxBreadth = track.m_usedBreadth;
     }
 }
 
-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;
 
         GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPosition.toInt()] : sizingData.rowTracks[trackPosition.toInt()];
         sizingData.filteredTracks.append(&track);
     }
 
     if (sizingData.filteredTracks.isEmpty())
         return;
 
@@ skipping 694 matching lines @@
     if (isOutOfFlowPositioned())
         return "RenderGrid (positioned)";
     if (isAnonymous())
         return "RenderGrid (generated)";
     if (isRelPositioned())
         return "RenderGrid (relative positioned)";
     return "RenderGrid";
 }
 
 } // namespace blink