Merge parseBorderRadius and parseInsetRoundedCorners

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 4603de75fc01a0ad47b24c1320cd41d8259d2a7f..ae3459ccf5018422de1d8778ec8b1132c600f0d0 100644
--- a/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
@@ -532,8 +532,19 @@ bool CSSPropertyParser::parseValue(CSSPropertyID unresolvedProperty, bool import
         return true;
     }
     case CSSPropertyBorderRadius:
-    case CSSPropertyAliasWebkitBorderRadius:
-        return parseBorderRadius(unresolvedProperty, important);
+    case CSSPropertyAliasWebkitBorderRadius: {

[Timothy Loh, 2016/01/11 02:01:46]

btw the switch is on a resolved property so the alias case isn't actually
needed.

[rwlbuis, 2016/01/11 14:46:18]

Done.

+        ShorthandScope scope(this, unresolvedProperty);
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4];
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> radii2[4];
+        if (!parseRadii(radii, radii2, m_valueList, unresolvedProperty))
+            return false;
+        ImplicitScope implicitScope(this);
+        addProperty(CSSPropertyBorderTopLeftRadius, CSSValuePair::create(radii[0].release(), radii2[0].release(), CSSValuePair::DropIdenticalValues), important);
+        addProperty(CSSPropertyBorderTopRightRadius, CSSValuePair::create(radii[1].release(), radii2[1].release(), CSSValuePair::DropIdenticalValues), important);
+        addProperty(CSSPropertyBorderBottomRightRadius, CSSValuePair::create(radii[2].release(), radii2[2].release(), CSSValuePair::DropIdenticalValues), important);
+        addProperty(CSSPropertyBorderBottomLeftRadius, CSSValuePair::create(radii[3].release(), radii2[3].release(), CSSValuePair::DropIdenticalValues), important);
+        return true;
+    }
     case CSSPropertyWebkitBoxReflect:
         if (id == CSSValueNone)
             validPrimitive = true;
@@ -2801,76 +2812,6 @@ static void completeBorderRadii(RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4])
     radii[3] = radii[1];
 }
 
