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

Attachment 403942: Patch

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




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

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

> Source/WebCore/platform/graphics/Color.h:294
> +
> +
> +    if constexpr (std::is_same_v<T, float>)

Excess blank line here. I would consider omitting the blank lines entirely.

> Source/WebCore/platform/graphics/Color.h:296
> +    else if constexpr (std::is_same_v<T, uint8_t>)

No need for else here. Also, it seems logical to me to check uint8_t first,
before float.

> Source/WebCore/platform/graphics/ColorUtilities.h:97
> +template<typename ColorType, typename Functor> ColorType
colorByModifingEachNonAlphaComponent(const ColorType& color, Functor&& functor)

I still prefer a style where we have exposition, declarations of the functions
and what they are for, followed by implementation, function definitions. This
file seems to mix the two and I think we could do better.

Given this calls the function and does not take ownership, would const Functor&
be better than Functor&&?

> Source/WebCore/platform/graphics/ColorUtilities.h:99
> +    // FIXME: This should be made to work with color's that don't use the
names red, green, and blue for their channels.

"color's" -> "colors".

> Source/WebCore/platform/graphics/ColorUtilities.h:104
> +    ColorType copy = color;
> +    copy.red = functor(color.red);
> +    copy.green = functor(color.green);
> +    copy.blue = functor(color.blue);
> +    return copy;

Does this generate efficient code? Seems like colors are first copied and then
overwritten, but maybe the compiler can optimize that away. I Guess there are
other examples below doing the same.

Also, for this and other cases below I suggest:

    auto copy = color;

Instead of writing out ColorType.

> Source/WebCore/platform/graphics/ColorUtilities.h:133
> +    ColorType copy = color;

auto

> Source/WebCore/platform/graphics/ColorUtilities.h:140
> +    ColorType copy = color;

auto

> Source/WebCore/platform/graphics/ColorUtilities.h:153
> +constexpr uint8_t invertComponent(uint8_t component)
> +{
> +    return 255 - component;
> +}
> +
> +constexpr float invertComponent(float component)
> +{
> +    return 1.0f - component;
> +}

This kind of overloading, where numeric values depend on type, still doesn’t
seem quite right to me. What happens with the 255.0f floating point code we
have in some places?

> Source/WebCore/platform/graphics/ColorUtilities.h:157
> +    ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto
component) {

auto

> Source/WebCore/platform/graphics/ColorUtilities.h:166
> +    ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto
component) {

auto

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:54
> +static Optional<SRGBA<uint8_t>> makeSimpleColorFromCGColor(CGColorRef color)

Seems like this function needs a name that better communicates its limitations,
because it:

- Reinterprets color components as either gray scale or sRGBA, based only on
component *count*, ignoring color space. That's a bad bug. I guess that's what
"FIXME: ExtendedColor - needs to handle color spaces" means, but it seems like
an understatement.

- Clamps to the sRGBA gamut, loses anything outside the 0.0-1.0, ruining colors
for HDR. That makes sense, I suppose since the return type is SRGBA<uint8_t>,
but no word "clamp" in the title.

- Rounds color channels to 0-255 integers, losing any precision beyond that.
Makes sense since the return type is SRGBA<uint8_t>.

I suppose "SimpleColor" is saying that? Also, no need for "FromCGColor" in this
function name. This is C++, not C.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:58
>      // FIXME: ExtendedColor - needs to handle color spaces.
> +    if (!color)
> +	   return WTF::nullopt;

Should leave a blank line after the comment since it’s about the whole
function, not the first paragraph.

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:87
> +    : Color(makeSimpleColorFromCGColor(color))

Not obvious to me that this will always be the right way to handle conversion
from all CGColorRef objects. Some might need representation as extended colors.
So probably needs to be a named constructor?

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:92
> +    : Color(makeSimpleColorFromCGColor(color), tag)

Ditto.