Move the grid shorthand into CSSPropertyParser

third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp

Index: third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
index 419adc0c8689172fb5591fedd15974a368a99914..fe227f6f1278f652b8914c306ca0c75e88a9ed2c 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -18,6 +18,7 @@
 #include "core/css/CSSGradientValue.h"
 #include "core/css/CSSGridAutoRepeatValue.h"
 #include "core/css/CSSGridLineNamesValue.h"
+#include "core/css/CSSGridTemplateAreasValue.h"
 #include "core/css/CSSImageSetValue.h"
 #include "core/css/CSSPaintValue.h"
 #include "core/css/CSSPathValue.h"
@@ -59,6 +60,39 @@ CSSPropertyParser::CSSPropertyParser(const CSSParserTokenRange& range,
     m_range.consumeWhitespace();
 }
 
+void CSSPropertyParser::addProperty(CSSPropertyID propId, CSSValue* value, bool important, bool implicit)

[Timothy Loh, 2016/04/13 06:09:41]

since you're moving this, can you rename propId to property

[rwlbuis, 2016/04/13 22:16:29]

Done.

+{
+    ASSERT(!isPropertyAlias(propId));
+
+    int shorthandIndex = 0;
+    bool setFromShorthand = false;
+
+    if (m_currentShorthand) {
+        Vector<StylePropertyShorthand, 4> shorthands;
+        getMatchingShorthandsForLonghand(propId, &shorthands);
+        setFromShorthand = true;
+        if (shorthands.size() > 1)
+            shorthandIndex = indexOfShorthandForLonghand(m_currentShorthand, shorthands);
+    }
+
+    m_parsedProperties->append(CSSProperty(propId, value, important, setFromShorthand, shorthandIndex, implicit));
+}
+
+void CSSPropertyParser::addExpandedPropertyForValue(CSSPropertyID propId, CSSValue* value, bool important)

[Timothy Loh, 2016/04/13 06:09:42]

propId -> property

[rwlbuis, 2016/04/13 22:16:29]

Done.