-// FIXME: This should be refactored with parseBorderRadius.
-// parseBorderRadius contains support for some legacy radius construction.
-PassRefPtrWillBeRawPtr<CSSBasicShapeInsetValue> CSSPropertyParser::parseInsetRoundedCorners(PassRefPtrWillBeRawPtr<CSSBasicShapeInsetValue> shape, CSSParserValueList* args)
-{
-    CSSParserValue* argument = args->next();
-
-    if (!argument)
-        return nullptr;
-
-    Vector<CSSParserValue*> radiusArguments;
-    while (argument) {
-        radiusArguments.append(argument);
-        argument = args->next();
-    }
-
-    unsigned num = radiusArguments.size();
-    if (!num || num > 9)
-        return nullptr;
-
-    // FIXME: Refactor completeBorderRadii and the array
-    RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[2][4];
-#if ENABLE(OILPAN)
-    // Zero initialize the array of raw pointers.
-    memset(&radii, 0, sizeof(radii));
-#endif
-
-    unsigned indexAfterSlash = 0;
-    for (unsigned i = 0; i < num; ++i) {
-        CSSParserValue* value = radiusArguments.at(i);
-        if (value->m_unit == CSSParserValue::Operator) {
-            if (value->iValue != '/')
-                return nullptr;
-
-            if (!i || indexAfterSlash || i + 1 == num)
-                return nullptr;
-
-            indexAfterSlash = i + 1;
-            completeBorderRadii(radii[0]);
-            continue;
-        }
-
-        if (i - indexAfterSlash >= 4)
-            return nullptr;
-
-        if (!validUnit(value, FLength | FPercent | FNonNeg))
-            return nullptr;
-
-        RefPtrWillBeRawPtr<CSSPrimitiveValue> radius = createPrimitiveNumericValue(value);
-
-        if (!indexAfterSlash)
-            radii[0][i] = radius;
-        else
-            radii[1][i - indexAfterSlash] = radius.release();
-    }
-
-    if (!indexAfterSlash) {
-        completeBorderRadii(radii[0]);
-        for (unsigned i = 0; i < 4; ++i)
-            radii[1][i] = radii[0][i];
-    } else {
-        completeBorderRadii(radii[1]);
-    }
-    shape->setTopLeftRadius(CSSValuePair::create(radii[0][0].release(), radii[1][0].release(), CSSValuePair::DropIdenticalValues));
-    shape->setTopRightRadius(CSSValuePair::create(radii[0][1].release(), radii[1][1].release(), CSSValuePair::DropIdenticalValues));
-    shape->setBottomRightRadius(CSSValuePair::create(radii[0][2].release(), radii[1][2].release(), CSSValuePair::DropIdenticalValues));
-    shape->setBottomLeftRadius(CSSValuePair::create(radii[0][3].release(), radii[1][3].release(), CSSValuePair::DropIdenticalValues));
-
-    return shape;
-}
-
 PassRefPtrWillBeRawPtr<CSSBasicShapeInsetValue> CSSPropertyParser::parseBasicShapeInset(CSSParserValueList* args)
 {
     ASSERT(args);
@@ -2883,6 +2824,8 @@ PassRefPtrWillBeRawPtr<CSSBasicShapeInsetValue> CSSPropertyParser::parseBasicSha
 
     while (argument) {
         if (argument->m_unit == CSSParserValue::Identifier && argument->id == CSSValueRound) {
+            if (!args->next())
+                return nullptr;
             hasRoundedInset = true;
             break;
         }
@@ -2916,8 +2859,18 @@ PassRefPtrWillBeRawPtr<CSSBasicShapeInsetValue> CSSPropertyParser::parseBasicSha
         return nullptr;
     }
 
-    if (hasRoundedInset)
-        return parseInsetRoundedCorners(shape.release(), args);
+    if (hasRoundedInset) {
+        // FIXME: Refactor completeBorderRadii and the array

[Timothy Loh, 2016/01/11 02:01:46]

What did you have in mind? Or is this just a legacy comment?

[rwlbuis, 2016/01/11 14:46:18]

Well I am still not completely sure the array is the best solution, hence the
FIXME. Maybe it can be addressed once it is ported to use CSSParserTokens.

+        RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4];
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> radii2[4];
+        if (!parseRadii(radii, radii2, args))
+            return nullptr;
+        shape->setTopLeftRadius(CSSValuePair::create(radii[0].release(), radii2[0].release(), CSSValuePair::DropIdenticalValues));
+        shape->setTopRightRadius(CSSValuePair::create(radii[1].release(), radii2[1].release(), CSSValuePair::DropIdenticalValues));
+        shape->setBottomRightRadius(CSSValuePair::create(radii[2].release(), radii2[2].release(), CSSValuePair::DropIdenticalValues));
+        shape->setBottomLeftRadius(CSSValuePair::create(radii[3].release(), radii2[3].release(), CSSValuePair::DropIdenticalValues));
+    }
+
     return shape.release();
 }
 
@@ -4032,66 +3985,40 @@ bool CSSPropertyParser::parseBorderImageOutset(RefPtrWillBeRawPtr<CSSQuadValue>&
     return parseBorderImageQuad(FLength | FNumber | FNonNeg, result);
 }
 
