[webkit-reviews] review granted: [Bug 212059] Remove almost always incorrect Color::getRGBA : [Attachment 399752] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted  review:
Bug 212059: Remove almost always incorrect Color::getRGBA
https://bugs.webkit.org/show_bug.cgi?id=212059

Attachment 399752: Patch

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




--- Comment #8 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 399752
  --> https://bugs.webkit.org/attachment.cgi?id=399752
Patch

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

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:136
> +	   color = cachedCGColor(SimpleColor { argb });

(Not your fault): It blows my mind that the arguments to SimpleColor are argb.
Seems very Cocoa-centric, and easy to get wrong. Maybe SimpleColor needs at
more scary name like PlatformPixelValue.

> Source/WebCore/platform/graphics/Color.cpp:359
>	   return Color(0x54, 0x54, 0x54, alpha());

Can we just say 84 rather than 0x54?

> Source/WebCore/platform/graphics/Color.cpp:393
> +    auto [r, g, b, a] = toSRGBAComponentsLossy();
> +    float largestNonAlphaChannel = std::max(r, std::max(g, b));
> +    return a > 0.5 && largestNonAlphaChannel < 0.5;

This should really compute luminance. Maybe a // FIXME

> Source/WebCore/platform/graphics/Color.cpp:544
> +    return { ColorSpace::SRGB, FloatComponents { red() / 255.0f, green() /
255.0f, blue() / 255.0f,  alpha() / 255.0f } };

I feel like the / 255.0f should be in a helper function somewhere. We've gone
back and forth several times on rounding/flooring of color components (e.g.
r252598).

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:117
> +	   return CGColorCreate(sRGBColorSpaceRef(), components);

We should use the linearRGBColorSpaceRef here (should fix in a separate bug).

> Source/WebCore/platform/graphics/cg/ColorCG.cpp:134
> +	       static CGColorRef whiteCGColor = leakCGColor(color)

How does this compile?

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:74
> +	   auto [colorSpace, components] = stop.color.colorSpaceAndComponent();
> +	   auto [r, g, b, a] = components;

This needs a FIXME; we drop colorSpace on the floor.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:202
> +    static constexpr SimpleColor spellingPatternColor { makeRGB(255, 0, 0)
};
> +    static constexpr SimpleColor grammarPatternColor { makeRGB(0, 128, 0) };

Ugh, my brain hurts. RGBA32 makeRGBA(), despite the name, returns ARGB which is
the argument to the SimpleColor ctor, so this is OK. I don't know why
SimpleColor doesn't take a RGBA32, and the names all need fixing.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:663
> +    auto [r, g, b, a] = bgColor.toSRGBAComponentsLossy();

Sad that I can't have a P3 frame background.