Move viewport descriptor handling 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 e746827912449a437188a22e750c36422013e339..ef4ee86adb9914723c5c33e0755635b010057d39 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -47,7 +47,7 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
 
     if (ruleType == StyleRule::Viewport) {
         parseSuccess = (RuntimeEnabledFeatures::cssViewportEnabled() || isUASheetBehavior(context.mode()))
-            && parser.parseViewportProperty(resolvedProperty, important);
+            && parser.parseViewportDescriptor(resolvedProperty, important);
     } else if (ruleType == StyleRule::FontFace) {
         parseSuccess = parser.parseFontFaceDescriptor(resolvedProperty);
     } else {
@@ -293,6 +293,22 @@ static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeAngle(CSSParserTokenRang
     return nullptr;
 }
 
+static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumePercent(CSSParserTokenRange& range, ValueRange valueRange)

[Timothy Loh, 2015/10/07 00:18:33]

Probably makes more sense if this is before consumeLengthOrPercent 

[rwlbuis, 2015/10/07 23:08:53]

Done.

+{
+    const CSSParserToken& token = range.peek();
+    if (token.type() == PercentageToken) {
+        if (valueRange == ValueRangeNonNegative && token.numericValue() < 0)
+            return nullptr;
+        return cssValuePool().createValue(range.consumeIncludingWhitespace().numericValue(), CSSPrimitiveValue::UnitType::Percentage);
+    }
+    CalcParser calcParser(range, valueRange);
+    if (const CSSCalcValue* calculation = calcParser.value()) {
+        if (calculation->category() == CalcPercent)
+            return calcParser.consumeValue();
+    }
+    return nullptr;
+}
+
 static inline bool isCSSWideKeyword(const CSSValueID& id)
 {
     return id == CSSValueInitial || id == CSSValueInherit || id == CSSValueUnset || id == CSSValueDefault;
@@ -784,6 +800,104 @@ bool CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID propId)
     return true;
 }
 
+static PassRefPtrWillBeRawPtr<CSSValue> consumeViewportLonghand(CSSParserTokenRange& range, CSSPropertyID propId, CSSParserMode cssParserMode)

[Timothy Loh, 2015/10/07 00:18:33]

consumeSingleViewportDescriptor (or something better)? I don't like the name
longhand here because longhands are properties/descriptors that can be expanded
from shorthands.

[rwlbuis, 2015/10/07 23:08:53]

Done.