-bool CSSPropertyParser::parseBorderRadius(CSSPropertyID unresolvedProperty, bool important)
+bool CSSPropertyParser::parseRadii(RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4], RefPtrWillBeRawPtr<CSSPrimitiveValue> radii2[4], CSSParserValueList* args, CSSPropertyID unresolvedProperty)
 {
-    unsigned num = m_valueList->size();
-    if (num > 9)
-        return false;
-
-    ShorthandScope scope(this, unresolvedProperty);
-    RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[2][4];
-#if ENABLE(OILPAN)
-    // Zero initialize the array of raw pointers.
-    memset(&radii, 0, sizeof(radii));
-#endif
-
-    unsigned indexAfterSlash = 0;
-    for (unsigned i = 0; i < num; ++i) {
-        CSSParserValue* value = m_valueList->valueAt(i);
-        if (value->m_unit == CSSParserValue::Operator) {
-            if (value->iValue != '/')
-                return false;
-
-            if (!i || indexAfterSlash || i + 1 == num || num > i + 5)
-                return false;
-
-            indexAfterSlash = i + 1;
-            completeBorderRadii(radii[0]);
-            continue;
-        }
-
-        if (i - indexAfterSlash >= 4)
-            return false;
-
+    CSSParserValue* value = args->current();
+    int i;
+    for (i = 0; i < 4 && value && value->m_unit != CSSParserValue::Operator;++i, value = args->next()) {
         if (!validUnit(value, FLength | FPercent | FNonNeg))
             return false;
 
-        RefPtrWillBeRawPtr<CSSPrimitiveValue> radius = createPrimitiveNumericValue(value);
-
-        if (!indexAfterSlash) {
-            radii[0][i] = radius;
-
-            // Legacy syntax: -webkit-border-radius: l1 l2; is equivalent to border-radius: l1 / l2;
-            if (num == 2 && unresolvedProperty == CSSPropertyAliasWebkitBorderRadius) {
-                indexAfterSlash = 1;
-                completeBorderRadii(radii[0]);
-            }
-        } else
-            radii[1][i - indexAfterSlash] = radius.release();
+        radii[i] = createPrimitiveNumericValue(value);
     }
 
-    if (!indexAfterSlash) {
-        completeBorderRadii(radii[0]);
+    if (!i || (value && value->iValue != '/'))

[Timothy Loh, 2016/01/11 02:01:46]

probably need to check that m_unit is Operator

[rwlbuis, 2016/01/11 14:46:18]

Done.

+        return false;
+    // Legacy syntax: -webkit-border-radius: l1 l2; is equivalent to border-radius: l1 / l2;
+    if (!value && i == 2 && unresolvedProperty == CSSPropertyAliasWebkitBorderRadius) {
+        radii2[0] = radii[1];
+        radii[1] = nullptr;
+        completeBorderRadii(radii);
+        completeBorderRadii(radii2);
+        return true;
+    }
+    completeBorderRadii(radii);
+    if (value) {
+        value = args->next();
+        for (i = 0; i < 4 && value && validUnit(value, FLength | FPercent | FNonNeg); ++i, value = args->next())

[Timothy Loh, 2016/01/11 02:01:46]

I don't like having so much logic inside a for-loop header, but I guess it's ok
for now -- I'll comment on the coding style more when we start to move the logic
across to use CSSParserTokens.

[rwlbuis, 2016/01/11 14:46:18]

Acknowledged.

+            radii2[i] = createPrimitiveNumericValue(value);
+        if (!i || value)
+            return false;
+        completeBorderRadii(radii2);
+    } else {
         for (unsigned i = 0; i < 4; ++i)
-            radii[1][i] = radii[0][i];
-    } else
-        completeBorderRadii(radii[1]);
+            radii2[i] = radii[i];
+    }
 
-    ImplicitScope implicitScope(this);
-    addProperty(CSSPropertyBorderTopLeftRadius, CSSValuePair::create(radii[0][0].release(), radii[1][0].release(), CSSValuePair::DropIdenticalValues), important);
-    addProperty(CSSPropertyBorderTopRightRadius, CSSValuePair::create(radii[0][1].release(), radii[1][1].release(), CSSValuePair::DropIdenticalValues), important);
-    addProperty(CSSPropertyBorderBottomRightRadius, CSSValuePair::create(radii[0][2].release(), radii[1][2].release(), CSSValuePair::DropIdenticalValues), important);
-    addProperty(CSSPropertyBorderBottomLeftRadius, CSSValuePair::create(radii[0][3].release(), radii[1][3].release(), CSSValuePair::DropIdenticalValues), important);
     return true;
 }