Changeset 272121 in webkit

Timestamp:

Jan 30, 2021, 2:33:00 PM (6 years ago)

Author:

weinig@apple.com

Message:

Additional cleanup of CSSPropertyParserHelpers
​https://bugs.webkit.org/show_bug.cgi?id=221174

Reviewed by Simon Fraser.

• Added "raw" versions of consumeInteger and consumePositiveInteger.
• Converted consumePosition and consumeOneOrTwoValuedPosition to use an Optional return value rather than out parameters, using a new PositionCoordinates helper struct.
• Added consumeNumberOrPercentDividedBy100Raw helper for common patter of supporting eiter a number, or a percentage normalized by dividing it by 100.

• css/MediaQueryExpression.cpp:
• css/parser/CSSPropertyParser.cpp:
• css/parser/CSSPropertyParserHelpers.cpp:
• css/parser/CSSPropertyParserHelpers.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• css/MediaQueryExpression.cpp (modified) (1 diff)
• css/parser/CSSPropertyParser.cpp (modified) (14 diffs)
• css/parser/CSSPropertyParserHelpers.cpp (modified) (21 diffs)
• css/parser/CSSPropertyParserHelpers.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-01-30  Sam Weinig  <weinig@apple.com>
+
+        Additional cleanup of CSSPropertyParserHelpers
+        https://bugs.webkit.org/show_bug.cgi?id=221174
+
+        Reviewed by Simon Fraser.
+
+        - Added "raw" versions of consumeInteger and consumePositiveInteger.
+        - Converted consumePosition and consumeOneOrTwoValuedPosition to use 
+          an Optional return value rather than out parameters, using a new
+          PositionCoordinates helper struct.
+        - Added consumeNumberOrPercentDividedBy100Raw helper for common patter
+          of supporting eiter a number, or a percentage normalized by dividing
+          it by 100.
+
+        * css/MediaQueryExpression.cpp:
+        * css/parser/CSSPropertyParser.cpp:
+        * css/parser/CSSPropertyParserHelpers.cpp:
+        * css/parser/CSSPropertyParserHelpers.h:
+
 2021-01-30  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/css/MediaQueryExpression.cpp

@@ -221 +221 @@
         if (!CSSPropertyParserHelpers::consumeSlashIncludingWhitespace(range))
             return;
-        RefPtr<CSSPrimitiveValue> denominatorValue = CSSPropertyParserHelpers::consumePositiveInteger(range);
+        auto denominatorValue = CSSPropertyParserHelpers::consumePositiveIntegerRaw(range);
         if (!denominatorValue)
             return;
 
         unsigned numerator = clampTo<unsigned>(firstValue->doubleValue());
-        unsigned denominator = clampTo<unsigned>(denominatorValue->doubleValue());
-        m_value = CSSAspectRatioValue::create(numerator, denominator);
+        m_value = CSSAspectRatioValue::create(numerator, *denominatorValue);
         m_isValid = true;
         return;

trunk/Source/WebCore/css/parser/CSSPropertyParser.cpp

