[css-align] Adapt the place-content shorthand to the new baseline syntax

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/parser/CSSPropertyParser.h"
 
 #include <memory>
 #include "core/StylePropertyShorthand.h"
 #include "core/css/CSSBasicShapeValues.h"
 #include "core/css/CSSBorderImage.h"
@@ skipping 3283 matching lines @@
               important);
   addProperty(CSSPropertyGridColumnGap, CSSPropertyGrid,
               *CSSInitialValue::create(), important);
   addProperty(CSSPropertyGridRowGap, CSSPropertyGrid,
               *CSSInitialValue::create(), important);
   return true;
 }
 
 static CSSValue* consumeSimplifiedContentPosition(CSSParserTokenRange& range) {
   CSSValueID id = range.peek().id();
-  if (identMatches<CSSValueNormal, CSSValueBaseline, CSSValueLastBaseline>(

[Bugs Nash, 2017/03/29 02:01:01]

should CSSValueLastBaseline be removed? it's not covered by
CSSPropertyAlignmentUtils::isBaselineKeyword

[jfernandez, 2017/03/29 10:43:10]

Actually not. It's debatable whether we need a different approach for the
general parsing, though. The issue is that the spec changed the baseline values
from "last-baseline" to "last baseline", so instead of having single CSSValue to
describe the baseline behavior we have two now. 

I thought that it could be a good idea to keep the CSSPrimitiveValue <==>
ComputedStyleConstant mapping, specially considering this change in the syntax
just affects to "last-baseline", since "first baseline" (new) is equivalent to
"baseline"

So, the answer is that we do need to keep CSSValueLastBaseline but it won 't be
allowed as an input, from the CSS parsing point of view. We will use is just for
generating the ComputedStyle representation.  

-          id)) {
+  if (identMatches<CSSValueNormal>(id)) {
     return CSSContentDistributionValue::create(
         CSSValueInvalid, range.consumeIncludingWhitespace().id(),
         CSSValueInvalid);
   }
-  if (identMatches<CSSValueSpaceBetween, CSSValueSpaceAround,
-                   CSSValueSpaceEvenly, CSSValueStretch>(id)) {
+
+  if (CSSPropertyAlignmentUtils::isBaselineKeyword(id)) {
+    CSSValue* baseline =
+        CSSPropertyAlignmentUtils::consumeBaselineKeyword(range);

[Bugs Nash, 2017/03/29 02:01:01]

consumeBaselineKeyword doesn't consume whitespace, will that be an issue here?

[jfernandez, 2017/03/29 10:43:10]

I'm not sure I understand this comment. consumeBaselineKeyword uses the
function CSSPropertyParserHelpers::consumeIdent(range), which as far as I
understand, it consumes whitespace. 

[Bugs Nash, 2017/03/29 21:52:11]

cool np

+    if (!baseline)
+      return nullptr;
+    return CSSContentDistributionValue::create(
+        CSSValueInvalid,
+        CSSPropertyAlignmentUtils::getBaselineKeyword(*baseline),
+        CSSValueInvalid);
+  }
+
+  if (CSSPropertyAlignmentUtils::isContentDistributionKeyword(id)) {
     return CSSContentDistributionValue::create(
         range.consumeIncludingWhitespace().id(), CSSValueInvalid,
         CSSValueInvalid);
   }
-  if (identMatches<CSSValueStart, CSSValueEnd, CSSValueCenter,
-                   CSSValueFlexStart, CSSValueFlexEnd, CSSValueLeft,
-                   CSSValueRight>(id)) {
+  if (CSSPropertyAlignmentUtils::isContentPositionKeyword(id)) {

[Bugs Nash, 2017/03/29 02:01:01]

this should be combined with the if block above that checks for CSSValueNormal

[jfernandez, 2017/03/29 10:43:10]

Done.

     return CSSContentDistributionValue::create(
         CSSValueInvalid, range.consumeIncludingWhitespace().id(),
         CSSValueInvalid);
   }
   return nullptr;
 }
 
 bool CSSPropertyParser::consumePlaceContentShorthand(bool important) {
   DCHECK(RuntimeEnabledFeatures::cssGridLayoutEnabled());
   DCHECK_EQ(shorthandForProperty(CSSPropertyPlaceContent).length(),
@@ skipping 277 matching lines @@
     case CSSPropertyGrid:
       return consumeGridShorthand(important);
     case CSSPropertyPlaceContent:
       return consumePlaceContentShorthand(important);
     default:
       return false;
   }
 }
 
 }  // namespace blink