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

Source/core/css/resolver/StyleAdjuster.cpp

Index: Source/core/css/resolver/StyleAdjuster.cpp
diff --git a/Source/core/css/resolver/StyleAdjuster.cpp b/Source/core/css/resolver/StyleAdjuster.cpp
index c8fd8aa82ed38fc4e1fcd32ff254bf1d5671a2b7..d4b20defe379a471919bc32d8966d4b618fdca91 100644
--- a/Source/core/css/resolver/StyleAdjuster.cpp
+++ b/Source/core/css/resolver/StyleAdjuster.cpp
@@ -46,6 +46,7 @@
 #include "core/rendering/style/RenderStyleConstants.h"
 #include "platform/Length.h"
 #include "wtf/Assertions.h"
+#include "wtf/text/StringBuilder.h"
 
 namespace WebCore {
 
@@ -385,6 +386,79 @@ void StyleAdjuster::adjustRenderStyle(RenderStyle* style, RenderStyle* parentSty
     }
 }
 
+static inline bool gridLineDefinedBeforeGridArea(const String& gridLineName, const String& gridAreaName, const NamedGridAreaMap& gridAreaMap, const NamedGridLinesMap& namedLinesMap, GridPositionSide side)
+{
+    ASSERT(namedLinesMap.contains(gridLineName));
+    // Grid line indexes are inserted in order.
+    size_t namedGridLineFirstDefinitionIndex = namedLinesMap.get(gridLineName)[0];
+
+    ASSERT(gridAreaMap.contains(gridAreaName));
+    const GridCoordinate gridAreaCoordinates = gridAreaMap.get(gridAreaName);

[Julien - ping for review, 2014/02/12 21:55:31]

It should be a reference, we don't need a copy here.

[svillar, 2014/02/13 10:13:39]

yep

+
+    // GridCoordinate refers to tracks while the indexes in namedLinesMap refer to lines, that's why we need to add 1 to
+    // the grid coordinate to get the end line index.

[Julien - ping for review, 2014/02/12 21:55:31]

We have a helper function that does something very similar so I would rather use
it here:

GridPosition::adjustGridPositionForSide

[svillar, 2014/02/13 10:13:39]

Oh indeed, I forgot about that

+    switch (side) {
+    case ColumnStartSide:
+        return namedGridLineFirstDefinitionIndex < gridAreaCoordinates.columns.initialPositionIndex;
+    case ColumnEndSide:
+        return namedGridLineFirstDefinitionIndex < gridAreaCoordinates.columns.finalPositionIndex + 1;
+    case RowStartSide:
+        return namedGridLineFirstDefinitionIndex < gridAreaCoordinates.rows.initialPositionIndex;
+    case RowEndSide:
+        return namedGridLineFirstDefinitionIndex < gridAreaCoordinates.rows.finalPositionIndex + 1;
+    default:
+        ASSERT_NOT_REACHED();
+        return false;
+    }
+}
+
+PassOwnPtr<GridPosition> StyleAdjuster::adjustNamedGridItemPosition(const NamedGridAreaMap& gridAreaMap, const NamedGridLinesMap& namedLinesMap, const GridPosition& position, GridPositionSide side) const
+{
+    ASSERT(position.isNamedGridArea());
+    // The StyleBuilder always treats <ident> as a named grid area. We must decide here if they are going to be resolved

[Julien - ping for review, 2014/02/12 21:55:31]

<ident> is not <custom-ident> so let's try to match the specification in the new
comments, adding FIXMEs where we need to convert <indent> to <custom-ident> in
the parsing.

+    // to either a grid area or a grid line.
+    OwnPtr<GridPosition> adjustedPosition;

[Julien - ping for review, 2014/02/12 21:55:31]

I don't understand why we use OwnPtr here instead of just a plain GridPosition?
That should simplify the code and the caller.

[svillar, 2014/02/28 17:37:13]

Note that I need to use a pointer here, because the function can return a new
GridPosition or not depending on if we have to modify the currently existing
one. By using the OwnPtr/PassOwnPtr stuff we avoid the handling of a raw
pointer.

+
+    String namedGridAreaOrGridLine = position.namedGridLine();
+    const bool hasStartSuffix = namedGridAreaOrGridLine.endsWith("-start");
+    const bool hasEndSuffix = namedGridAreaOrGridLine.endsWith("-end");

[Julien - ping for review, 2014/02/12 21:55:31]

I don't think this logic is right. We are asked to try to match against
<custom-ident>-start / <custom-ident>-end. I don't see where we are asked to
check if the value ends by -start / -end.

I have to munch over this as it seems super gross from an API perspective.

[svillar, 2014/02/13 10:13:39]

Which specific logic do you think is wrong? I'm using the hasStartSuffix and
hasEndSuffix to check if the author is using the implicit line names for a named
grid area. We need to know that because the syntax allows:
- named grid areas
- implicitly named grid lines
- explicitly named grid lines

[Julien - ping for review, 2014/02/13 19:32:01]

The way you handle the named grid line doesn't follow what the prose says. With
grid-row-start: area :

1. If "area" is a valid named area
   1.1. If "area-start" is a valid named line, then place the grid item in the
first named line.
   1.2. Else place the grid item in the start line of the named grid area
"area".
2. If "area" is a named line then place the grid item in the first named line.
3. We cannot resolve the property, it is equivalent to "auto".

[svillar, 2014/02/24 16:44:31]

I think the code works like the algorithm above but it's a little bit different
because it covers more cases. For example, the author may specify:

grid-column: a-start;

so you cannot just run the above algorithm using "a-start" as the name of the
area. You need to extract "a" and check if that's an area name. There are some
other particularities, for example, if in the example above you decide to
resolve a-start to a named area, you have to replace the original position from
the parser (which is a namedgridarea called "a-start") by another one suitable
for the renderer (a namedgridarea called "a") because the renderer will look in
the named areas map using the name specified in the position, so it will never
find the area "a-start" but the area called "a".

The code has also some checks for other stuff like for example, the above
specification should be expanded, according to the spec, to 

grid-column: a-start / a-start;

Should we have a named area called "a", this code will resolve to: 

grid-column: "start line of area a" / auto

because the code looks for "-start" only for column-start|row-start (same for
-end in column-end|row-end).

As a final note, it's very important to know that this method _adjusts_ the
position, so in some cases it returns null which means that the original
position is left untouched. That's the case of what you mention in 1.2 for
example. We could change it and always replace the original position if you
think that improves the readability of the code.

+    const bool isStartSide = side == ColumnStartSide || side == RowStartSide;
+    const bool hasStartSuffixForStartSide = hasStartSuffix && isStartSide;
+    const bool hasEndSuffixForEndSide = hasEndSuffix && !isStartSide;
+    const size_t suffixLength = hasStartSuffix ? strlen("-start") : strlen("-end");
+    String gridAreaName = hasStartSuffixForStartSide || hasEndSuffixForEndSide ? namedGridAreaOrGridLine.substring(0, namedGridAreaOrGridLine.length() - suffixLength) : namedGridAreaOrGridLine;
+
+    if (gridAreaMap.contains(gridAreaName)) {

[Julien - ping for review, 2014/02/13 19:32:01]

Tab pointed out that in the new specification, named areas are not a primitive
concept anymore and thus we could handle them by generating and storing the
appropriate named lines. This would remove a lot of complexity in the resolution
code.

[svillar, 2014/02/24 16:44:31]

That's right, I wonder how that would affect the auto-placement algorithm tough.

+        StringBuilder gridLineNameBuilder;
+        gridLineNameBuilder.append(namedGridAreaOrGridLine);
+        if (isStartSide && !hasStartSuffix)
+            gridLineNameBuilder.append("-start");
+        else if (!isStartSide && !hasEndSuffix)
+            gridLineNameBuilder.append("-end");
+
+        String gridLineName = gridLineNameBuilder.toString();
+        if (namedLinesMap.contains(gridLineName) && gridLineDefinedBeforeGridArea(gridLineName, gridAreaName, gridAreaMap, namedLinesMap, side)) {
+            // Use the explicitly defined grid line defined before the grid area instead of the grid area.
+            adjustedPosition = adoptPtr(new GridPosition());
+            adjustedPosition->setExplicitPosition(1, gridLineName);
+        } else if (hasStartSuffixForStartSide || hasEndSuffixForEndSide) {
+            // Renderer expects the grid area name instead of the implicit grid line name.
+            adjustedPosition = adoptPtr(new GridPosition());
+            adjustedPosition->setNamedGridArea(gridAreaName);
+        }
+
+        return adjustedPosition.release();
+    }
+
+    if (namedLinesMap.contains(namedGridAreaOrGridLine)) {
+        adjustedPosition = adoptPtr(new GridPosition());
+        adjustedPosition->setExplicitPosition(1, namedGridAreaOrGridLine);
+        return adjustedPosition.release();
+    }
+
+    return adoptPtr(new GridPosition());
+}
+
 void StyleAdjuster::adjustGridItemPosition(RenderStyle* style, RenderStyle* parentStyle) const
 {
     const GridPosition& columnStartPosition = style->gridColumnStart();
@@ -403,17 +477,28 @@ void StyleAdjuster::adjustGridItemPosition(RenderStyle* style, RenderStyle* pare
         style->setGridRowEnd(GridPosition());
     }
 
-    // Unknown named grid area compute to 'auto'.
-    const NamedGridAreaMap& map = parentStyle->namedGridArea();
-
-#define CLEAR_UNKNOWN_NAMED_AREA(prop, Prop) \
-    if (prop.isNamedGridArea() && !map.contains(prop.namedGridLine())) \
-        style->setGrid##Prop(GridPosition());
+    // If the grid position is a single <ident> then the spec mandates us to resolve it following this steps:
+    //     * If there is a named grid area called <ident> resolve the position to the area's corresponding edge.
+    //         * If a grid area was found with that name, check that there is no <ident>-start or <ident>-end (depending
+    // on the css property being defined) specified before the grid area. If that's the case resolve to that grid line.
+    //     * Otherwise check if there is a grid line named <ident>.
+    //     * Otherwise treat it as auto.
+
+    const NamedGridLinesMap& namedGridColumnLines = parentStyle->namedGridColumnLines();
+    const NamedGridLinesMap& namedGridRowLines = parentStyle->namedGridRowLines();
+    const NamedGridAreaMap& gridAreaMap = parentStyle->namedGridArea();
+
+#define ADJUST_GRID_POSITION_MAYBE(position, Prop, namedGridLines, side) \
+    if (position.isNamedGridArea()) {                                   \
+        OwnPtr<GridPosition> adjustedPosition = adjustNamedGridItemPosition(gridAreaMap, namedGridLines, position, side); \
+        if (adjustedPosition)                                           \
+            style->setGrid##Prop(*adjustedPosition);                    \
+    }
 
-    CLEAR_UNKNOWN_NAMED_AREA(columnStartPosition, ColumnStart);
-    CLEAR_UNKNOWN_NAMED_AREA(columnEndPosition, ColumnEnd);
-    CLEAR_UNKNOWN_NAMED_AREA(rowStartPosition, RowStart);
-    CLEAR_UNKNOWN_NAMED_AREA(rowEndPosition, RowEnd);
+    ADJUST_GRID_POSITION_MAYBE(columnStartPosition, ColumnStart, namedGridColumnLines, ColumnStartSide);
+    ADJUST_GRID_POSITION_MAYBE(columnEndPosition, ColumnEnd, namedGridColumnLines, ColumnEndSide);
+    ADJUST_GRID_POSITION_MAYBE(rowStartPosition, RowStart, namedGridRowLines, RowStartSide);
+    ADJUST_GRID_POSITION_MAYBE(rowEndPosition, RowEnd, namedGridRowLines, RowEndSide);
 }
 
 }