@@ -371 +371 @@
 bool CSSPropertyParser::consumeTransformOrigin(bool important)
 {
-    RefPtr<CSSPrimitiveValue> resultX;
-    RefPtr<CSSPrimitiveValue> resultY;
-    if (consumeOneOrTwoValuedPosition(m_range, m_context.mode, UnitlessQuirk::Forbid, resultX, resultY)) {
+    if (auto resultXY = consumeOneOrTwoValuedPositionCoordinates(m_range, m_context.mode, UnitlessQuirk::Forbid)) {
         m_range.consumeWhitespace();
         bool atEnd = m_range.atEnd();
-        RefPtr<CSSPrimitiveValue> resultZ = consumeLength(m_range, m_context.mode, ValueRangeAll);
+        auto resultZ = consumeLength(m_range, m_context.mode, ValueRangeAll);
         bool hasZ = resultZ;
         if (!hasZ && !atEnd)
             return false;
-        addProperty(CSSPropertyTransformOriginX, CSSPropertyTransformOrigin, resultX.releaseNonNull(), important);
-        addProperty(CSSPropertyTransformOriginY, CSSPropertyTransformOrigin, resultY.releaseNonNull(), important);
+        addProperty(CSSPropertyTransformOriginX, CSSPropertyTransformOrigin, WTFMove(resultXY->x), important);
+        addProperty(CSSPropertyTransformOriginY, CSSPropertyTransformOrigin, WTFMove(resultXY->y), important);
         addProperty(CSSPropertyTransformOriginZ, CSSPropertyTransformOrigin, resultZ ? resultZ.releaseNonNull() : CSSValuePool::singleton().createValue(0, CSSUnitType::CSS_PX), important, !hasZ);
         
@@ -391 +389 @@
 bool CSSPropertyParser::consumePerspectiveOrigin(bool important)
 {
-    RefPtr<CSSPrimitiveValue> resultX;
-    RefPtr<CSSPrimitiveValue> resultY;
-    if (consumePosition(m_range, m_context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position, resultX, resultY)) {
-        addProperty(CSSPropertyPerspectiveOriginX, CSSPropertyPerspectiveOrigin, resultX.releaseNonNull(), important);
-        addProperty(CSSPropertyPerspectiveOriginY, CSSPropertyPerspectiveOrigin, resultY.releaseNonNull(), important);
+    if (auto result = consumePositionCoordinates(m_range, m_context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position)) {
+        addProperty(CSSPropertyPerspectiveOriginX, CSSPropertyPerspectiveOrigin, WTFMove(result->x), important);
+        addProperty(CSSPropertyPerspectiveOriginY, CSSPropertyPerspectiveOrigin, WTFMove(result->y), important);
         return true;
     }
@@ -475 +471 @@
     if (!range.atEnd() && range.peek().type() != CommaToken) {
         // Feature tag values could follow: <integer> | on | off
-        if (auto primitiveValue = consumeInteger(range, 0))
-            tagValue = primitiveValue->intValue();
+        if (auto integer = consumeIntegerRaw(range, 0))
+            tagValue = *integer;
         else if (range.peek().id() == CSSValueOn || range.peek().id() == CSSValueOff)
             tagValue = range.consumeIncludingWhitespace().id() == CSSValueOn;
@@ -1149 +1145 @@
             return nullptr;
         int i = defaultValue;
-        if (RefPtr<CSSPrimitiveValue> counterValue = consumeInteger(range))
-            i = counterValue->intValue();
+        if (auto counterValue = consumeIntegerRaw(range))
+            i = *counterValue;
         list->append(createPrimitiveValuePair(counterName.releaseNonNull(), CSSPrimitiveValue::create(i, CSSUnitType::CSS_NUMBER), Pair::IdenticalValueEncoding::Coalesce));
     } while (!range.atEnd());
@@ -1333 +1329 @@
 static RefPtr<CSSPrimitiveValue> consumeLineClamp(CSSParserTokenRange& range)
 {
-    RefPtr<CSSPrimitiveValue> clampValue = consumePercent(range, ValueRangeNonNegative);
-    if (clampValue)
+    if (auto clampValue = consumePercent(range, ValueRangeNonNegative))
         return clampValue;
     // When specifying number of lines, don't allow 0 as a valid value.
@@ -1455 +1450 @@
     CSSParserTokenRange args = consumeFunction(rangeCopy);
     
-    RefPtr<CSSPrimitiveValue> stepsValue = consumePositiveInteger(args);
+    auto stepsValue = consumePositiveIntegerRaw(args);
     if (!stepsValue)
         return nullptr;
@@ -1494 +1489 @@
         return nullptr;
 
-    auto steps = stepsValue->intValue();
-    if (steps <= 1 && stepPosition == StepsTimingFunction::StepPosition::JumpNone)
-        return nullptr;
-    
+    if (*stepsValue == 1 && stepPosition == StepsTimingFunction::StepPosition::JumpNone)
+        return nullptr;
+
     range = rangeCopy;
-    return CSSStepsTimingFunctionValue::create(steps, stepPosition);
+    return CSSStepsTimingFunctionValue::create(*stepsValue, stepPosition);
 }
 
@@ -2606 +2600 @@
         shape->setRadius(radius.releaseNonNull());
     if (consumeIdent<CSSValueAt>(args)) {
-        RefPtr<CSSPrimitiveValue> centerX;
-        RefPtr<CSSPrimitiveValue> centerY;
-        if (!consumePosition(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position, centerX, centerY))
-            return nullptr;
-        shape->setCenterX(centerX.releaseNonNull());
-        shape->setCenterY(centerY.releaseNonNull());
+        auto centerCoordinates = consumePositionCoordinates(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position);
+        if (!centerCoordinates)
+            return nullptr;
+        shape->setCenterX(WTFMove(centerCoordinates->x));
+        shape->setCenterY(WTFMove(centerCoordinates->y));
     }
     return shape;
@@ -2629 +2622 @@
     }
     if (consumeIdent<CSSValueAt>(args)) {
-        RefPtr<CSSPrimitiveValue> centerX;
-        RefPtr<CSSPrimitiveValue> centerY;
-        if (!consumePosition(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position, centerX, centerY))
-            return nullptr;
-        shape->setCenterX(centerX.releaseNonNull());
-        shape->setCenterY(centerY.releaseNonNull());
+        auto centerCoordinates = consumePositionCoordinates(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position);
+        if (!centerCoordinates)
+            return nullptr;
+        shape->setCenterX(WTFMove(centerCoordinates->x));
+        shape->setCenterY(WTFMove(centerCoordinates->y));
     }
     return shape;
@@ -3603 +3595 @@
         repeatedValues = CSSGridAutoRepeatValue::create(args.consumeIncludingWhitespace().id());
     else {
-        // FIXME: a consumeIntegerRaw would be more efficient here.
-        RefPtr<CSSPrimitiveValue> repetition = consumePositiveInteger(args);
+        auto repetition = consumePositiveIntegerRaw(args);
         if (!repetition)
             return false;
-        repetitions = clampTo<size_t>(repetition->doubleValue(), 0, GridPosition::max());
+        repetitions = clampTo<size_t>(static_cast<size_t>(*repetition), 0, GridPosition::max());
         repeatedValues = CSSValueList::createSpaceSeparated();
     }
@@ -4364 +4355 @@
     case CSSPropertyFloodOpacity:
     case CSSPropertyOpacity:
-    {
-        RefPtr<CSSValue> parsedValue = consumeNumber(m_range, ValueRangeAll);
-        if (parsedValue)
+        if (auto parsedValue = consumeNumber(m_range, ValueRangeAll))
             return parsedValue;
         return consumePercent(m_range, ValueRangeAll);
-    }
     case CSSPropertyWebkitBoxFlex:
         return consumeNumber(m_range, ValueRangeAll);
@@ -5036 +5024 @@
         unsigned index = 0;
         while (!m_range.atEnd() && index++ < 3) {
-            if (auto num = consumeNumberRaw(m_range)) {
-                if (*num < 0)
+            if (auto number = consumeNumberRaw(m_range)) {
+                if (*number < 0)
                     return false;
                 if (flexGrow == unsetValue)
-                    flexGrow = *num;
+                    flexGrow = *number;
                 else if (flexShrink == unsetValue)
-                    flexShrink = *num;
-                else if (!*num) // flex only allows a basis of 0 (sans units) if flex-grow and flex-shrink values have already been set.
+                    flexShrink = *number;
+                else if (!*number) // flex only allows a basis of 0 (sans units) if flex-grow and flex-shrink values have already been set.
                     flexBasis = CSSPrimitiveValue::create(0, CSSUnitType::CSS_PX);
                 else
@@ -5284 +5272 @@
 {
     do {
-        RefPtr<CSSPrimitiveValue> positionX;
-        RefPtr<CSSPrimitiveValue> positionY;
-        if (!consumePosition(range, context.mode, unitless, PositionSyntax::BackgroundPosition, positionX, positionY))
+        auto position = consumePositionCoordinates(range, context.mode, unitless, PositionSyntax::BackgroundPosition);
+        if (!position)
             return false;
-        addBackgroundValue(resultX, positionX.releaseNonNull());
-        addBackgroundValue(resultY, positionY.releaseNonNull());
+        addBackgroundValue(resultX, WTFMove(position->x));
+        addBackgroundValue(resultY, WTFMove(position->y));
     } while (consumeCommaIncludingWhitespace(range));
     return true;
@@ -5362 +5349 @@
                 } else if (property == CSSPropertyBackgroundPositionX || property == CSSPropertyWebkitMaskPositionX) {
                     CSSParserTokenRange rangeCopy = m_range;
-                    RefPtr<CSSPrimitiveValue> primitiveValue;
-                    RefPtr<CSSPrimitiveValue> primitiveValueY;
-                    if (!consumePosition(rangeCopy, m_context.mode, UnitlessQuirk::Forbid, PositionSyntax::BackgroundPosition, primitiveValue, primitiveValueY))
+                    auto position = consumePositionCoordinates(rangeCopy, m_context.mode, UnitlessQuirk::Forbid, PositionSyntax::BackgroundPosition);
+                    if (!position)
                         continue;
-                    value = primitiveValue;
-                    valueY = primitiveValueY;
+                    value = WTFMove(position->x);
+                    valueY = WTFMove(position->y);
                     m_range = rangeCopy;
                 } else if (property == CSSPropertyBackgroundSize || property == CSSPropertyWebkitMaskSize) {

trunk/Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp

@@ -84 +84 @@
 }
 
+static Optional<double> consumeNumberOrPercentDividedBy100Raw(CSSParserTokenRange& range, ValueRange valueRange = ValueRangeAll)
+{
+    if (auto percent = consumePercentRaw(range, valueRange))
+        return *percent / 100.0;
+    if (auto number = consumeNumberRaw(range, valueRange))
+        return *number;
+    return WTF::nullopt;
+}
+
 // FIXME: consider pulling in the parsing logic from CSSCalculationValue.cpp.
 class CalcParser {
@@ -112 +121 @@
             return nullptr;
         m_sourceRange = m_range;
-        double value = std::max(m_calcValue->doubleValue(), minimumValue);
-        value = std::round(value);
-        return CSSValuePool::singleton().createValue(value, CSSUnitType::CSS_NUMBER);
+        return CSSValuePool::singleton().createValue(std::round(std::max(m_calcValue->doubleValue(), minimumValue)), CSSUnitType::CSS_NUMBER);
+    }
+
+    template<typename IntType> Optional<IntType> consumeIntegerTypeRaw(double minimumValue)
+    {
+        if (!m_calcValue)
+            return WTF::nullopt;
+        m_sourceRange = m_range;
+        return clampTo<IntType>(std::round(std::max(m_calcValue->doubleValue(), minimumValue)));
     }
 
@@ -185 +200 @@
 };
 
-RefPtr<CSSPrimitiveValue> consumeInteger(CSSParserTokenRange& range, double minimumValue)
+template<typename IntType> Optional<IntType> consumeIntegerTypeRaw(CSSParserTokenRange& range, double minimumValue)
 {
     const CSSParserToken& token = range.peek();
     if (token.type() == NumberToken) {
         if (token.numericValueType() == NumberValueType || token.numericValue() < minimumValue)
-            return nullptr;
-        return CSSValuePool::singleton().createValue(range.consumeIncludingWhitespace().numericValue(), CSSUnitType::CSS_NUMBER);
+            return WTF::nullopt;
+        return clampTo<IntType>(range.consumeIncludingWhitespace().numericValue());
     }
 
     if (token.type() != FunctionToken)
-        return nullptr;
+        return WTF::nullopt;
 
     CalcParser calcParser(range, CalculationCategory::Number);
     if (const CSSCalcValue* calculation = calcParser.value()) {
         if (calculation->category() != CalculationCategory::Number)
-            return nullptr;
-        return calcParser.consumeInteger(minimumValue);
-    }
-
+            return WTF::nullopt;
+        return calcParser.consumeIntegerTypeRaw<IntType>(minimumValue);
+    }
+
+    return WTF::nullopt;
+}
+
+Optional<int> consumeIntegerRaw(CSSParserTokenRange& range, double minimumValue)
+{
+    return consumeIntegerTypeRaw<int>(range, minimumValue);
+}
+
+RefPtr<CSSPrimitiveValue> consumeInteger(CSSParserTokenRange& range, double minimumValue)
+{
+    if (auto integer = consumeIntegerRaw(range, minimumValue))
+        return CSSValuePool::singleton().createValue(*integer, CSSUnitType::CSS_NUMBER);
     return nullptr;
 }
 
+Optional<unsigned> consumePositiveIntegerRaw(CSSParserTokenRange& range)
+{
+    return consumeIntegerTypeRaw<unsigned>(range, 1);
+}
+
 RefPtr<CSSPrimitiveValue> consumePositiveInteger(CSSParserTokenRange& range)
 {
-    return consumeInteger(range, 1);
+    if (auto integer = consumePositiveIntegerRaw(range))
+        return CSSValuePool::singleton().createValue(*integer, CSSUnitType::CSS_NUMBER);
+    return nullptr;
 }
 
@@ -724 +758 @@
     uint8_t alphaComponent = 255;
     if (consumeAlphaSeparator()) {
-        if (auto number = consumeNumberRaw(args))
-            alphaComponent = convertFloatAlphaTo<uint8_t>(*number);
-        else if (auto percent = consumePercentRaw(args))
-            alphaComponent = convertFloatAlphaTo<uint8_t>(*percent / 100.0);
-        else
+        auto alpha = consumeNumberOrPercentDividedBy100Raw(args);
+        if (!alpha)
             return { };
-    };
+        alphaComponent = convertFloatAlphaTo<uint8_t>(*alpha);
+    }
 
     if (!args.atEnd())
@@ -775 +807 @@
     double alpha = 1.0;
     if (commaConsumed || slashConsumed) {
-        if (auto number = consumeNumberRaw(args))
-            alpha = clampTo<double>(*number, 0.0, 1.0);
-        else if (auto percent = consumePercentRaw(args))
-            alpha = clampTo<double>(*percent / 100.0f, 0.0, 1.0);
-        else
+        auto alphaParameter = consumeNumberOrPercentDividedBy100Raw(args);
+        if (!alphaParameter)
             return { };
+        alpha = clampTo<double>(*alphaParameter, 0.0, 1.0);
     }
 
@@ -794 +824 @@
         return 1.0f;
 
-    if (auto alphaParameter = consumePercentRaw(range, ValueRangeAll))
-        return clampTo<float>(*alphaParameter / 100.0, 0.0, 1.0);
-
-    if (auto alphaParameter = consumeNumberRaw(range, ValueRangeAll))
+    if (auto alphaParameter = consumeNumberOrPercentDividedBy100Raw(range))
         return clampTo<float>(*alphaParameter, 0.0, 1.0);
 
@@ -873 +900 @@
     double channels[3] = { 0, 0, 0 };
     for (int i = 0; i < 3; ++i) {
-        if (auto number = consumeNumberRaw(args))
-            channels[i] = *number;
-        else if (auto percent = consumePercentRaw(args))
-            channels[i] = *percent / 100.0;
-        else
+        auto value = consumeNumberOrPercentDividedBy100Raw(args);
+        if (!value)
             break;
+        channels[i] = *value;
     }
 
@@ -1082 +1107 @@
 }
 
-static void positionFromOneValue(CSSPrimitiveValue& value, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
+static PositionCoordinates positionFromOneValue(CSSPrimitiveValue& value)
 {
     bool valueAppliesToYAxisOnly = isVerticalPositionKeywordOnly(value);
-    resultX = &value;
-    resultY = CSSPrimitiveValue::createIdentifier(CSSValueCenter);
     if (valueAppliesToYAxisOnly)
-        std::swap(resultX, resultY);
-}
-
-static bool positionFromTwoValues(CSSPrimitiveValue& value1, CSSPrimitiveValue& value2,
-    RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
-{
-    bool mustOrderAsXY = isHorizontalPositionKeywordOnly(value1) || isVerticalPositionKeywordOnly(value2)
-        || !value1.isValueID() || !value2.isValueID();
+        return { CSSPrimitiveValue::createIdentifier(CSSValueCenter), value };
+    return { value, CSSPrimitiveValue::createIdentifier(CSSValueCenter) };
+}
+
+static Optional<PositionCoordinates> positionFromTwoValues(CSSPrimitiveValue& value1, CSSPrimitiveValue& value2)
+{
+    bool mustOrderAsXY = isHorizontalPositionKeywordOnly(value1) || isVerticalPositionKeywordOnly(value2) || !value1.isValueID() || !value2.isValueID();
     bool mustOrderAsYX = isVerticalPositionKeywordOnly(value1) || isHorizontalPositionKeywordOnly(value2);
     if (mustOrderAsXY && mustOrderAsYX)
-        return false;
-    resultX = &value1;
-    resultY = &value2;
+        return WTF::nullopt;
     if (mustOrderAsYX)
-        std::swap(resultX, resultY);
-    return true;
+        return PositionCoordinates { value2, value1 };
+    return PositionCoordinates { value1, value2 };
 }
 
@@ -1124 +1144 @@
 //   [ center | [ top | bottom ] <length-percentage>? ]
 //
-static bool backgroundPositionFromThreeValues(const std::array<CSSPrimitiveValue*, 5>& values, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
-{
+static Optional<PositionCoordinates> backgroundPositionFromThreeValues(const std::array<CSSPrimitiveValue*, 5>& values)
+{
+    RefPtr<CSSPrimitiveValue> resultX;
+    RefPtr<CSSPrimitiveValue> resultY;
+
     CSSPrimitiveValue* center = nullptr;
     for (int i = 0; values[i]; i++) {
         CSSPrimitiveValue* currentValue = values[i];
         if (!currentValue->isValueID())
-            return false;
+            return WTF::nullopt;
 
         CSSValueID id = currentValue->valueID();
         if (id == CSSValueCenter) {
             if (center)
-                return false;
+                return WTF::nullopt;
             center = currentValue;
             continue;
@@ -1148 +1171 @@
         if (id == CSSValueLeft || id == CSSValueRight) {
             if (resultX)
-                return false;
+                return WTF::nullopt;
             resultX = result;
         } else {
             ASSERT(id == CSSValueTop || id == CSSValueBottom);
             if (resultY)
-                return false;
+                return WTF::nullopt;
             resultY = result;
         }
@@ -1161 +1184 @@
         ASSERT(resultX || resultY);
         if (resultX && resultY)
-            return false;
+            return WTF::nullopt;
         if (!resultX)
             resultX = center;
@@ -1169 +1192 @@
 
     ASSERT(resultX && resultY);
-    return true;
+    return PositionCoordinates { resultX.releaseNonNull(), resultY.releaseNonNull() };
 }
 
@@ -1182 +1205 @@
 //   [ [ top | bottom ] <length-percentage> ]
 //
-static bool positionFromFourValues(const std::array<CSSPrimitiveValue*, 5>& values, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
-{
+static Optional<PositionCoordinates> positionFromFourValues(const std::array<CSSPrimitiveValue*, 5>& values)
+{
+    RefPtr<CSSPrimitiveValue> resultX;
+    RefPtr<CSSPrimitiveValue> resultY;
+
     for (int i = 0; values[i]; i++) {
         CSSPrimitiveValue* currentValue = values[i];
         if (!currentValue->isValueID())
-            return false;
+            return WTF::nullopt;
 
         CSSValueID id = currentValue->valueID();
         if (id == CSSValueCenter)
-            return false;
+            return WTF::nullopt;
 
         RefPtr<CSSPrimitiveValue> result;
@@ -1201 +1227 @@
         if (id == CSSValueLeft || id == CSSValueRight) {
             if (resultX)
-                return false;
+                return WTF::nullopt;
             resultX = result;
         } else {
             ASSERT(id == CSSValueTop || id == CSSValueBottom);
             if (resultY)
-                return false;
+                return WTF::nullopt;
             resultY = result;
         }
@@ -1212 +1238 @@
 
     ASSERT(resultX && resultY);
-    return true;
+    return PositionCoordinates { resultX.releaseNonNull(), resultY.releaseNonNull() };
 }
 
 // FIXME: This may consume from the range upon failure. The background
 // shorthand works around it, but we should just fix it here.
-bool consumePosition(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, PositionSyntax positionSyntax, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
-{
-    RefPtr<CSSPrimitiveValue> value1 = consumePositionComponent(range, cssParserMode, unitless);
+Optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, PositionSyntax positionSyntax)
+{
+    auto value1 = consumePositionComponent(range, cssParserMode, unitless);
     if (!value1)
-        return false;
-
-    RefPtr<CSSPrimitiveValue> value2 = consumePositionComponent(range, cssParserMode, unitless);
-    if (!value2) {
-        positionFromOneValue(*value1, resultX, resultY);
-        return true;
-    }
-
-    RefPtr<CSSPrimitiveValue> value3 = consumePositionComponent(range, cssParserMode, unitless);
+        return WTF::nullopt;
+
+    auto value2 = consumePositionComponent(range, cssParserMode, unitless);
+    if (!value2)
+        return positionFromOneValue(*value1);
+
+    auto value3 = consumePositionComponent(range, cssParserMode, unitless);
     if (!value3)
-        return positionFromTwoValues(*value1, *value2, resultX, resultY);
-
-    RefPtr<CSSPrimitiveValue> value4 = consumePositionComponent(range, cssParserMode, unitless);
+        return positionFromTwoValues(*value1, *value2);
+
+    auto value4 = consumePositionComponent(range, cssParserMode, unitless);
     
-    std::array<CSSPrimitiveValue*, 5> values;
-    values[0] = value1.get();
-    values[1] = value2.get();
-    values[2] = value3.get();
-    values[3] = value4.get();
-    values[4] = nullptr;
+    std::array<CSSPrimitiveValue*, 5> values {
+        value1.get(),
+        value2.get(),
+        value3.get(),
+        value4.get(),
+        nullptr
+    };
     
     if (value4)
-        return positionFromFourValues(values, resultX, resultY);
+        return positionFromFourValues(values);
     
     if (positionSyntax != PositionSyntax::BackgroundPosition)
-        return false;
+        return WTF::nullopt;
     
-    return backgroundPositionFromThreeValues(values, resultX, resultY);
+    return backgroundPositionFromThreeValues(values);
 }
 
 RefPtr<CSSPrimitiveValue> consumePosition(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, PositionSyntax positionSyntax)
 {
-    RefPtr<CSSPrimitiveValue> resultX;
-    RefPtr<CSSPrimitiveValue> resultY;
-    if (consumePosition(range, cssParserMode, unitless, positionSyntax, resultX, resultY))
-        return CSSPropertyParserHelpersInternal::createPrimitiveValuePair(resultX.releaseNonNull(), resultY.releaseNonNull());
+    if (auto coordinates = consumePositionCoordinates(range, cssParserMode, unitless, positionSyntax))
+        return CSSPropertyParserHelpersInternal::createPrimitiveValuePair(WTFMove(coordinates->x), WTFMove(coordinates->y));
     return nullptr;
 }
 
-bool consumeOneOrTwoValuedPosition(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY)
-{
-    RefPtr<CSSPrimitiveValue> value1 = consumePositionComponent(range, cssParserMode, unitless);
+Optional<PositionCoordinates> consumeOneOrTwoValuedPositionCoordinates(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless)
+{
+    auto value1 = consumePositionComponent(range, cssParserMode, unitless);
     if (!value1)
-        return false;
-    RefPtr<CSSPrimitiveValue> value2 = consumePositionComponent(range, cssParserMode, unitless);
-    if (!value2) {
-        positionFromOneValue(*value1, resultX, resultY);
-        return true;
-    }
-    return positionFromTwoValues(*value1, *value2, resultX, resultY);
+        return WTF::nullopt;
+    auto value2 = consumePositionComponent(range, cssParserMode, unitless);
+    if (!value2)
+        return positionFromOneValue(*value1);
+    return positionFromTwoValues(*value1, *value2);
 }
 
@@ -1311 +1332 @@
     } else {
         ASSERT(id == CSSValueColorStop);
-        if (auto percent = consumePercentRaw(args, ValueRangeAll))
-            position = *percent / 100.0;
-        else if (auto number = consumeNumberRaw(args))
-            position = *number;
-        else
+        auto value = consumeNumberOrPercentDividedBy100Raw(args);
+        if (!value)
             return false;
+        position = *value;
 
         if (!consumeCommaIncludingWhitespace(args))
@@ -1437 +1456 @@
     auto result = CSSRadialGradientValue::create(repeating, CSSPrefixedRadialGradient);
 
-    RefPtr<CSSPrimitiveValue> centerX;
-    RefPtr<CSSPrimitiveValue> centerY;
-    consumeOneOrTwoValuedPosition(args, cssParserMode, UnitlessQuirk::Forbid, centerX, centerY);
-    if ((centerX || centerY) && !consumeCommaIncludingWhitespace(args))
+    auto centerCoordinate = consumeOneOrTwoValuedPositionCoordinates(args, cssParserMode, UnitlessQuirk::Forbid);
+    if (centerCoordinate && !consumeCommaIncludingWhitespace(args))
         return nullptr;
 
@@ -1467 +1484 @@
         return nullptr;
 
-    result->setFirstX(centerX.copyRef());
-    result->setFirstY(centerY.copyRef());
-    result->setSecondX(WTFMove(centerX));
-    result->setSecondY(WTFMove(centerY));
+    if (centerCoordinate) {
+        result->setFirstX(centerCoordinate->x.copyRef());
+        result->setFirstY(centerCoordinate->y.copyRef());
+        result->setSecondX(WTFMove(centerCoordinate->x));
+        result->setSecondY(WTFMove(centerCoordinate->y));
+    }
     result->setShape(WTFMove(shape));
     result->setSizingBehavior(WTFMove(sizeKeyword));
@@ -1539 +1558 @@
     if (args.peek().id() == CSSValueAt) {
         args.consumeIncludingWhitespace();
-        consumePosition(args, cssParserMode, UnitlessQuirk::Forbid, PositionSyntax::Position, centerX, centerY);
-        if (!(centerX && centerY))
+        
+        auto centerCoordinate = consumePositionCoordinates(args, cssParserMode, UnitlessQuirk::Forbid, PositionSyntax::Position);
+        if (!centerCoordinate)
             return nullptr;
+        
+        centerX = WTFMove(centerCoordinate->x);
+        centerY = WTFMove(centerCoordinate->y);
         
         result->setFirstX(centerX.copyRef());
@@ -1614 +1637 @@
 
         if (consumeIdent<CSSValueAt>(args)) {
-            RefPtr<CSSPrimitiveValue> centerX;
-            RefPtr<CSSPrimitiveValue> centerY;
-            consumePosition(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position, centerX, centerY);
-            if (!(centerX && centerY))
+            auto centerCoordinate = consumePositionCoordinates(args, context.mode, UnitlessQuirk::Forbid, PositionSyntax::Position);
+            if (!centerCoordinate)
                 return nullptr;
 
-            result->setFirstX(centerX.copyRef());
-            result->setFirstY(centerY.copyRef());
+            result->setFirstX(centerCoordinate->x.copyRef());
+            result->setFirstY(centerCoordinate->y.copyRef());
 
             // Right now, conic gradients have the same start and end centers.
-            result->setSecondX(WTFMove(centerX));
-            result->setSecondY(WTFMove(centerY));
+            result->setSecondX(WTFMove(centerCoordinate->x));
+            result->setSecondY(WTFMove(centerCoordinate->y));
 
             expectComma = true;
@@ -1653 +1674 @@
 static RefPtr<CSSValue> consumeCrossFade(CSSParserTokenRange& args, CSSParserContext context, bool prefixed)
 {
-    RefPtr<CSSValue> fromImageValue = consumeImageOrNone(args, context);
+    auto fromImageValue = consumeImageOrNone(args, context);
     if (!fromImageValue || !consumeCommaIncludingWhitespace(args))
         return nullptr;
-    RefPtr<CSSValue> toImageValue = consumeImageOrNone(args, context);
+    auto toImageValue = consumeImageOrNone(args, context);
     if (!toImageValue || !consumeCommaIncludingWhitespace(args))
         return nullptr;
 
-    RefPtr<CSSPrimitiveValue> percentage;
-    if (auto percentValue = consumePercent(args, ValueRangeAll))
-        percentage = CSSValuePool::singleton().createValue(clampTo<double>(percentValue->doubleValue() / 100.0, 0, 1), CSSUnitType::CSS_NUMBER);
-    else if (auto numberValue = consumeNumber(args, ValueRangeAll))
-        percentage = CSSValuePool::singleton().createValue(clampTo<double>(numberValue->doubleValue(), 0, 1), CSSUnitType::CSS_NUMBER);
-
+    auto percentage = consumeNumberOrPercentDividedBy100Raw(args);
     if (!percentage)
         return nullptr;
-    return CSSCrossfadeValue::create(fromImageValue.releaseNonNull(), toImageValue.releaseNonNull(), percentage.releaseNonNull(), prefixed);
+    auto percentageValue = CSSValuePool::singleton().createValue(clampTo<double>(*percentage, 0, 1), CSSUnitType::CSS_NUMBER);
+    return CSSCrossfadeValue::create(fromImageValue.releaseNonNull(), toImageValue.releaseNonNull(), WTFMove(percentageValue), prefixed);
 }
 

trunk/Source/WebCore/css/parser/CSSPropertyParserHelpers.h

@@ -71 +71 @@
 using LengthOrPercentRaw = Variant<LengthRaw, double>;
 
+Optional<int> consumeIntegerRaw(CSSParserTokenRange&, double minimumValue = -std::numeric_limits<double>::max());
 RefPtr<CSSPrimitiveValue> consumeInteger(CSSParserTokenRange&, double minimumValue = -std::numeric_limits<double>::max());
+Optional<unsigned> consumePositiveIntegerRaw(CSSParserTokenRange&);
 RefPtr<CSSPrimitiveValue> consumePositiveInteger(CSSParserTokenRange&);
 Optional<double> consumeNumberRaw(CSSParserTokenRange&, ValueRange = ValueRangeAll);
@@ -109 +111 @@
 };
 
+struct PositionCoordinates {
+    Ref<CSSPrimitiveValue> x;
+    Ref<CSSPrimitiveValue> y;
+};
+
 RefPtr<CSSPrimitiveValue> consumePosition(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax);
-bool consumePosition(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY);
-bool consumeOneOrTwoValuedPosition(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, RefPtr<CSSPrimitiveValue>& resultX, RefPtr<CSSPrimitiveValue>& resultY);
+Optional<PositionCoordinates> consumePositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk, PositionSyntax);
+Optional<PositionCoordinates> consumeOneOrTwoValuedPositionCoordinates(CSSParserTokenRange&, CSSParserMode, UnitlessQuirk);
 
 enum class AllowedImageType : uint8_t {