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

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 6fe47f228470057c49d82480d02fdb3d9e35ff00..a74e71a1f3c502bd2b36c779cdbc3948500e91ff 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -3301,21 +3301,29 @@ bool CSSPropertyParser::consumeGridShorthand(bool important) {
 
 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);