[CSS Grid Layout] Implementation of the grid-template shorthand.

Source/core/css/parser/BisonCSSParser-in.cpp

Index: Source/core/css/parser/BisonCSSParser-in.cpp
diff --git a/Source/core/css/parser/BisonCSSParser-in.cpp b/Source/core/css/parser/BisonCSSParser-in.cpp
index 3ae3360ca552c196d1ea8f38df5284128d5a7437..2f8428eb1171784fa6958bfcb5f0ceec3633ab16 100644
--- a/Source/core/css/parser/BisonCSSParser-in.cpp
+++ b/Source/core/css/parser/BisonCSSParser-in.cpp
@@ -42,7 +42,6 @@
 #include "core/css/CSSFunctionValue.h"
 #include "core/css/CSSGradientValue.h"
 #include "core/css/CSSGridLineNamesValue.h"
-#include "core/css/CSSGridTemplateAreasValue.h"
 #include "core/css/CSSImageSetValue.h"
 #include "core/css/CSSImageValue.h"
 #include "core/css/CSSInheritedValue.h"
@@ -2394,7 +2393,8 @@ bool CSSPropertyParser::parseValue(CSSPropertyID propId, bool important)
     case CSSPropertyGridTemplateRows:
         if (!RuntimeEnabledFeatures::cssGridLayoutEnabled())
             return false;
-        return parseGridTrackList(propId, important);
+        parsedValue = parseGridTrackList(important);
+        break;
 
     case CSSPropertyGridColumnEnd:
     case CSSPropertyGridColumnStart:
@@ -2422,6 +2422,11 @@ bool CSSPropertyParser::parseValue(CSSPropertyID propId, bool important)
         parsedValue = parseGridTemplateAreas();
         break;
 