+{
+    RefPtrWillBeRawPtr<CSSValue> parsedValue = nullptr;
+    CSSValueID id = range.peek().id();
+    switch (propId) {
+    case CSSPropertyMinWidth:
+    case CSSPropertyMaxWidth:
+    case CSSPropertyMinHeight:
+    case CSSPropertyMaxHeight:
+        if (id == CSSValueAuto || id == CSSValueInternalExtendToZoom)
+            parsedValue = consumeIdent(range);

[Timothy Loh, 2015/10/07 00:18:33]

return consumeIdent(range); and so on for the rest of the function?

[rwlbuis, 2015/10/07 23:08:53]

Yeah much better :) Done.

+        else
+            parsedValue = consumeLengthOrPercent(range, cssParserMode, ValueRangeNonNegative);
+        break;
+    case CSSPropertyMinZoom:
+    case CSSPropertyMaxZoom:
+    case CSSPropertyZoom:
+        if (id == CSSValueAuto) {
+            parsedValue = consumeIdent(range);
+        } else {
+            parsedValue = consumeNumber(range, ValueRangeNonNegative);
+            if (!parsedValue)
+                parsedValue = consumePercent(range, ValueRangeNonNegative);
+        }
+        break;
+    case CSSPropertyUserZoom:
+        if (id == CSSValueZoom || id == CSSValueFixed)
+            parsedValue = consumeIdent(range);
+        break;
+    case CSSPropertyOrientation:
+        if (id == CSSValueAuto || id == CSSValuePortrait || id == CSSValueLandscape)
+            parsedValue = consumeIdent(range);
+    default:
+        break;
+    }
+
+    return parsedValue;
+}
+
+bool CSSPropertyParser::parseViewportDescriptor(CSSPropertyID propId, bool important)
+{
+    ASSERT(RuntimeEnabledFeatures::cssViewportEnabled() || isUASheetBehavior(m_context.mode()));
+
+    m_range.consumeWhitespace();
+    RefPtrWillBeRawPtr<CSSValue> parsedValue = nullptr;
+
+    switch (propId) {
+    case CSSPropertyWidth: {

[Timothy Loh, 2015/10/07 00:18:33]

I think there's an assertion in addProperty that can be updated or removed

[rwlbuis, 2015/10/07 23:08:53]

Done.

+        RefPtrWillBeRawPtr<CSSValue> minWidth = consumeViewportLonghand(m_range, CSSPropertyMinWidth, m_context.mode());
+        if (!minWidth)
+            return false;
+        addProperty(CSSPropertyMinWidth, minWidth, important);

[Timothy Loh, 2015/10/07 00:18:33]

I think this would be slightly nicer as:

RefPtrWillBeRawPtr<CSSValue> minWidth = consume..;
if (!minWidth)
    return false;

RefPtrWillBeRawPtr<CSSValue> maxWidth = minWidth;
if (!m_range.atEnd())
    maxWidth = consume...
if (!maxWidth || !m_range.atEnd())
    return false;

addProperty(.., minWidth, ..);
addProperty(.., maxWidth, ..);

[rwlbuis, 2015/10/07 23:08:53]

Done.

+        if (m_range.atEnd()) {
+            addProperty(CSSPropertyMaxWidth, minWidth, important);
+            return true;
+        }
+        RefPtrWillBeRawPtr<CSSValue> maxWidth = consumeViewportLonghand(m_range, CSSPropertyMaxWidth, m_context.mode());
+        if (!maxWidth || !m_range.atEnd())
+            return false;
+        addProperty(CSSPropertyMaxWidth, maxWidth, important);
+        return true;
+    }
+    case CSSPropertyHeight: {
+        RefPtrWillBeRawPtr<CSSValue> minHeight = consumeViewportLonghand(m_range, CSSPropertyMinHeight, m_context.mode());
+        if (!minHeight)
+            return false;
+        addProperty(CSSPropertyMinHeight, minHeight, important);
+        if (m_range.atEnd()) {
+            addProperty(CSSPropertyMaxHeight, minHeight, important);
+            return true;
+        }
+        RefPtrWillBeRawPtr<CSSValue> maxHeight = consumeViewportLonghand(m_range, CSSPropertyMaxHeight, m_context.mode());
+        if (!maxHeight || !m_range.atEnd())
+            return false;
+        addProperty(CSSPropertyMaxHeight, maxHeight, important);
+        return true;
+    }
+    case CSSPropertyMinWidth:
+    case CSSPropertyMaxWidth:
+    case CSSPropertyMinHeight:
+    case CSSPropertyMaxHeight:
+    case CSSPropertyMinZoom:
+    case CSSPropertyMaxZoom:
+    case CSSPropertyZoom:
+    case CSSPropertyUserZoom:
+    case CSSPropertyOrientation:
+    default:

[Timothy Loh, 2015/10/07 00:18:33]

How about return false in this default and ASSERT_NOT_REACHED for default in the
other function?

[rwlbuis, 2015/10/07 23:08:52]

Done.

+        parsedValue = consumeViewportLonghand(m_range, propId, m_context.mode());
+        break;
+    }
+
+    if (!parsedValue || !m_range.atEnd())
+        return false;
+
+    addProperty(propId, parsedValue.release(), important);
+    return true;
+}
+
 bool CSSPropertyParser::parseShorthand(CSSPropertyID propId, bool important)
 {
     m_range.consumeWhitespace();