[webkit-reviews] review granted: [Bug 212059] Simplify Color interface by replacing Color::getRGBA() with a variant returning a FloatComponents : [Attachment 399700] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 212059: Simplify Color interface by replacing Color::getRGBA() with a
variant returning a FloatComponents
https://bugs.webkit.org/show_bug.cgi?id=212059

Attachment 399700: Patch

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




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

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

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:154
> +    if (from.color.isExtended())
> +	   fromComponents = from.color.asExtended().channels();
> +    else
> +	   fromComponents = from.color.rgba();

Should make rgba() just work for extended colors, as long as they are sRGB.
This code should really ask about the colorspace, not about the isExtended.
Maybe just call toSRGBAComponentsLossy().

> Source/WebCore/platform/graphics/cairo/GradientCairo.cpp:165
>      float r = r1 + (r2 - r1) * 0.5f;

Maybe we should just put blending into FloatCompontents.

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

Broken with extended colors (not your fault). Call toSRGBAComponentsLossy()?

> Source/WebCore/platform/graphics/filters/FilterOperations.cpp:114
> +    FloatComponents components = color.rgba();

Broken with extended colors (not your fault). Call toSRGBAComponentsLossy()?

> Source/WebCore/platform/graphics/filters/FilterOperations.cpp:133
> +    FloatComponents components = color.rgba();

Ditto.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:256
> +    auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).rgba();

Would be nicer to do the premultiply on FloatComponents.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:281
> +	   auto [r, g, b, a] = color.rgba();

toSRGBAComponentsLossy?

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:421
> +	       auto [r, g, b, a] =
Color(premultipliedARGBFromColor(shadow.color())).rgba();

Would be nicer to do the premultiply on FloatComponents.

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:683
> +    auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).rgba();

Would be nicer to do the premultiply on FloatComponents.

> Source/WebCore/rendering/TextPaintStyle.cpp:66
> +    return contrastRatio(textColor.rgba(), backgroundColor.rgba()) > 4.5;

Broken with extended colors.

> Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp:85
> +    auto [r, g, b, a] = webCoreColor.rgba().components;

Broken with extended colors.

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

Broken with extended colors.