[css-grid] Implement auto-repeat computation

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 cfde1c770390d00cb5a418ebb7f1d54fd22443fa..be9deea740a363e94ec04bb7ab69755bc3624fdc 100644
--- a/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutGrid.cpp
@@ -301,7 +301,8 @@ void LayoutGrid::styleDidChange(StyleDifference diff, const ComputedStyle* oldSt
 
     if (explicitGridDidResize(*oldStyle)
         || namedGridLinesDefinitionDidChange(*oldStyle)
-        || oldStyle->getGridAutoFlow() != styleRef().getGridAutoFlow())
+        || oldStyle->getGridAutoFlow() != styleRef().getGridAutoFlow()
+        || (diff.needsLayout() && (styleRef().gridAutoRepeatColumns().size() || styleRef().gridAutoRepeatRows().size())))

[Manuel Rego, 2016/04/18 21:31:42]

We don't have a test for this.

BTW, why you need the diff.needsLayout()?

[svillar, 2016/04/19 08:09:36]

ACK
Because we don't want to dirty the grid every time there is a style change when
we have autorepeat tracks _unless_ the size have changed.

         dirtyGrid();
 }
 
@@ -310,7 +311,9 @@ bool LayoutGrid::explicitGridDidResize(const ComputedStyle& oldStyle) const
     return oldStyle.gridTemplateColumns().size() != styleRef().gridTemplateColumns().size()
         || oldStyle.gridTemplateRows().size() != styleRef().gridTemplateRows().size()
         || oldStyle.namedGridAreaColumnCount() != styleRef().namedGridAreaColumnCount()
-        || oldStyle.namedGridAreaRowCount() != styleRef().namedGridAreaRowCount();
+        || oldStyle.namedGridAreaRowCount() != styleRef().namedGridAreaRowCount()
+        || oldStyle.gridAutoRepeatColumns().size() != styleRef().gridAutoRepeatColumns().size()
+        || oldStyle.gridAutoRepeatRows().size() != styleRef().gridAutoRepeatRows().size();

[Manuel Rego, 2016/04/18 21:31:41]

We don't have a test for this either.

[svillar, 2016/04/19 08:09:36]

Acknowledged.

 }
 
 bool LayoutGrid::namedGridLinesDefinitionDidChange(const ComputedStyle& oldStyle) const
@@ -694,13 +697,36 @@ static bool shouldClearOverrideContainingBlockContentSizeForChild(const LayoutBo
     return child.hasRelativeLogicalHeight() || child.styleRef().logicalHeight().isIntrinsicOrAuto();
 }
 
-GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t i) const
+const GridTrackSize& LayoutGrid::unresolvedGridTrackSize(GridTrackSizingDirection direction, int i) const

[Manuel Rego, 2016/04/18 21:31:42]

We could find a better name than "i" for the variable. :-)

[svillar, 2016/04/19 08:09:36]

Acknowledged.

 {
     bool isForColumns = direction == ForColumns;

[Manuel Rego, 2016/04/18 21:31:42]

Probably better use "isRowAxis" as in most of the code.

[svillar, 2016/04/19 08:09:35]

Acknowledged.

-    const Vector<GridTrackSize>& trackStyles = isForColumns ? style()->gridTemplateColumns() : style()->gridTemplateRows();
-    const GridTrackSize& autoTrackSize = isForColumns ? style()->gridAutoColumns() : style()->gridAutoRows();
-    int translatedIndex = i + (isForColumns ? m_smallestColumnStart : m_smallestRowStart);
-    const GridTrackSize& trackSize = (translatedIndex < 0 || translatedIndex >= static_cast<int>(trackStyles.size())) ? autoTrackSize : trackStyles[translatedIndex];
+    const Vector<GridTrackSize>& trackStyles = isForColumns ? styleRef().gridTemplateColumns() : styleRef().gridTemplateRows();
+    const Vector<GridTrackSize>& autoRepeatTrackStyles = isForColumns ? styleRef().gridAutoRepeatColumns() : styleRef().gridAutoRepeatRows();
+    const GridTrackSize& autoTrackSize = isForColumns ? styleRef().gridAutoColumns() : styleRef().gridAutoRows();
+    size_t insertionPoint = isForColumns ? styleRef().gridAutoRepeatColumnsInsertionPoint() : styleRef().gridAutoRepeatRowsInsertionPoint();
+    size_t repetitions = autoRepeatCountForDirection(direction);
+
+    // We should not use GridPositionsResolver::explicitGridXXXCount() for this because the
+    // explicit grid might be larger than the number of tracks in grid-template-rows|columns (if
+    // grid-template-areas is specified for example).

[Manuel Rego, 2016/04/18 21:31:42]

But explicitGridXXXCount() now already manages
the grid-template-areas issues.
I'm not sure if I understand completely this comment,
or it's an outdated comment
(before http://crrev.com/1885913002).

[svillar, 2016/04/19 08:09:35]

It's perfectly up to date :). So the thing is that we don't want the number of
explicit tracks but just the size of the trackStyles vector because what we want
is to know where to find the track size. Example:

grid-template-areas: "a b c"
grid-template-columns: 20px;
grid-auto-columns: 10%;

If we ask for the 2nd column (index 1), we should return the auto track because
1>=trackStyles.size(). If we had used the number of explicit tracks we could end
up trying to access trackStyles[1] -> segmentation fault.

[Manuel Rego, 2016/04/19 11:37:30]

Yeah I read the comment wrong, I got it now. Thanks.

+    size_t explicitTracksCount = trackStyles.size() + repetitions;
+
+    if (i < 0 || i >= explicitTracksCount)
+        return autoTrackSize;
+
+    if (LIKELY(!repetitions) || i < insertionPoint)

[Manuel Rego, 2016/04/18 21:31:42]

Wow, first time I see LIKELY in a patch. :-)

[svillar, 2016/04/19 08:09:36]

Not sure if this is a complain or what :). There are not a lot of them but it's
used sometimes.

