[webkit-reviews] review granted: [Bug 221880] Add experimental support for CSS Color 5 Relative Color Syntax : [Attachment 420775] 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 221880: Add experimental support for CSS Color 5 Relative Color Syntax
https://bugs.webkit.org/show_bug.cgi?id=221880

Attachment 420775: Patch

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




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

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

> Source/WebCore/ChangeLog:19
> +	   Some additional caveates about this initial implementation that need
further 

Typo in caveats here.

> Source/WebCore/css/parser/CSSParserContext.h:133
> +	       & key.mode				       << 20; // Keep
this last.

That (non-new) comment is mysterious. Why do we need to keep it last?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:925
> +    // and clamp at the end. It won't make a difference until we support
calculations on the origins

Should put a question mark here. Also "origins" should be "origin's".

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:926
> +    // components.

I see that you have orphaned a word. How do you expect me to sleep at night?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1057
> +    return convertColor<SRGBA<uint8_t>>(HSLA<float> {
static_cast<float>(resolvedHue), static_cast<float>(resolvedSaturation),
static_cast<float>(resolvedLightness), static_cast<float>(resolvedAlpha) });

Are these casts all narrowing from double? Or is there something else going on
here? I see this pattern repeatedly and I would like us to improve the idiom.