Bug 164146 – Parse color() function

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Parse color() function

[Dean Jackson]

<rdar://problem/29007218>

[Dean Jackson]

Created attachment 293205 [details]
Patch

[Darin Adler]

Comment on attachment 293205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293205&action=review

> Source/WebCore/css/parser/CSSParser.cpp:7637
> +inline double CSSParser::colorDoubleFromValue(ValueWithCalculation& valueWithCalculation)

This should have the name parse in it for consistency with the other function. Maybe parsedColorDouble. Or maybe parseColorDouble and we should rename the others to match.

> Source/WebCore/css/parser/CSSParser.cpp:7651
> +    bool isPercent;
> +
> +    if (valueWithCalculation.calculation())
> +        isPercent = valueWithCalculation.calculation()->category() == CalcPercent;
> +    else
> +        isPercent = valueWithCalculation.value().unit == CSSPrimitiveValue::CSS_PERCENTAGE;
> +
> +    const double doubleValue = parsedDouble(valueWithCalculation);
> +
> +    if (isPercent)
> +        return doubleValue / 100.0;
> +
> +    return doubleValue;

I wouldn’t have left so many blank lines. No need to make doubleValue be const. Could make an isPercent helper function to make this easier to read.

> Source/WebCore/css/parser/CSSParser.cpp:7692
> +bool CSSParser::parseColorFunctionParameters(CSSParserValue& value, ColorSpace& colorSpace, double* colorValues)

Argument type should be more like:

    double colorValues[4]

Rather than double* colorValues.

Or even better could use a return value of type Optional<std:array<double, 4>>.

> Source/WebCore/css/parser/CSSParser.cpp:7715
> +            colorValues[i] = std::max<double>(0, std::min<double>(1, parsedDouble(argumentWithCalculation)));

Could write this using 0.0 and 1.0 and then would not need either of the <double>. Could do that many other places in this file and I think it’s nicer than what we have.

Why does this use parsedDouble instead of colorDoubleFromValue? Maybe colorDoubleFromValue has the wrong name if it’s only for the alpha channel.

> Source/WebCore/css/parser/CSSParser.cpp:7721
> +        return true;

We should set colorValues[3] to 1 here, instead of having the caller do it.

> Source/WebCore/css/parser/CSSParser.cpp:7831
> +        && value.function->args->size()

Seems slightly peculiar to check this here rather than inside parseColorFunctionParameters. In the other cases above we are checking for an exact size, but here we are doing part of the size checking; better to do none.

> Source/WebCore/css/parser/CSSParser.cpp:7835
> +        ColorSpace colorSpace = ColorSpaceSRGB;

No need to use a local variable for this.

> Source/WebCore/css/parser/CSSParser.cpp:7838
> +        return Color(colorValues[0], colorValues[1], colorValues[2], colorValues[3], colorSpace);

I would prefer the Color { } syntax to the Color() syntax. Same arguments would work.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:469
> +    ColorSpace colorSpace;

I suggest declaring this right before the switch instead of up here.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:470
> +    Color result;

Unused.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:495
> +        RefPtr<CSSPrimitiveValue> alphaParameter = consumePercent(args, ValueRangeAll);

Use auto here?

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:130
> +        switch (extendedColor.colorSpace()) {
> +        case ColorSpaceDisplayP3:
> +            return CGColorCreate(displayP3ColorSpaceRef(), components);
> +        default:
> +            return CGColorCreate(sRGBColorSpaceRef(), components);
> +        }

It would be better to cover all values for ColorSpace, and no default case, so this fails to compile if we add a new color space.

[Dean Jackson]

Comment on attachment 293205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=293205&action=review

>> Source/WebCore/css/parser/CSSParser.cpp:7637
>> +inline double CSSParser::colorDoubleFromValue(ValueWithCalculation& valueWithCalculation)
> 
> This should have the name parse in it for consistency with the other function. Maybe parsedColorDouble. Or maybe parseColorDouble and we should rename the others to match.

Done. Now called parseColorDouble, and I renamed parseColorInt too. I couldn't rename parsedDouble, which was annoying, because it already exists.

>> Source/WebCore/css/parser/CSSParser.cpp:7651
>> +    return doubleValue;
> 
> I wouldn’t have left so many blank lines. No need to make doubleValue be const. Could make an isPercent helper function to make this easier to read.

Done

>> Source/WebCore/css/parser/CSSParser.cpp:7692
>> +bool CSSParser::parseColorFunctionParameters(CSSParserValue& value, ColorSpace& colorSpace, double* colorValues)
> 
> Argument type should be more like:
> 
>     double colorValues[4]
> 
> Rather than double* colorValues.
> 
> Or even better could use a return value of type Optional<std:array<double, 4>>.

I changed it to Optional<std::pair<std::array<double, 4>, ColorSpace>>
That way it's clear that all the return values hold the info.

>> Source/WebCore/css/parser/CSSParser.cpp:7715
>> +            colorValues[i] = std::max<double>(0, std::min<double>(1, parsedDouble(argumentWithCalculation)));
> 
> Could write this using 0.0 and 1.0 and then would not need either of the <double>. Could do that many other places in this file and I think it’s nicer than what we have.
> 
> Why does this use parsedDouble instead of colorDoubleFromValue? Maybe colorDoubleFromValue has the wrong name if it’s only for the alpha channel.

Done. And fixed the other places in the file.

colorDoubleFromValue accepts percentages, but they are not allowed in the color() function except for the alpha value. (I have no idea why - this is just the typical CSS WG approach of trying to accept as much as possible).

>> Source/WebCore/css/parser/CSSParser.cpp:7721
>> +        return true;
> 
> We should set colorValues[3] to 1 here, instead of having the caller do it.

I now do that by returning the colorValues. I initialize the alpha to 1.

>> Source/WebCore/css/parser/CSSParser.cpp:7831
>> +        && value.function->args->size()
> 
> Seems slightly peculiar to check this here rather than inside parseColorFunctionParameters. In the other cases above we are checking for an exact size, but here we are doing part of the size checking; better to do none.

Fixed.

>> Source/WebCore/css/parser/CSSParser.cpp:7835
>> +        ColorSpace colorSpace = ColorSpaceSRGB;
> 
> No need to use a local variable for this.

This is gone now that we return an Optional std::pair.

>> Source/WebCore/css/parser/CSSParser.cpp:7838
>> +        return Color(colorValues[0], colorValues[1], colorValues[2], colorValues[3], colorSpace);
> 
> I would prefer the Color { } syntax to the Color() syntax. Same arguments would work.

It seems that { } is not willing to convert from double to float, where the () syntax is.

I wonder if ExtendedColor should use double anyway.

>> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:469
>> +    ColorSpace colorSpace;
> 
> I suggest declaring this right before the switch instead of up here.

Done.

>> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:470
>> +    Color result;
> 
> Unused.

Removed.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:491
> +        else
> +            colorChannels[i] = 0;

I also now initialize colorChannels so I don't need to do this. (I do break the loop though).

>> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:495
>> +        RefPtr<CSSPrimitiveValue> alphaParameter = consumePercent(args, ValueRangeAll);
> 
> Use auto here?

Changed.

>> Source/WebCore/platform/graphics/cg/ColorCG.cpp:130
>> +        }
> 
> It would be better to cover all values for ColorSpace, and no default case, so this fails to compile if we add a new color space.

Done.

[Dean Jackson]

Committed r208116: <http://trac.webkit.org/changeset/208116>

[Darin Adler]

The API tests seem to be failing on the dashboard.