[CSS Grid Layout] Add support to place items using named grid lines

Source/core/rendering/style/GridResolvedPosition.cpp

Index: Source/core/rendering/style/GridResolvedPosition.cpp
diff --git a/Source/core/rendering/style/GridResolvedPosition.cpp b/Source/core/rendering/style/GridResolvedPosition.cpp
index 7249b15e739b0e8d7dac18dabeae7a6dd85219bb..142c51335d339556d56e3a9d134e52ca6b9b6bf6 100644
--- a/Source/core/rendering/style/GridResolvedPosition.cpp
+++ b/Source/core/rendering/style/GridResolvedPosition.cpp
@@ -80,11 +80,16 @@ size_t GridResolvedPosition::explicitGridSizeForSide(const RenderStyle& gridCont
     return (side == ColumnStartSide || side == ColumnEndSide) ? explicitGridColumnCount(gridContainerStyle) : explicitGridRowCount(gridContainerStyle);
 }
 
+static const NamedGridLinesMap& gridLinesForSide(const RenderStyle& style, GridPositionSide side)
+{
+    return (side == ColumnStartSide || side == ColumnEndSide) ? style.namedGridColumnLines() : style.namedGridRowLines();
+}
+
 GridResolvedPosition GridResolvedPosition::resolveNamedGridLinePositionFromStyle(const RenderStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
 {
     ASSERT(!position.namedGridLine().isNull());
 
-    const NamedGridLinesMap& gridLinesNames = (side == ColumnStartSide || side == ColumnEndSide) ? gridContainerStyle.namedGridColumnLines() : gridContainerStyle.namedGridRowLines();
+    const NamedGridLinesMap& gridLinesNames = gridLinesForSide(gridContainerStyle, side);
     NamedGridLinesMap::const_iterator it = gridLinesNames.find(position.namedGridLine());
     if (it == gridLinesNames.end()) {
         if (position.isPositive())
@@ -101,55 +106,53 @@ GridResolvedPosition GridResolvedPosition::resolveNamedGridLinePositionFromStyle
     return adjustGridPositionForSide(it->value[namedGridLineIndex], side);
 }
 
-GridResolvedPosition GridResolvedPosition::resolveGridPositionFromStyle(const RenderStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
+GridResolvedPosition GridResolvedPosition::resolveExplicitPositionFromStyle(const RenderStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
 {
-    switch (position.type()) {
-    case ExplicitPosition: {
-        ASSERT(position.integerPosition());
+    ASSERT(position.integerPosition());
 
-        if (!position.namedGridLine().isNull())
-            return resolveNamedGridLinePositionFromStyle(gridContainerStyle, position, side);
+    if (!position.namedGridLine().isNull())
+        return resolveNamedGridLinePositionFromStyle(gridContainerStyle, position, side);
 
-        // Handle <integer> explicit position.
-        if (position.isPositive())
-            return adjustGridPositionForSide(position.integerPosition() - 1, side);
+    if (position.isPositive())
+        return adjustGridPositionForSide(position.integerPosition() - 1, side);
 
-        size_t resolvedPosition = abs(position.integerPosition()) - 1;
-        const size_t endOfTrack = explicitGridSizeForSide(gridContainerStyle, side);
+    size_t resolvedPosition = abs(position.integerPosition()) - 1;
+    const size_t endOfTrack = explicitGridSizeForSide(gridContainerStyle, side);
 
-        // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we clamp negative value to the first line.
-        if (endOfTrack < resolvedPosition)
-            return GridResolvedPosition(0);
+    // Per http://lists.w3.org/Archives/Public/www-style/2013Mar/0589.html, we clamp negative value to the first line.
+    if (endOfTrack < resolvedPosition)
+        return 0;
 
-        return adjustGridPositionForSide(endOfTrack - resolvedPosition, side);
-    }
-    case NamedGridAreaPosition:
-    {
-        NamedGridAreaMap::const_iterator it = gridContainerStyle.namedGridArea().find(position.namedGridLine());
-        // Unknown grid area should have been computed to 'auto' by now.
-        ASSERT_WITH_SECURITY_IMPLICATION(it != gridContainerStyle.namedGridArea().end());
-        const GridCoordinate& gridAreaCoordinate = it->value;
-        switch (side) {
-        case ColumnStartSide:
-            return gridAreaCoordinate.columns.resolvedInitialPosition;
-        case ColumnEndSide:
-            return gridAreaCoordinate.columns.resolvedFinalPosition;
-        case RowStartSide:
-            return gridAreaCoordinate.rows.resolvedInitialPosition;
-        case RowEndSide:
-            return GridResolvedPosition(gridAreaCoordinate.rows.resolvedFinalPosition);
+    return adjustGridPositionForSide(endOfTrack - resolvedPosition, side);
+}
+
+GridResolvedPosition GridResolvedPosition::resolveGridPositionFromStyle(const RenderStyle& gridContainerStyle, const GridPosition& position, GridPositionSide side)
+{
+    // We shouldn't see any other type of position here because 'auto' and span depend on the opposite position for
+    // resolution (e.g. grid-row: auto / 1 or grid-column: span 3 / myHeader).
+    ASSERT(position.type() == NamedGridAreaPosition || position.type() == ExplicitPosition);
+
+    if (position.type() == NamedGridAreaPosition) {
+        // Four implicit named grid lines are created for each grid area (areaName-{start|end} for rows and
+        // columns). Authors can either use the area names or the implicit grid line names.
+        NamedGridAreaMap::const_iterator areaIter = gridContainerStyle.namedGridArea().find(position.namedGridLine());

[Julien - ping for review, 2014/05/08 18:48:05]

I think this should be a FIXME: if we inserted grid lines for grid areas as the
specification states, we could follow the exact algorithm from the
specification, which would be less confusing.

[svillar, 2014/05/09 11:14:39]

I quite don't get this comment. We *do* insert grid lines for grid areas in the
StyleBuilder.

[Julien - ping for review, 2014/05/09 17:37:54]

OK, I missed that important piece of information.

+        if (areaIter != gridContainerStyle.namedGridArea().end()) {
+            String implicitNamedGridLine = position.namedGridLine() + ((side == ColumnStartSide || side == RowStartSide) ? "-start" : "-end");
+            const NamedGridLinesMap& gridLineNames = gridLinesForSide(gridContainerStyle, side);
+            NamedGridLinesMap::const_iterator lineIter = gridLineNames.find(implicitNamedGridLine);
+
+            if (lineIter != gridLineNames.end())
+                return adjustGridPositionForSide(lineIter->value[0], side);

[Julien - ping for review, 2014/05/08 18:48:05]

Shouldn't we pick the smallest between the grid-area and the found grid-line?

[svillar, 2014/05/09 11:14:39]

Indeed that's a bug

[svillar, 2014/05/09 14:40:14]

Errr, no the code is correct. The reason why is correct is because the
NamedGridLinesMap already contains the implicit grid lines, so by returning [0]
we ensure that the result is always the right one (we don't mind if it's an
implicit or an explicitly named grid line)

[Julien - ping for review, 2014/05/09 17:37:54]

I agree. Sorry for the noise.

+
+            return areaIter->value.positionForSide(side);
         }
-        ASSERT_NOT_REACHED();
-        return GridResolvedPosition(0);
-    }
-    case AutoPosition:
-    case SpanPosition:
-        // 'auto' and span depend on the opposite position for resolution (e.g. grid-row: auto / 1 or grid-column: span 3 / "myHeader").
-        ASSERT_NOT_REACHED();
-        return GridResolvedPosition(0);
+        // Fallback to a implicit grid line position if there is no grid area with that name.
+        GridPosition adjustedPosition;
+        adjustedPosition.setExplicitPosition(1, position.namedGridLine());
+        return resolveExplicitPositionFromStyle(gridContainerStyle, adjustedPosition, side);

[Julien - ping for review, 2014/05/08 18:48:05]

Why do we need to override the integer position? This would pick the second
line, not the first.

We don't really discriminate between <custom-ident> and <integer> &&
<custom-ident>? so it seems like we can't handle this case correctly as the
specification says to treat it differently depending on the previous cases.

[svillar, 2014/05/09 11:14:39]

We're converting a NamedGridAreaPosition into a explicit position because as
there is no named area with that name, we want to treat it as any other explicit
grid line name.

Regarding picking the second line, I am not sure why you say this, remember that
0 makes a <integer> && <custom-ident>? declaration invalid.
 
I think we properly do that, but not in this function. We handle that case in
resolveNamedGridLinePositionFromStyle() which is called by
resolveExplicitPositionFromStyle().

     }
-    ASSERT_NOT_REACHED();
-    return GridResolvedPosition(0);
+

[Julien - ping for review, 2014/05/08 18:48:05]

I think we should check for the named grid line in this case too.

[svillar, 2014/05/09 11:14:39]

Again, resolveExplicitPositionFromStyle() calls
resolveNamedGridLinePositionFromStyle() for named grid lines.

[svillar, 2014/05/09 11:14:39]

Same comment as above we handle that case in
resolveNamedGridLinePositionFromStyle() which is called by
resolveExplicitPositionFromStyle(). Remember that named grid lines are also
explicit positions.

+    return resolveExplicitPositionFromStyle(gridContainerStyle, position, side);
 }
 
 PassOwnPtr<GridSpan> GridResolvedPosition::resolveGridPositionAgainstOppositePosition(const RenderStyle& gridContainerStyle, const GridResolvedPosition& resolvedOppositePosition, const GridPosition& position, GridPositionSide side)
@@ -175,7 +178,7 @@ PassOwnPtr<GridSpan> GridResolvedPosition::resolveNamedGridLinePositionAgainstOp
     // Negative positions are not allowed per the specification and should have been handled during parsing.
     ASSERT(position.spanPosition() > 0);
 
-    const NamedGridLinesMap& gridLinesNames = (side == ColumnStartSide || side == ColumnEndSide) ? gridContainerStyle.namedGridColumnLines() : gridContainerStyle.namedGridRowLines();
+    const NamedGridLinesMap& gridLinesNames = gridLinesForSide(gridContainerStyle, side);
     NamedGridLinesMap::const_iterator it = gridLinesNames.find(position.namedGridLine());
 
     // If there is no named grid line of that name, we resolve the position to 'auto' (which is equivalent to 'span 1' in this case).