Move two color related properties 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 5d73210fff7c5d6fe4b1556c003ccbd3d01e95eb..9440feca619976def34519e99155653386d08862 100644
--- a/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
+++ b/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
@@ -358,6 +358,167 @@ static PassRefPtrWillBeRawPtr<CSSPrimitiveValue> consumeTime(CSSParserTokenRange
     return nullptr;
 }
 
+static inline int colorIntFromValue(double value, bool isPercent)
+{

[Timothy Loh, 2015/10/26 05:23:09]

Both IE and FF round instead of truncating, and multiply by 255 instead of 256
(as per spec). Also, why don't we just take a CSSValue here?

static int clampForRGBColor(const CSSValue& value) {
    double result = value.getDoubleValue();
    // TODO(timloh): Multiply by 2.55 and round instead of floor.
    if (value.isPercentage())
        result *= 2.56;
    return clampTo<int>(result, 0, 255);
}

Probably also should be using clampTo everywhere else too.

[rwlbuis, 2015/10/26 22:21:24]

Done.

+    if (value <= 0.0)
+        return 0;
+
+    if (isPercent) {
+        if (value >= 100.0)
+            return 255;
+        return static_cast<int>(value * 256.0 / 100.0);
+    }
+
+    if (value >= 255.0)
+        return 255;
+
+    return static_cast<int>(value);
+}
+
+static bool parseColorParameters(CSSParserTokenRange& range, int* colorArray, bool parseAlpha)
+{
+    ASSERT(range.peek().type() == FunctionToken);

[Timothy Loh, 2015/10/26 05:23:09]

I think consumeFunction asserts this

[rwlbuis, 2015/10/26 22:21:24]

Done.

+    CSSParserTokenRange args = consumeFunction(range);
+    // Get the first value and its type
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> colorParameter = consumeInteger(args);
+    if (!colorParameter)
+        colorParameter = consumePercent(args, ValueRangeAll);
+    if (!colorParameter)
+        return false;
+
+    const bool isPercent = colorParameter->isPercentage();
+    colorArray[0] = colorIntFromValue(colorParameter->getDoubleValue(), isPercent);
+    for (int i = 1; i < 3; i++) {
+        if (!consumeCommaIncludingWhitespace(args))
+            return false;
+        colorParameter = consumeInteger(args);

[Timothy Loh, 2015/10/26 05:23:09]

I'd prefer:

= isPercent ? consumePercent(args, ValueRangeAll) : consumeInteger(args);

[rwlbuis, 2015/10/26 22:21:24]

Way better, done.

+        if (!colorParameter)
+            colorParameter = consumePercent(args, ValueRangeAll);
+        if (!colorParameter || colorParameter->isPercentage() != isPercent)
+            return false;
+        colorArray[i] = colorIntFromValue(colorParameter->getDoubleValue(), isPercent);
+    }
+    if (parseAlpha) {
+        if (!consumeCommaIncludingWhitespace(args))
+            return false;
+        RefPtrWillBeRawPtr<CSSPrimitiveValue> alphaValue = consumeNumber(args, ValueRangeAll);
+        if (!alphaValue)
+            return false;
+        double doubleValue = alphaValue->getDoubleValue();
+        // Convert the floating pointer number of alpha to an integer in the range [0, 256),
+        // with an equal distribution across all 256 values.
+        colorArray[3] = static_cast<int>(std::max(0.0, std::min(1.0, doubleValue)) * nextafter(256.0, 0.0));
+    }
+    return args.atEnd();
+}
+
+static bool parseHSLParameters(CSSParserTokenRange& range, double* colorArray, bool parseAlpha)
+{
+    // Get the first value

[Timothy Loh, 2015/10/26 05:23:09]

I don't think this comment is useful

[rwlbuis, 2015/10/26 22:21:24]

Done.

+    ASSERT(range.peek().type() == FunctionToken);
+    CSSParserTokenRange args = consumeFunction(range);
+    RefPtrWillBeRawPtr<CSSPrimitiveValue> hslValue = consumeNumber(args, ValueRangeAll);
+    if (!hslValue)
+        return false;
+    // normalize the Hue value and change it to be between 0 and 1.0

[Timothy Loh, 2015/10/26 05:23:09]

I don't think this comment is useful either 

[rwlbuis, 2015/10/26 22:21:24]

Done.

+    colorArray[0] = (((static_cast<int>(hslValue->getDoubleValue()) % 360) + 360) % 360) / 360.0;

[Timothy Loh, 2015/10/26 05:23:09]

we can probably drop the static_cast here

[rwlbuis, 2015/10/26 22:21:24]

You mean using getIntValue right? Done.

+    for (int i = 1; i < 3; i++) {
+        if (!consumeCommaIncludingWhitespace(args))
+            return false;
+        hslValue = consumePercent(args, ValueRangeAll);
+        if (!hslValue)
+            return false;
+        double doubleValue = hslValue->getDoubleValue();
+        colorArray[i] = std::max(0.0, std::min(100.0, doubleValue)) / 100.0; // needs to be value between 0 and 1.0
+    }
+    if (parseAlpha) {
+        if (!consumeCommaIncludingWhitespace(args))
+            return false;
+        hslValue = consumeNumber(args, ValueRangeAll);
+        if (!hslValue)
+            return false;
+        double doubleValue = hslValue->getDoubleValue();
+        colorArray[3] = std::max(0.0, std::min(1.0, doubleValue));
+    }
+    return args.atEnd();
+}
+
+static bool parseColorFromValue(CSSParserTokenRange& range, RGBA32& result, bool acceptQuirkyColors)
+{
+    const CSSParserToken& token = range.peek();
+    if (acceptQuirkyColors && token.type() == NumberToken && token.numericValueType() == IntegerValueType
+        && token.numericValue() >= 0. && token.numericValue() < 1000000.) {
+        String str = String::format("%06d", static_cast<int>(token.numericValue()));
+        if (!Color::parseHexColor(str, result))
+            return false;
+        range.consumeIncludingWhitespace();
+        return true;
+    }
+    if (acceptQuirkyColors && token.type() == DimensionToken) {
+        String color = token.value();

[Timothy Loh, 2015/10/26 05:23:09]

Don't we need to prepend the numeric value to this?

[rwlbuis, 2015/10/26 22:21:24]

Yeah, sorry. I changes color-quirk.html test to catch this.

+        if (color.length() > 6)
+            return false;
+        while (color.length() < 6)
+            color = "0" + color;
+        range.consumeIncludingWhitespace();
+        return Color::parseHexColor(color, result);
+    }
+    if (token.type() == IdentToken) {
+        Color color;
+        if (color.setNamedColor(token.value()))

[Timothy Loh, 2015/10/26 05:23:09]

isn't this covered by isColorKeyword in the calling function?

[rwlbuis, 2015/10/26 22:21:24]

Well spotted! I guess the original code has the same issue. Done.

+            result = color.rgb();
+        else if (!acceptQuirkyColors || !Color::parseHexColor(token.value(), result))
+            return false;
+        range.consumeIncludingWhitespace();
+        return true;
+    }
+    if (token.type() == HashToken) {
+        CSSParserString string = range.consumeIncludingWhitespace().value();
+        if (string.is8Bit())
+            return Color::parseHexColor(string.characters8(), string.length(), result);
+        return Color::parseHexColor(string.characters16(), string.length(), result);
+    }
+    if (token.functionId() == CSSValueRgb) {
+        int colorValues[3];
+        if (!parseColorParameters(range, colorValues, false))
+            return false;
+        result = makeRGB(colorValues[0], colorValues[1], colorValues[2]);
+    } else if (token.functionId() == CSSValueRgba) {
+        int colorValues[4];
+        if (!parseColorParameters(range, colorValues, true))
+            return false;
+        result = makeRGBA(colorValues[0], colorValues[1], colorValues[2], colorValues[3]);
+    } else if (token.functionId() == CSSValueHsl) {
+        double colorValues[3];
+        if (!parseHSLParameters(range, colorValues, false))
+            return false;
+        result = makeRGBAFromHSLA(colorValues[0], colorValues[1], colorValues[2], 1.0);
+    } else if (token.functionId() == CSSValueHsla) {
+        double colorValues[4];
+        if (!parseHSLParameters(range, colorValues, true))
+            return false;
+        result = makeRGBAFromHSLA(colorValues[0], colorValues[1], colorValues[2], colorValues[3]);
+    } else {
+        return false;
+    }
+
+    return true;
+}
+
+static PassRefPtrWillBeRawPtr<CSSValue> consumeColor(CSSParserTokenRange& range, const CSSParserContext& context, bool acceptQuirkyColors = false)
+{
+    CSSValueID id = range.peek().id();
+    if (CSSPropertyParser::isColorKeyword(id)) {
+        if (!isValueAllowedInMode(id, context.mode()))
+            return nullptr;
+        return consumeIdent(range);
+    }
+    RGBA32 c = Color::transparent;
+    if (!parseColorFromValue(range, c, acceptQuirkyColors))

[Timothy Loh, 2015/10/26 05:23:09]

There are a few places where we're updating the range but returning false (e.g.
#foo, rgb(bla)). We probably don't want to modify the range in these cases.

[rwlbuis, 2015/10/26 22:21:24]

Done.

+        return nullptr;
+    return cssValuePool().createColorValue(c);
+}
+
 static inline bool isCSSWideKeyword(const CSSValueID& id)
 {
     return id == CSSValueInitial || id == CSSValueInherit || id == CSSValueUnset || id == CSSValueDefault;
@@ -1307,6 +1468,11 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSProperty
     case CSSPropertyOrphans:
     case CSSPropertyWidows:
         return consumeWidowsOrOrphans(m_range);
+    case CSSPropertyWebkitTextFillColor:
+    case CSSPropertyWebkitTapHighlightColor:
+        return consumeColor(m_range, m_context);
+    case CSSPropertyColor:
+        return consumeColor(m_range, m_context, inQuirksMode());
     default:
         return nullptr;
     }