[Manuel Rego, 2016/04/19 11:37:30]

Just a funny comment, sorry. :-)

+        return trackStyles[i];
+
+    if (i < (insertionPoint + repetitions))
+        return autoRepeatTrackStyles[0];
+
+    return trackStyles[i - repetitions];
+}
+
+GridTrackSize LayoutGrid::gridTrackSize(GridTrackSizingDirection direction, size_t i) const
+{
+    int translatedIndex = i + (direction == ForColumns ? m_smallestColumnStart : m_smallestRowStart);

[Manuel Rego, 2016/04/18 21:31:42]

You could do this inside unresolvedGridTrackSize() as it's only used there.

[svillar, 2016/04/19 08:09:36]

Acknowledged.

+    const GridTrackSize& trackSize = unresolvedGridTrackSize(direction, translatedIndex);
 
     GridLength minTrackBreadth = trackSize.minTrackBreadth();
     GridLength maxTrackBreadth = trackSize.maxTrackBreadth();
@@ -1182,8 +1208,79 @@ void LayoutGrid::insertItemIntoGrid(LayoutBox& child, const GridArea& area)
 
 size_t LayoutGrid::computeAutoRepeatTracksCount(GridTrackSizingDirection direction) const
 {
-    // TODO(svillar): implement the algorithm to compute the number of auto repeat tracks.
-    return 0;
+    bool isRowAxis = direction == ForColumns;
+    const auto& autoRepeatTracks = isRowAxis ? styleRef().gridAutoRepeatColumns() : styleRef().gridAutoRepeatRows();
+
+    if (!autoRepeatTracks.size())
+        return 0;
+
+    DCHECK_EQ(autoRepeatTracks.size(), 1);
+    auto autoTrackSize = autoRepeatTracks.at(0);
+    DCHECK(autoTrackSize.minTrackBreadth().isLength() && !autoTrackSize.minTrackBreadth().isContentSized());

[Manuel Rego, 2016/04/18 21:31:42]

Usually it's preferred to split this in 2 lines,
to get the right error directly if some of the conditions fail.

[svillar, 2016/04/19 08:09:35]

Acknowledged.

+
+    const Length& minSize = isRowAxis ? styleRef().logicalMinWidth() : styleRef().logicalMinHeight();

[Manuel Rego, 2016/04/18 21:31:42]

This should be moved below where it's first used.

[svillar, 2016/04/19 08:09:36]

Acknowledged.

+    LayoutUnit availableSize = isRowAxis ? availableLogicalWidth() : computeContentLogicalHeight(MainOrPreferredSize, styleRef().logicalHeight(), LayoutUnit(-1));
+
+    bool needsToFulfillMinimumSize = false;

[Manuel Rego, 2016/04/18 21:31:42]

Ditto.

[svillar, 2016/04/19 08:09:36]

Acknowledged.

+    if (availableSize == -1) {
+        const Length& maxLength = isRowAxis ? styleRef().logicalMaxWidth() : styleRef().logicalMaxHeight();
+        if (!maxLength.isMaxSizeNone()) {
+            LayoutUnit maxSize = isRowAxis
+                ? computeLogicalWidthUsing(MaxSize, maxLength, containingBlockLogicalWidthForContent(), containingBlock())
+                : computeContentLogicalHeight(MaxSize, maxLength, LayoutUnit(-1));
+            if (maxSize != -1)
+                availableSize = availableSize != -1 ? std::min(availableSize, maxSize) : maxSize;

[Manuel Rego, 2016/04/18 21:31:42]

"availableSize" should be -1 if we are in this part of the if.

[svillar, 2016/04/19 08:09:36]

Acknowledged.

+        }
+    } else {
+        availableSize = isRowAxis
+            ? constrainLogicalWidthByMinMax(availableSize, containingBlockLogicalWidthForContent(), containingBlock())
+            : constrainLogicalHeightByMinMax(availableSize, LayoutUnit(-1));
+    }
+
+    bool indefiniteMainAndMaxSizes = availableSize == LayoutUnit(-1);
+    if (indefiniteMainAndMaxSizes) {
+        if (!minSize.isSpecified())
+            return 1;
+
+        LayoutUnit containingBlockAvailableSize = isRowAxis ? containingBlockLogicalWidthForContent() : containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);
+        availableSize = valueForLength(minSize, containingBlockAvailableSize);
+        needsToFulfillMinimumSize = true;
+    }
+
+    bool hasDefiniteMaxTrackSizingFunction = autoTrackSize.maxTrackBreadth().isLength() && !autoTrackSize.maxTrackBreadth().isContentSized();
+    const Length trackLength = hasDefiniteMaxTrackSizingFunction ? autoTrackSize.maxTrackBreadth().length() : autoTrackSize.minTrackBreadth().length();
+    // For the purpose of finding the number of auto-repeated tracks, the UA must floor the track size to a UA-specified
+    // value to avoid division by zero. It is suggested that this floor be 1px.
+    LayoutUnit autoRepeatTrackSize = std::max<LayoutUnit>(LayoutUnit(1), valueForLength(trackLength, availableSize));
+
+    // There will be always at least 1 auto-repeat track, so take it already into account when computing the total track size
+    LayoutUnit tracksSize = autoRepeatTrackSize;
+    const Vector<GridTrackSize>& trackSizes = isRowAxis ? styleRef().gridTemplateColumns() : styleRef().gridTemplateRows();
+
+    for (const auto& track : trackSizes) {
+        bool hasDefiniteMaxTrackBreadth = track.maxTrackBreadth().isLength() && !track.maxTrackBreadth().isContentSized();
+        DCHECK(hasDefiniteMaxTrackBreadth || (track.minTrackBreadth().isLength() && !track.minTrackBreadth().isContentSized()));
+        tracksSize += valueForLength(hasDefiniteMaxTrackBreadth ? track.maxTrackBreadth().length() : track.minTrackBreadth().length(), availableSize);
+    }
+
+    // Add gutters as if there where only 1 auto repeat track. Gaps between auto repeat tracks will be added later when
+    // computing the repetitions.
+    LayoutUnit gapSize = guttersSize(direction, 2);
+    tracksSize += gapSize * trackSizes.size();
+
+    LayoutUnit freeSpace = availableSize - tracksSize;
+    if (freeSpace <= 0)
+        return 1;
+
+    size_t repetitions = 1 + (freeSpace / (autoRepeatTrackSize + gapSize)).toInt();

[Manuel Rego, 2016/04/18 21:31:42]

I'm not sure if I get this, if you've for example
freeSpace = 600px and autoRepeatTrackSize = 200px (without gaps)
I guess the number of repetitions should be 3,
but this calculation will be 1 + 600/200 = 4.

What I'm missing here?

[svillar, 2016/04/19 08:09:36]

You're missing the freeSpace computation just a couple of lines above. As
tracksSize is at least the size of 1 of the autorepeat tracks, the freeSpace
would be 600-200 = 400.

Then the value of repetitions is 1 + (400/200) = 3.

[Manuel Rego, 2016/04/19 11:37:30]

Thanks again.

+
+    // Otherwise, if the grid container has a definite min size in the relevant axis, the
+    // number of repetitions is the largest possible positive integer that fulfills that
+    // minimum requirement.
+    if (needsToFulfillMinimumSize)
+        ++repetitions;

[Manuel Rego, 2016/04/18 21:31:42]

Again, I don't get why in this case we've to increment
the number of repetitions.
If the grid is using min-width: 600px and width: min-content,
I guess this "needsToFulfillMinimumSize" will be true
and we'll be incrementing the number of repetitions
calculated above.

Also I'm not sure if the comment is 100% accurate,
it seems that the grid container needs to be sized
under a min-content constraint (or something like that)
and not just have a definite min size.
If it has width: 800px and min-width: 600px,
I understand it should enter in this if.

[svillar, 2016/04/19 08:09:36]

Because in the above code we compute the maximum number of repetitions that does
not overflow the container. This means that 1 more repetition will be enough to
fulfill the minimum size requirement.
Well the comment is a literal transcription from the specs, so if you think it
is not accurate you should send an editorial comment :). What the specs say is:

1- if you have definite size or max size -> compute the largest number that does
not overflow
2- otherwise (and it explicitly says otherwise) if the min size is definite then
compute the minimum value that fulfills that requirement.

[Manuel Rego, 2016/04/19 11:37:30]

And thanks again, I was not getting it.
The thing is that the comment on the spec has both parts 1 and 2.
Here we only have the "Otherwise, ..." so it's harder to read,
as we don't have the first part explaining that if the size or max-size
is definite we use that one.

+
+    return std::min<size_t>(repetitions, kGridMaxTracks);

[Manuel Rego, 2016/04/18 21:31:42]

I guess the limit for kGridMaxTracks should be
repetitions + trackSizes.size().
Not sure if really important anyway.

[svillar, 2016/04/19 08:09:36]

Yeah good catch.

 }
 
 void LayoutGrid::placeItemsOnGrid()