+    case CSSPropertyGridTemplate:
+        if (!RuntimeEnabledFeatures::cssGridLayoutEnabled())
+            return false;
+        return parseGridTemplateShorthand(important);
+
     case CSSPropertyWebkitMarginCollapse: {
         if (num == 1) {
             ShorthandScope scope(this, CSSPropertyWebkitMarginCollapse);
@@ -4609,6 +4614,112 @@ bool CSSPropertyParser::parseGridItemPositionShorthand(CSSPropertyID shorthandId
     return true;
 }
 
+
+bool CSSPropertyParser::parseGridTemplateRowsAndAreas(PassRefPtrWillBeRawPtr<CSSValue> templateColumns, bool important)
+{
+    NamedGridAreaMap gridAreaMap;
+    size_t rowCount = 0;
+    size_t columnCount = 0;
+    bool traillingIdentAdded = false;
+    RefPtr<CSSValueList> templateRows = CSSValueList::createSpaceSeparated();
+
+    // At least template-areas strings must be defined.
+    if (!m_valueList->current())
+        return false;
+
+    while (m_valueList->current()) {
+        // Handle leading <custom-ident>*.
+        if (m_valueList->current()->unit == CSSParserValue::ValueList) {
+            if (traillingIdentAdded)
+                parseGridLineNames(m_valueList.get(), *templateRows, static_cast<CSSGridLineNamesValue*>(templateRows->item(templateRows->length() - 1)));

[Julien - ping for review, 2014/03/01 01:56:12]

I don't think I understand what this code is trying to achieve by special
handling of trailingIdent. This is totally me but it means that we are missing
some comment or could make the intent clearer in the code.

[jfernandez, 2014/03/03 13:02:09]

The syntax indicate to concatenate one's row trailing ident with the next row's
heading one. 

I've added a comment in the code clarifying this point.

+            else
+                parseGridLineNames(m_valueList.get(), *templateRows);
+        }
+
+        // Handle a template-area's row.
+        if (!parseGridTemplateAreasRow(gridAreaMap, rowCount, columnCount))
+            return false;
+        ++rowCount;
+
+        // Handle template-rows's track-size.
+        if (m_valueList->current() && m_valueList->current()->unit != CSSParserValue::ValueList && m_valueList->current()->unit != CSSPrimitiveValue::CSS_STRING) {
+            RefPtr<CSSValue> value = parseGridTrackSize(*m_valueList);
+            if (!value)
+                return false;
+            templateRows->append(value);
+        } else {
+            templateRows->append(cssValuePool().createIdentifierValue(CSSValueAuto));
+        }
+
+        // This will handle the trailing/leading <custom-ident>* in the grammar.
+        traillingIdentAdded = false;
+        if (m_valueList->current() && m_valueList->current()->unit == CSSParserValue::ValueList) {
+            parseGridLineNames(m_valueList.get(), *templateRows);
+            traillingIdentAdded = true;
+        }
+    }
+
+    // [<track-list> /]?
+    if (templateColumns)
+        addProperty(CSSPropertyGridTemplateColumns, templateColumns, important);
+    else
+        addProperty(CSSPropertyGridTemplateColumns,  cssValuePool().createIdentifierValue(CSSValueNone), important);
+
+    // [<line-names>? <string> [<track-size> <line-names>]? ]+
+    RefPtr<CSSValue> templateAreas = CSSGridTemplateAreasValue::create(gridAreaMap, rowCount, columnCount);
+    addProperty(CSSPropertyGridTemplateAreas, templateAreas.release(), important);
+    addProperty(CSSPropertyGridTemplateRows, templateRows.release(), important);
+
+
+    return true;
+}
+
+
+bool CSSPropertyParser::parseGridTemplateShorthand(bool important)
+{
+    ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+
+    ShorthandScope scope(this, CSSPropertyGridTemplate);
+    ASSERT(gridTemplateShorthand().length() == 3);
+
+    // At least "none" defined.
+    if (!m_valueList->current())
+        return false;
+
+    RefPtr<CSSValue> columnsValue;
+    RefPtr<CSSValue> rowsValue;
+    unsigned index = 0;
+    bool firstValueIsNone = m_valueList->current()->id == CSSValueNone;

[Julien - ping for review, 2014/03/01 01:56:12]

I think it would be better if we returned early for 'none'. Handling it early
should simplify the rest of the code. You can also easily enumerate the cases in
this case.

[jfernandez, 2014/03/03 13:02:09]

Done.

+
+    if (columnsValue = parseGridTrackList(important)) {
+        if (!m_valueList->current()) {
+            if (firstValueIsNone)
+                rowsValue = cssValuePool().createIdentifierValue(CSSValueNone);
+        } else if (isForwardSlashOperator(m_valueList->current()) && m_valueList->next()) {
+            index = m_valueList->currentIndex();
+            rowsValue = parseGridTrackList(important);
+        }
+    }
+
+    // <grid-template-columns> / <grid-template-columns> syntax.
+    if (columnsValue && rowsValue) {
+        addProperty(CSSPropertyGridTemplateColumns, columnsValue, important);
+        addProperty(CSSPropertyGridTemplateRows, rowsValue, important);
+        addProperty(CSSPropertyGridTemplateAreas, cssValuePool().createIdentifierValue(CSSValueNone), important);
+        return !m_valueList->current();

[Julien - ping for review, 2014/03/01 01:56:12]

I don't think we should be setting the values if we are going to return false
but I may be wrong on this one.

[jfernandez, 2014/03/03 13:02:09]

Done.

+    }
+
+    if (columnsValue && firstValueIsNone)

[Julien - ping for review, 2014/03/01 01:56:12]

Isn't this check redundant? If firstValueIsNone is true, columnsValue can't be
nullptr (it's CSSValueNone as parseGridTrackList early out)

[jfernandez, 2014/03/03 13:02:09]

You are right, I must be blind :) Anyway, I've eventually decided ot move this
check to the parseGridTemplateRowsAndAreas function, because it's part of the
secondary syntax and also is more robust, since it always check the
templateColumns value received as argument is not CSSValueNone.

I had doubts regarding this, specially whether a simple bool in the previous
caller saving the 3 checks and the casts would worth or not, so I'm open to
suggestions. I think code is clearer this way, though.

+        return false; // <track-list> can't be 'none'.
+
+    // Need to rewind parsing to explore the alternative syntax.
+    m_valueList->setCurrentIndex(index);
+
+    // [<track-list> /]? [<line-names>? <string> [<track-size> <line-names>]? ]+ syntax.
+    return parseGridTemplateRowsAndAreas(columnsValue, important);
+}
+
+
 bool CSSPropertyParser::parseGridAreaShorthand(bool important)
 {
     ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
@@ -4664,7 +4775,7 @@ bool CSSPropertyParser::parseSingleGridAreaLonghand(RefPtrWillBeRawPtr<CSSValue>
     return true;
 }
 
-void CSSPropertyParser::parseGridLineNames(CSSParserValueList* parserValueList, CSSValueList& valueList)
+void CSSPropertyParser::parseGridLineNames(CSSParserValueList* parserValueList, CSSValueList& valueList, CSSGridLineNamesValue* lineNamesToConcat)
 {
     ASSERT(parserValueList->current() && parserValueList->current()->unit == CSSParserValue::ValueList);
 
@@ -4674,29 +4785,30 @@ void CSSPropertyParser::parseGridLineNames(CSSParserValueList* parserValueList,
         return;
     }
 
-    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = CSSGridLineNamesValue::create();
+    // Need to ensure the identList is at the heading index, since the parserList might have been rewound.
+    identList->setCurrentIndex(0);

[Julien - ping for review, 2014/03/01 01:56:12]

That's a really bad smell that we need this line. It would seem like some caller
is not doing its job.

[jfernandez, 2014/03/03 13:02:09]

Yeah, this change is interesting. I found out that when rewinding the parser
list, whenever a CSSValueList was part of the syntax next time it was parsed it
was at the end of the list. This is because of the loop the parseGridLineNames
is doing. We can reset the index just after the loop, but I think the effect is
the same and not sure whether is clearer or not.

+
+    RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = lineNamesToConcat ? lineNamesToConcat : CSSGridLineNamesValue::create();
     while (CSSParserValue* identValue = identList->current()) {
         ASSERT(identValue->unit == CSSPrimitiveValue::CSS_IDENT);
         RefPtrWillBeRawPtr<CSSPrimitiveValue> lineName = createPrimitiveStringValue(identValue);
         lineNames->append(lineName.release());
         identList->next();
     }
-    valueList.append(lineNames.release());
+    if (!lineNamesToConcat)
+        valueList.append(lineNames.release());
 
     parserValueList->next();
 }
 
-bool CSSPropertyParser::parseGridTrackList(CSSPropertyID propId, bool important)
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList(bool important)
 {
     ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
 
     CSSParserValue* value = m_valueList->current();
     if (value->id == CSSValueNone) {
-        if (m_valueList->next())
-            return false;
-
-        addProperty(propId, cssValuePool().createIdentifierValue(value->id), important);
-        return true;
+        m_valueList->next();
+        return cssValuePool().createIdentifierValue(CSSValueNone);
     }
 
     RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
@@ -4707,14 +4819,16 @@ bool CSSPropertyParser::parseGridTrackList(CSSPropertyID propId, bool important)
 
     bool seenTrackSizeOrRepeatFunction = false;
     while (CSSParserValue* currentValue = m_valueList->current()) {
+        if (isForwardSlashOperator(currentValue))
+            break;
         if (currentValue->unit == CSSParserValue::Function && equalIgnoringCase(currentValue->function->name, "repeat(")) {
             if (!parseGridTrackRepeatFunction(*values))
-                return false;
+                return nullptr;
             seenTrackSizeOrRepeatFunction = true;
         } else {
             RefPtrWillBeRawPtr<CSSValue> value = parseGridTrackSize(*m_valueList);
             if (!value)
-                return false;
+                return nullptr;
             values->append(value);
             seenTrackSizeOrRepeatFunction = true;
         }
@@ -4726,10 +4840,9 @@ bool CSSPropertyParser::parseGridTrackList(CSSPropertyID propId, bool important)
 
     // We should have found a <track-size> or else it is not a valid <track-list>
     if (!seenTrackSizeOrRepeatFunction)
-        return false;
+        return nullptr;
 
-    addProperty(propId, values.release(), important);
-    return true;
+    return values;
 }
 
 bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
@@ -4827,71 +4940,79 @@ PassRefPtrWillBeRawPtr<CSSPrimitiveValue> CSSPropertyParser::parseGridBreadth(CS
     return createPrimitiveNumericValue(currentValue);
 }
 
-PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTemplateAreas()
+bool CSSPropertyParser::parseGridTemplateAreasRow(NamedGridAreaMap& gridAreaMap, const size_t rowCount, size_t& columnCount)
 {
-    NamedGridAreaMap gridAreaMap;
-    size_t rowCount = 0;
-    size_t columnCount = 0;
-
-    while (CSSParserValue* currentValue = m_valueList->current()) {
-        if (currentValue->unit != CSSPrimitiveValue::CSS_STRING)
-            return nullptr;
+    CSSParserValue* currentValue = m_valueList->current();
+    if (!currentValue || currentValue->unit != CSSPrimitiveValue::CSS_STRING)
+        return false;
 
-        String gridRowNames = currentValue->string;
-        if (!gridRowNames.length())
-            return nullptr;
+    String gridRowNames = currentValue->string;
+    if (!gridRowNames.length())
+        return false;
 
-        Vector<String> columnNames;
-        gridRowNames.split(' ', columnNames);
+    Vector<String> columnNames;
+    gridRowNames.split(' ', columnNames);
 
-        if (!columnCount) {
-            columnCount = columnNames.size();
-            ASSERT(columnCount);
-        } else if (columnCount != columnNames.size()) {
-            // The declaration is invalid is all the rows don't have the number of columns.
-            return nullptr;
-        }
+    if (!columnCount) {
+        columnCount = columnNames.size();
+        ASSERT(columnCount);
+    } else if (columnCount != columnNames.size()) {
+        // The declaration is invalid is all the rows don't have the number of columns.
+        return false;
+    }
 
-        for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
-            const String& gridAreaName = columnNames[currentCol];
+    for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
+        const String& gridAreaName = columnNames[currentCol];
 
-            // Unamed areas are always valid (we consider them to be 1x1).
-            if (gridAreaName == ".")
-                continue;
+        // Unamed areas are always valid (we consider them to be 1x1).
+        if (gridAreaName == ".")
+            continue;
 
-            // We handle several grid areas with the same name at once to simplify the validation code.
-            size_t lookAheadCol;
-            for (lookAheadCol = currentCol; lookAheadCol < (columnCount - 1); ++lookAheadCol) {
-                if (columnNames[lookAheadCol + 1] != gridAreaName)
-                    break;
-            }
+        // We handle several grid areas with the same name at once to simplify the validation code.
+        size_t lookAheadCol;
+        for (lookAheadCol = currentCol; lookAheadCol < (columnCount - 1); ++lookAheadCol) {
+            if (columnNames[lookAheadCol + 1] != gridAreaName)
+                break;
+        }
 
-            NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName);
-            if (gridAreaIt == gridAreaMap.end()) {
-                gridAreaMap.add(gridAreaName, GridCoordinate(GridSpan(rowCount, rowCount), GridSpan(currentCol, lookAheadCol)));
-            } else {
-                GridCoordinate& gridCoordinate = gridAreaIt->value;
+        NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName);
+        if (gridAreaIt == gridAreaMap.end()) {
+            gridAreaMap.add(gridAreaName, GridCoordinate(GridSpan(rowCount, rowCount), GridSpan(currentCol, lookAheadCol)));
+        } else {
+            GridCoordinate& gridCoordinate = gridAreaIt->value;
 
-                // The following checks test that the grid area is a single filled-in rectangle.
-                // 1. The new row is adjacent to the previously parsed row.
-                if (rowCount != gridCoordinate.rows.initialPositionIndex + 1)
-                    return nullptr;
+            // The following checks test that the grid area is a single filled-in rectangle.
+            // 1. The new row is adjacent to the previously parsed row.
+            if (rowCount != gridCoordinate.rows.initialPositionIndex + 1)
+                return false;
 
-                // 2. The new area starts at the same position as the previously parsed area.
-                if (currentCol != gridCoordinate.columns.initialPositionIndex)
-                    return nullptr;
+            // 2. The new area starts at the same position as the previously parsed area.
+            if (currentCol != gridCoordinate.columns.initialPositionIndex)
+                return false;
 
-                // 3. The new area ends at the same position as the previously parsed area.
-                if (lookAheadCol != gridCoordinate.columns.finalPositionIndex)
-                    return nullptr;
+            // 3. The new area ends at the same position as the previously parsed area.
+            if (lookAheadCol != gridCoordinate.columns.finalPositionIndex)
+                return false;
 
-                ++gridCoordinate.rows.finalPositionIndex;
-            }
-            currentCol = lookAheadCol;
+            ++gridCoordinate.rows.finalPositionIndex;
         }
+        currentCol = lookAheadCol;
+    }
+
+    m_valueList->next();
+    return true;
+}
+
+PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTemplateAreas()
+{
+    NamedGridAreaMap gridAreaMap;
+    size_t rowCount = 0;
+    size_t columnCount = 0;
 
+    while (m_valueList->current()) {
+        if (!parseGridTemplateAreasRow(gridAreaMap, rowCount, columnCount))
+            return nullptr;
         ++rowCount;
-        m_valueList->next();
     }
 
     if (!rowCount || !columnCount)