[webkit-reviews] review granted: [Bug 226513] Add support for "relative color syntax" for color() : [Attachment 430312] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 226513: Add support for "relative color syntax" for color()
https://bugs.webkit.org/show_bug.cgi?id=226513

Attachment 430312: Patch

https://bugs.webkit.org/attachment.cgi?id=430312&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 430312
  --> https://bugs.webkit.org/attachment.cgi?id=430312
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1566
> +    ASSERT(range.peek().functionId() == CSSValueRgb ||
range.peek().functionId() == CSSValueRgba);

Wouldn’t we want the more precise assertion?

    ASSERT(range.peek().functionId() == (Mode == RGBFunctionMode::RGB ?
CSSValueRgb : CSSValueRgba));

Or is my assertion wrong?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1583
> +    return convertColor<SRGBA<uint8_t>>(HSLA<float> {
static_cast<float>(normalizedHue), static_cast<float>(normalizedSaturation),
static_cast<float>(normalizedLightness), static_cast<float>(normalizedAlpha)
});

This explicit conversion to SRGBA really does need a comment. I know it’s not
new. The fact that we do not create an extended color, but instead round and
clamp to SRGBA intentionally really does not go without saying!

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1659
> +    ASSERT(range.peek().functionId() == CSSValueHsl ||
range.peek().functionId() == CSSValueHsla);

Same thought about the assertion.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1720
> +    return convertColor<SRGBA<uint8_t>>(HWBA<float> {
static_cast<float>(normalizedHue), static_cast<float>(normalizedWhitness),
static_cast<float>(normalizedBlackness), static_cast<float>(*alpha) });

Same thought about the comment.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1929
> +    for (int i = 0; i < 3; ++i) {

for (auto& channel : channels) {

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1932
> +	   if (!value)
> +	       break;

All unspecified channels are set to 0? Peculiar syntax!