+{
+    const StylePropertyShorthand& shorthand = shorthandForProperty(propId);
+    unsigned shorthandLength = shorthand.length();
+    if (!shorthandLength) {
+        addProperty(propId, value, important);
+        return;
+    }
+
+    ShorthandScope scope(this, propId);
+    const CSSPropertyID* longhands = shorthand.properties();
+    for (unsigned i = 0; i < shorthandLength; ++i)
+        addProperty(longhands[i], value, important);
+}
+
 static bool hasInvalidNumericValues(const CSSParserTokenRange& range)
 {
     for (const CSSParserToken& token : range) {
@@ -2923,7 +2957,7 @@ static CSSValue* consumeBackgroundSize(CSSPropertyID unresolvedProperty, CSSPars
     return CSSValuePair::create(horizontal, vertical, CSSValuePair::KeepIdenticalValues);
 }
 
-CSSValueList* consumeGridAutoFlow(CSSParserTokenRange& range)
+static CSSValueList* consumeGridAutoFlow(CSSParserTokenRange& range)
 {
     CSSPrimitiveValue* rowOrColumnValue = consumeIdent<CSSValueRow, CSSValueColumn>(range);
     CSSPrimitiveValue* denseAlgorithm = consumeIdent<CSSValueDense>(range);
@@ -3102,6 +3136,124 @@ static CSSValue* consumeGridLine(CSSParserTokenRange& range)
     return values;
 }
 
+static bool allTracksAreFixedSized(CSSValueList& valueList)
+{
+    for (auto value : valueList) {

[Timothy Loh, 2016/04/13 06:09:42]

does it work to write

for (CSSValue* value : valueList)

?

[rwlbuis, 2016/04/13 22:16:29]

Done.

+        if (value->isGridLineNamesValue())
+            continue;
+        // The auto-repeat value holds a <fixed-size> = <fixed-breadth> | minmax( <fixed-breadth>, <track-breadth> )
+        if (value->isGridAutoRepeatValue()) {
+            if (!allTracksAreFixedSized(toCSSValueList(*value)))
+                return false;
+            continue;
+        }
+        ASSERT(value->isPrimitiveValue() || (value->isFunctionValue() && toCSSFunctionValue(*value).item(0)));

[Timothy Loh, 2016/04/13 06:09:42]

I guess this assertion is unnecessary since the conversions below will check the
same

[rwlbuis, 2016/04/13 22:16:29]

It is not 100% covered just by that code. In theory the value could be something
else than a primitive of function value. In reality, the way
consumeGridTrackList works it can't be something else. I don't feel too strongly
about this, can remove it if you want, but it is not completely useless just
because of the ternary below.

[Timothy Loh, 2016/04/14 02:04:12]

Not sure I follow. Ternary below says:

non-null primitive value (case 1)
OR
function value, first value is non-null primitive value (case 2)

If it's neither a primitive nor function value, then toCSSFunctionValue will hit
an assertion. So unless I'm misunderstanding, I think it'd be better to not have
the assertion because it isn't useful.

+        const CSSPrimitiveValue& primitiveValue = value->isPrimitiveValue()
+            ? toCSSPrimitiveValue(*value)
+            : toCSSPrimitiveValue(*toCSSFunctionValue(*value).item(0));
+        CSSValueID valueID = primitiveValue.getValueID();
+        if (valueID == CSSValueMinContent || valueID == CSSValueMaxContent || valueID == CSSValueAuto || primitiveValue.isFlex())
+            return false;
+    }
+    return true;
+}
+
+static Vector<String> parseGridTemplateAreasColumnNames(const String& gridRowNames)
+{
+    ASSERT(!gridRowNames.isEmpty());
+    Vector<String> columnNames;
+    // Using StringImpl to avoid checks and indirection in every call to String::operator[].
+    StringImpl& text = *gridRowNames.impl();
+
+    StringBuilder areaName;
+    for (unsigned i = 0; i < text.length(); ++i) {
+        if (text[i] == ' ') {

[Timothy Loh, 2016/04/13 06:09:42]

Needs a TODO that this whitespace check is missing \n and \t

https://drafts.csswg.org/css-grid/#valdef-grid-template-areas-string
https://drafts.csswg.org/css-syntax-3/#whitespace

[rwlbuis, 2016/04/13 22:16:29]

Done.

+            if (!areaName.isEmpty()) {
+                columnNames.append(areaName.toString());
+                areaName.clear();
+            }
+            continue;
+        }
+        if (text[i] == '.') {
+            if (areaName == ".")
+                continue;
+            if (!areaName.isEmpty()) {
+                columnNames.append(areaName.toString());
+                areaName.clear();
+            }
+        } else {

[Timothy Loh, 2016/04/13 06:09:42]

Needs a TODO to handle trash tokens (should only allow name code points here)

[rwlbuis, 2016/04/13 22:16:28]

Done.

+            if (areaName == ".") {
+                columnNames.append(areaName.toString());
+                areaName.clear();
+            }
+        }
+
+        areaName.append(text[i]);
+    }
+
+    if (!areaName.isEmpty())
+        columnNames.append(areaName.toString());
+
+    return columnNames;
+}
+
+static bool parseGridTemplateAreasRow(const String& gridRowNames, NamedGridAreaMap& gridAreaMap, const size_t rowCount, size_t& columnCount)
+{
+    if (gridRowNames.isEmpty() || gridRowNames.containsOnlyWhitespace())

[Timothy Loh, 2016/04/13 06:09:42]

I feel like I had this discussion before... but isEmpty is redundant here
because it vacuously contains only whitespace.

Anyway I think this is a strange way to handle this error condition. I think
it'd be easiest if parseGridTemplateAreasColumnNames just returned an empty
vector as its failure case (when we support rejecting trash characters in the
string), and if we do that then there's no point in explicitly checking for
whitespace-only up-front (and it doesn't really make sense as an optimisation
because authors shouldn't be writing these invalid declarations in the first
place). But let's make that a separate change.

[rwlbuis, 2016/04/13 22:16:29]

Acknowledged.

+        return false;
+
+    Vector<String> columnNames = parseGridTemplateAreasColumnNames(gridRowNames);
+    if (!columnCount) {

[Timothy Loh, 2016/04/13 06:09:42]

IMO more obvious if you write if (rowCount == 0)

[rwlbuis, 2016/04/13 22:16:29]

You meant columnCount == 0, right? Done.

[Timothy Loh, 2016/04/14 02:04:12]

Nope, I meant rowCount. I think it makes more sense to check (rowCount == 0),
which is "this is the first row", rather than (columnCount == 0), which is "we
don't know how many columns yet, which we signal by setting it to 0".

+        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.

[Timothy Loh, 2016/04/13 06:09:41]

invalid is -> invalid if

[rwlbuis, 2016/04/13 22:16:29]

Done.

+        return false;
+    }
+
+    for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {

[Timothy Loh, 2016/04/13 06:09:41]

currentColumn

[rwlbuis, 2016/04/13 22:16:29]

Done.

+        const String& gridAreaName = columnNames[currentCol];
+
+        // 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.

[Timothy Loh, 2016/04/13 06:09:42]

I'm not really sure what this comment is trying to say. Maybe better to remove
it. Grid cells with the same name form a single grid area.

[rwlbuis, 2016/04/13 22:16:29]

Done.

+        size_t lookAheadCol;

[Timothy Loh, 2016/04/13 06:09:42]

imo more readable as

size_t lookAheadColumn = currentColumn + 1;
while (lookAheadColumn < columnCount && columnNames[lookAheadColumn] !=
gridAreaName)
    lookAheadColumn++;

[rwlbuis, 2016/04/13 22:16:29]

Done.

+        for (lookAheadCol = currentCol + 1; lookAheadCol < columnCount; ++lookAheadCol) {
+            if (columnNames[lookAheadCol] != gridAreaName)
+                break;
+        }
+
+        NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName);
+        if (gridAreaIt == gridAreaMap.end()) {
+            gridAreaMap.add(gridAreaName, GridArea(GridSpan::translatedDefiniteGridSpan(rowCount, rowCount + 1), GridSpan::translatedDefiniteGridSpan(currentCol, lookAheadCol)));
+        } else {
+            GridArea& gridArea = 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 != gridArea.rows.endLine())
+                return false;
+
+            // 2. The new area starts at the same position as the previously parsed area.
+            if (currentCol != gridArea.columns.startLine())
+                return false;
+
+            // 3. The new area ends at the same position as the previously parsed area.
+            if (lookAheadCol != gridArea.columns.endLine())
+                return false;
+
+            gridArea.rows = GridSpan::translatedDefiniteGridSpan(gridArea.rows.startLine(), gridArea.rows.endLine() + 1);
+        }
+        currentCol = lookAheadCol - 1;
+    }
+
+    return true;
+}
+
+enum TrackSizeRestriction { FixedSizeOnly, AllowAll };
+
 static CSSPrimitiveValue* consumeGridBreadth(CSSParserTokenRange& range, CSSParserMode cssParserMode, TrackSizeRestriction restriction = AllowAll)
 {
     if (restriction == AllowAll) {
@@ -3118,7 +3270,7 @@ static CSSPrimitiveValue* consumeGridBreadth(CSSParserTokenRange& range, CSSPars
 }
 
 // TODO(rob.buis): This needs a bool parameter so we can disallow <auto-track-list> for the grid shorthand.
-CSSValue* consumeGridTrackSize(CSSParserTokenRange& range, CSSParserMode cssParserMode, TrackSizeRestriction restriction)
+static CSSValue* consumeGridTrackSize(CSSParserTokenRange& range, CSSParserMode cssParserMode, TrackSizeRestriction restriction = AllowAll)
 {
     const CSSParserToken& token = range.peek();
     if (restriction == AllowAll && identMatches<CSSValueAuto>(token.id()))
@@ -4563,6 +4715,71 @@ bool CSSPropertyParser::consumeGridTemplateShorthand(bool important)
     return consumeGridTemplateRowsAndAreasAndColumns(important);
 }
 
+bool CSSPropertyParser::consumeGridShorthand(bool important)
+{
+    ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+    ASSERT(shorthandForProperty(CSSPropertyGrid).length() == 8);
+
+    CSSParserTokenRange rangeCopy = m_range;
+
+    // 1- <grid-template>
+    if (consumeGridTemplateShorthand(important)) {
+        // It can only be specified the explicit or the implicit grid properties in a single grid declaration.
+        // The sub-properties not specified are set to their initial value, as normal for shorthands.
+        addProperty(CSSPropertyGridAutoFlow, cssValuePool().createImplicitInitialValue(), important);
+        addProperty(CSSPropertyGridAutoColumns, cssValuePool().createImplicitInitialValue(), important);
+        addProperty(CSSPropertyGridAutoRows, cssValuePool().createImplicitInitialValue(), important);
+        addProperty(CSSPropertyGridColumnGap, cssValuePool().createImplicitInitialValue(), important);
+        addProperty(CSSPropertyGridRowGap, cssValuePool().createImplicitInitialValue(), important);
+        return true;
+    }
+
+    m_range = rangeCopy;
+
+    // 2- <grid-auto-flow> [ <grid-auto-rows> [ / <grid-auto-columns> ]? ]
+    CSSValueList* gridAutoFlow = consumeGridAutoFlow(m_range);
+    if (!gridAutoFlow)
+        return false;
+
+    CSSValue* autoColumnsValue = nullptr;
+    CSSValue* autoRowsValue = nullptr;
+
+    if (!m_range.atEnd()) {
+        autoRowsValue = consumeGridTrackSize(m_range, m_context.mode());
+        if (!autoRowsValue)
+            return false;
+        if (consumeSlashIncludingWhitespace(m_range)) {
+            autoColumnsValue = consumeGridTrackSize(m_range, m_context.mode());
+            if (!autoColumnsValue)
+                return false;
+        }
+        if (!m_range.atEnd())
+            return false;
+    } else {
+        // Other omitted values are set to their initial values.
+        autoColumnsValue = cssValuePool().createImplicitInitialValue();
+        autoRowsValue = cssValuePool().createImplicitInitialValue();
+    }
+
+    // if <grid-auto-columns> value is omitted, it is set to the value specified for grid-auto-rows.
+    if (!autoColumnsValue)
+        autoColumnsValue = autoRowsValue;
+
+    addProperty(CSSPropertyGridAutoFlow, gridAutoFlow, important);
+    addProperty(CSSPropertyGridAutoColumns, autoColumnsValue, important);
+    addProperty(CSSPropertyGridAutoRows, autoRowsValue, important);
+
+    // It can only be specified the explicit or the implicit grid properties in a single grid declaration.
+    // The sub-properties not specified are set to their initial value, as normal for shorthands.
+    addProperty(CSSPropertyGridTemplateColumns, cssValuePool().createImplicitInitialValue(), important);

[Timothy Loh, 2016/04/13 06:09:42]

I think it makes sense for the longhands to always get added in the same order,
so lets move the grid-template-* longhands before the grid-auto-* longhands to
match the above order and the CSSProperties.in order.

[rwlbuis, 2016/04/13 22:16:29]

Done.

+    addProperty(CSSPropertyGridTemplateRows, cssValuePool().createImplicitInitialValue(), important);
+    addProperty(CSSPropertyGridTemplateAreas, cssValuePool().createImplicitInitialValue(), important);
+    addProperty(CSSPropertyGridColumnGap, cssValuePool().createImplicitInitialValue(), important);
+    addProperty(CSSPropertyGridRowGap, cssValuePool().createImplicitInitialValue(), important);
+
+    return true;
+}
+
 bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool important)
 {
     CSSPropertyID property = resolveCSSPropertyID(unresolvedProperty);
@@ -4747,13 +4964,11 @@ bool CSSPropertyParser::parseShorthand(CSSPropertyID unresolvedProperty, bool im
         return consumeGridAreaShorthand(important);
     case CSSPropertyGridTemplate:
         return consumeGridTemplateShorthand(important);
+    case CSSPropertyGrid:
+        return consumeGridShorthand(important);
     default:
         m_currentShorthand = oldShorthand;
-        CSSParserValueList valueList(m_range);
-        if (!valueList.size())
-            return false;
-        m_valueList = &valueList;
-        return legacyParseShorthand(unresolvedProperty, important);
+        return false;
     }
 }