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

Source/core/css/parser/CSSPropertyParser.cpp

 /*
  * Copyright (C) 2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2005 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Nicholas Shanks <webkit@nickshanks.com>
  * Copyright (C) 2008 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
  * Copyright (C) 2012 Intel Corporation. All rights reserved.
  *
@@ skipping 3210 matching lines @@
     size_t columnCount = 0;
     bool trailingIdentWasAdded = false;
     RefPtrWillBeRawPtr<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 (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))
             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) {
             RefPtrWillBeRawPtr<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.
         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> /]?
     if (templateColumns)
         addProperty(CSSPropertyGridTemplateColumns, templateColumns, important);
     else
         addProperty(CSSPropertyGridTemplateColumns,  cssValuePool().createIdentifierValue(CSSValueNone), important);
 
     // [<line-names>? <string> [<track-size> <line-names>]? ]+
     RefPtrWillBeRawPtr<CSSValue> templateAreas = CSSGridTemplateAreasValue::create(gridAreaMap, rowCount, columnCount);
@@ skipping 167 matching lines @@
     if (!isForwardSlashOperator(m_valueList->current()))
         return false;
 
     if (!m_valueList->next())
         return false;
 
     property = parseGridPosition();
     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()
 {
     ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
 
     CSSParserValue* value = m_valueList->current();
     if (value->id == CSSValueNone) {
         m_valueList->next();
         return cssValuePool().createIdentifierValue(CSSValueNone);
     }
 
     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()) {
         if (isForwardSlashOperator(currentValue))
             break;
         if (currentValue->unit == CSSParserValue::Function && currentValue->function->id == CSSValueRepeat) {
             if (!parseGridTrackRepeatFunction(*values))
                 return nullptr;
             seenTrackSizeOrRepeatFunction = true;
         } else {
             RefPtrWillBeRawPtr<CSSValue> value = parseGridTrackSize(*m_valueList);
             if (!value)
                 return nullptr;
             values->append(value);
             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>
     if (!seenTrackSizeOrRepeatFunction)
         return nullptr;
 
     return values;
 }
 
 bool CSSPropertyParser::parseGridTrackRepeatFunction(CSSValueList& list)
 {
     CSSParserValueList* arguments = m_valueList->current()->function->args.get();
     if (!arguments || arguments->size() < 3 || !validUnit(arguments->valueAt(0), FPositiveInteger) || !isComma(arguments->valueAt(1)))
         return false;
 
     ASSERT(arguments->valueAt(0)->fValue > 0);
     size_t repetitions = clampTo<size_t>(arguments->valueAt(0)->fValue, 0, kGridMaxTracks);
 
     RefPtrWillBeRawPtr<CSSValueList> repeatedValues = CSSValueList::createSpaceSeparated();
     arguments->next(); // Skip the repetition count.
     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()) {
         RefPtrWillBeRawPtr<CSSValue> trackSize = parseGridTrackSize(*arguments);
         if (!trackSize)
             return false;
 
         repeatedValues->append(trackSize);
         ++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>.
     if (!numberOfTracks)
         return false;
 
     // We clamp the number of repetitions to a multiple of the repeat() track list's size, while staying below the max
     // grid size.
     repetitions = std::min(repetitions, kGridMaxTracks / numberOfTracks);
 
@@ skipping 4834 matching lines @@
         }
     }
 
     if (!list->length())
         return nullptr;
 
     return list.release();
 }
 
 } // namespace blink