[CSSGridLayout] Switch from parentheses to brackets for grid line names

Source/core/css/parser/CSSPropertyParser.cpp

Index: Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/Source/core/css/parser/CSSPropertyParser.cpp b/Source/core/css/parser/CSSPropertyParser.cpp
index 119ea6bf959ffa64c81bf48d7c10d1bf913d9a73..2efde4e190207b6e6b736cb6393e878af2bba70f 100644
--- a/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/Source/core/css/parser/CSSPropertyParser.cpp
@@ -3228,12 +3228,9 @@ bool CSSPropertyParser::parseGridTemplateRowsAndAreas(PassRefPtrWillBeRawPtr<CSS
 
     while (m_valueList->current()) {
         // Handle leading <custom-ident>*.
-        if (trailingIdentWasAdded) {
-            // A row's trailing ident must be concatenated with the next row's leading one.
-            maybeParseGridLineNames(*m_valueList, *templateRows, toCSSGridLineNamesValue(templateRows->item(templateRows->length() - 1)));
-        } else {
-            maybeParseGridLineNames(*m_valueList, *templateRows);
-        }
+

[jfernandez, 2015/06/04 22:49:56]

Unneeded white line. 

+        if (!maybeParseGridLineNames(*m_valueList, *templateRows, trailingIdentWasAdded ? toCSSGridLineNamesValue(templateRows->item(templateRows->length() - 1)) : nullptr))
+            return false;

[Julien - ping for review, 2015/06/04 23:17:58]

Aren't you tightening our parsing here and thus this should get some extra test
coverage?

[svillar, 2015/06/05 07:25:43]

Actually I acknowledge that it looks like that we tighten our parsing, but in
fact we aren't. I'll explain why:

* with the old bison-based parser, the grammar allowed us to create a
CSSParserValue::ValueList object for each ident list. That simplified a lot the
property parsing code as we didn't have to verify the existence of
opening/closing parentheses, nested parentheses etc... the grammar was just
rejecting those rules before our property parsing code was executed.

* with the new one, we just get a flow of tokens that need to be parsed, and
thus, we're the ones who need to do all the verifications.

 
         // Handle a template-area's row.
         if (!parseGridTemplateAreasRow(gridAreaMap, rowCount, columnCount))
@@ -3253,7 +3250,8 @@ bool CSSPropertyParser::parseGridTemplateRowsAndAreas(PassRefPtrWillBeRawPtr<CSS
         // This will handle the trailing/leading <custom-ident>* in the grammar.
         const CSSParserValue* current = m_valueList->current();
         trailingIdentWasAdded = current && current->unit == CSSParserValue::ValueList && current->valueList->size() > 0;
-        maybeParseGridLineNames(*m_valueList, *templateRows);
+        if (!maybeParseGridLineNames(*m_valueList, *templateRows))
+            return false;
     }
 
     // [<track-list> /]?
@@ -3441,34 +3439,44 @@ bool CSSPropertyParser::parseSingleGridAreaLonghand(RefPtrWillBeRawPtr<CSSValue>
     return true;
 }
 
-void CSSPropertyParser::maybeParseGridLineNames(CSSParserValueList& inputList, CSSValueList& valueList, CSSGridLineNamesValue* previousNamedAreaTrailingLineNames)
+static inline bool isClosingBracket(CSSParserValue* value)

[jfernandez, 2015/06/04 22:49:56]

Shouldn't value argument be 'const' ?

[Julien - ping for review, 2015/06/04 23:17:58]

Even better a constant reference.

[svillar, 2015/06/05 07:25:43]

Acknowledged.

 {
-    if (!inputList.current() || inputList.current()->unit != CSSParserValue::ValueList)
-        return;
+    return value->unit == CSSParserValue::Operator && value->iValue == ']';
+}
 
-    CSSParserValueList* identList = inputList.current()->valueList;
-    if (!identList->size()) {
-        inputList.next();
-        return;
-    }
+bool CSSPropertyParser::maybeParseGridLineNames(CSSParserValueList& inputList, CSSValueList& valueList, CSSGridLineNamesValue* previousNamedAreaTrailingLineNames)
+{
+    if (!inputList.current() || inputList.current()->unit != CSSParserValue::Operator || inputList.current()->iValue != '[')

[jfernandez, 2015/06/04 22:49:57]

Wouldn't be a good idea to define  a 'isOpenBracket' inline function as well ?

[svillar, 2015/06/05 07:25:43]

I don't think so, it'd be just used here.

+        return true;
 
-    // Need to ensure the identList is at the heading index, since the parserList might have been rewound.
-    identList->setCurrentIndex(0);
+    // Skip '['
+    inputList.next();
 
     RefPtrWillBeRawPtr<CSSGridLineNamesValue> lineNames = previousNamedAreaTrailingLineNames;
     if (!lineNames)
         lineNames = CSSGridLineNamesValue::create();
-    while (CSSParserValue* identValue = identList->current()) {
+
+    while (CSSParserValue* identValue = inputList.current()) {
+        if (isClosingBracket(identValue))
+            break;
+
         if (identValue->unit != CSSPrimitiveValue::CSS_IDENT)
-            return;
+            return false;
+
         RefPtrWillBeRawPtr<CSSPrimitiveValue> lineName = createPrimitiveCustomIdentValue(identValue);
         lineNames->append(lineName.release());
-        identList->next();
+        inputList.next();
     }
+
+    if (!inputList.current() || !isClosingBracket(inputList.current()))
+        return false;
+
     if (!previousNamedAreaTrailingLineNames)
         valueList.append(lineNames.release());
 
+    // Consume ']'
     inputList.next();
+    return true;
 }
 
 PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList()
@@ -3483,7 +3491,8 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList()
 
     RefPtrWillBeRawPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
     // Handle leading  <ident>*.

[jfernandez, 2015/06/04 22:49:56]

Shouldn't be <custom-ident> ?

[svillar, 2015/06/05 07:25:43]

Yes, that's a legacy comment.

-    maybeParseGridLineNames(*m_valueList, *values);
+    if (!maybeParseGridLineNames(*m_valueList, *values))
+        return nullptr;
 
     bool seenTrackSizeOrRepeatFunction = false;
     while (CSSParserValue* currentValue = m_valueList->current()) {
@@ -3501,7 +3510,8 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseGridTrackList()
             seenTrackSizeOrRepeatFunction = true;
         }
         // This will handle the trailing <ident>* in the grammar.
-        maybeParseGridLineNames(*m_valueList, *values);
+        if (!maybeParseGridLineNames(*m_valueList, *values))
+            return nullptr;
     }
 
     // We should have found a <track-size> or else it is not a valid <track-list>
@@ -3525,7 +3535,8 @@ bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
     arguments->next(); // Skip the comma.
 
     // Handle leading <ident>*.

[jfernandez, 2015/06/04 22:49:56]

Ditto.

-    maybeParseGridLineNames(*arguments, *repeatedValues);
+    if (!maybeParseGridLineNames(*arguments, *repeatedValues))
+        return false;
 
     size_t numberOfTracks = 0;
     while (arguments->current()) {
@@ -3537,7 +3548,8 @@ bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
         ++numberOfTracks;
 
         // This takes care of any trailing <ident>* in the grammar.
-        maybeParseGridLineNames(*arguments, *repeatedValues);
+        if (!maybeParseGridLineNames(*arguments, *repeatedValues))

[Julien - ping for review, 2015/06/04 23:17:58]

This function is not 'maybe' parsing anymore if we make all the caller reject
the value list when it returns false. It should be rename to parseGridLineNames.

I am concerned that we implemented too complicated a logic due to those maybe
function.

[svillar, 2015/06/05 07:25:43]

Sure, I also thought about that.
Not sure what you exactly mean. With the old parser it made total sense, because
we were getting a proper CSSParserValueList from the parser in the event of
having a valid ident list. Right now that has totally changed and we have to do
that parsing on our own. 

[Julien - ping for review, 2015/06/05 16:51:20]

The comment was basically that with the new parser, maybe a lot of the
complexity in the code is not needed anymore (as embodied by the maybe*
functions) and we could follow with a simplification. Again this was me thinking
out loud.

+            return false;
     }
 
     // We should have found at least one <track-size> or else it is not a valid <track-list>.