Bug 226513 – Add support for "relative color syntax" for color()

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Add support for "relative color syntax" for color()

[Sam Weinig]

Created attachment 430296 [details]
Patch

[Sam Weinig]

Created attachment 430312 [details]
Patch

[Darin Adler]

Comment on attachment 430312 [details]
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!

[Sam Weinig]

Created attachment 430356 [details]
Patch

[Sam Weinig]

Created attachment 430362 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 430312 [details]
> 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?

Nope. Great idea. The template parameter and the assertion were added at different times and I didn't put them together. Done.

> 
> > 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!

Totally agree. Added.

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

Done.

> 
> > 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.

Done.

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

Done.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1932
> > +        if (!value)
> > +            break;
> 
> All unspecified channels are set to 0? Peculiar syntax!

So odd. It has to do with the fact that a custom profile can have any number of components, so it just defaults everything to 0, but I think they should just special case the predefined ones to require the right number of components, but I think I lost that argument.

[Sam Weinig]

Created attachment 430363 [details]
Patch

[EWS]

Committed r278364 (238394@main): <https://commits.webkit.org/238394@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430363 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78771441>