[css-grid] Implement grid gutters

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

Index: third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
diff --git a/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp b/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
index 36a928edc5b68376f7e4e5711e2774dee99ba3d3..3f6f65b003d6853c862f2aa7631da0534618de24 100644
--- a/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
@@ -1172,6 +1172,16 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
         parsedValue = parseGridPosition();
         break;
 
+    case CSSPropertyGridColumnGap:
+    case CSSPropertyGridRowGap:
+        ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+        validPrimitive = validUnit(value, FLength | FNonNeg);

[rune, 2015/10/05 12:23:39]

The current spec draft doesn't say it's limited to non-negative lengths, but it
probably should, like the spec for column-gap in multicol does.

[svillar, 2015/10/05 12:36:01]

True, I think it's just an overlook, actually the Mozilla guy that is going to
implement it in Gecko sent this
https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html to the list
asking for clarifications.

I think that the assumption I made is correct in any case.

[rune, 2015/10/05 12:47:10]

Yes, I just wanted to make sure the spec issue is addressed.

+        break;
+
+    case CSSPropertyGridGap:
+        ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
+        return parseGridGapShorthand(important);
+
     case CSSPropertyGridColumn:
     case CSSPropertyGridRow:
         ASSERT(RuntimeEnabledFeatures::cssGridLayoutEnabled());
@@ -3197,6 +3207,49 @@ bool CSSPropertyParser::parseGridItemPositionShorthand(CSSPropertyID shorthandId
     return true;
 }
 
+bool CSSPropertyParser::parseGridGapShorthand(bool important)

[fs, 2015/10/05 10:53:28]

Could you use parseShorthand instead?

[svillar, 2015/10/05 11:58:40]

I don't think so because although ideally the shorthand gets two values it might
get just one so we have to add that specific behavior. AFAIK that cannot be done
with parseShorthand...

[fs, 2015/10/05 12:01:54]

Right, I figured it might be the column-gap -> row-gap value expansion (didn't
remember if parseShorthand handled that or not.)

+{
+    ShorthandScope scope(this, CSSPropertyGridGap);
+    ASSERT(shorthandForProperty(CSSPropertyGridGap).length() == 2);
+
+    CSSParserValue* value = m_valueList->current();
+    if (!value)
+        return false;
+
+    if (value->id == CSSValueNormal) {
+        if (m_valueList->next())
+            return false;
+        addProperty(CSSPropertyGridColumnGap, cssValuePool().createImplicitInitialValue(), important);

[rune, 2015/10/05 12:23:39]

According to the spec, "normal" is not a valid value for any of the longhands.

Also, the spec doesn't say what normal mean, afaict.

[svillar, 2015/10/05 12:36:01]

That was specified in the previous draft, see
http://www.w3.org/TR/2015/WD-css-grid-1-20150806/#gutters. My first patch was
from 3 weeks ago when that was the latest version.

Anyway in the email I linked above there is another question for "normal" as
well so they'll clarify it for sure.

[rune, 2015/10/05 12:47:10]

Ah. So the property used to be shared with multicol. Now that explains the need
for 'normal' to mean something. Now that they're separate properties, and the
initial value is 0, and normal used to map to 0, I see no point in keeping
'normal'. I suggest that you just drop normal as a valid value for now and
re-introduce it if the spec for some reason re-introduces it instead of removing
it for the shorthand.

+        addProperty(CSSPropertyGridRowGap, cssValuePool().createImplicitInitialValue(), important);
+        return true;
+    }
+
+    if (!validUnit(value, FLength | FNonNeg))
+        return false;
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> columnGap = createPrimitiveNumericValue(value);
+
+    value = m_valueList->next();
+    if (!value) {
+        addProperty(CSSPropertyGridColumnGap, columnGap, important);
+        addProperty(CSSPropertyGridRowGap, columnGap, important);

[rune, 2015/10/05 12:23:39]

I think it this code might be easier to read if you have the addProperty calls
at the end and have two variables columnGap and rowGap which is set to the same
value if there's only one value in the declaration.

[svillar, 2015/10/05 12:36:01]

Acknowledged.

+        return true;
+    }
+
+    if (!validUnit(value, FLength | FNonNeg))
+        return false;
+
+    if (m_valueList->next())
+        return false;
+
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> rowGap = createPrimitiveNumericValue(value);
+
+    addProperty(CSSPropertyGridColumnGap, columnGap, important);
+    addProperty(CSSPropertyGridRowGap, rowGap, important);
+
+    return true;
+}
+
 bool CSSPropertyParser::parseGridTemplateRowsAndAreas(PassRefPtrWillBeRawPtr<CSSValue> templateColumns, bool important)
 {
     NamedGridAreaMap gridAreaMap;