[webkit-reviews] review granted: [Bug 214082] Part 3 of SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them : [Attachment 403782] 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 214082: Part 3 of SimpleColor and SRGBA<uint8_t> are essentially the same -
let's converge them
https://bugs.webkit.org/show_bug.cgi?id=214082

Attachment 403782: Patch

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




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

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

SRGBA<uint8_t> sure is more explicit, and also *much* less human readable than
"simple color".

> Source/WebCore/ChangeLog:8
> +	   - Replaces all uses of SimpleColor that are not implementation
details of Color, with SRGBA<uint8_t>.

Why is SimpleColor needed as an implementation detail of Color?

> Source/WebCore/css/parser/CSSParserFastPaths.h:39
>  class CSSValue;
> -class SimpleColor;
> +template<typename> struct SRGBA;

We typically use blank lines, to separate paragraphs, between a paragraph of
forward-declared classes and a paragraph of forward-declared struct templates.
Might even nice to have a canonical order.

> Source/WebCore/platform/graphics/Color.h:57
>      Color() { }

= default

> Source/WebCore/platform/graphics/Color.h:199
> +    Color(SimpleColor color)
> +	   : Color(color.asSRGBA<uint8_t>())
> +    {
> +    }

This is yet another, but multi-line function bodies really detract from the
exposition qualities of a class definition.

> Source/WebCore/platform/graphics/ColorBlending.cpp:123
> -    return makeSimpleColor(
> +    return clampToComponentBytes<SRGBA>(

This seems unfortunate. After interpolating uint8_t values then we should not
need to clamp.

> Source/WebCore/platform/graphics/ColorBuilder.h:33
> +template<typename ColorType>
> +struct ColorBuilder : public ColorType {

Put on a single line? Omit explicit "public" since this is a struct?

> Source/WebCore/platform/graphics/ColorBuilder.h:44
> +    constexpr ColorBuilder colorWithAlpha(uint8_t overrideAlpha) const

Not sure I like the name, since the starting color might also have alpha. Might
need the name "override" in the function, or could go even more terse for this
"builder" idiom and just name this "alpha", "withAlpha", "setAlpha" or
something nutty like that.

> Source/WebCore/platform/graphics/SimpleColor.h:35
>  class SimpleColor {

I think this class’s days are numbered!

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2557
> -    return showingBorders ? makeSimpleColor(255, 122, 251) : Color();
> +    return showingBorders ? makeSimpleColor(255, 122, 251) : Color { };

Gratuitous!