[webkit-reviews] review granted: [Bug 236200] Conversion to a color space with a smaller gamut should perform gamut mapping : [Attachment 451051] 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 236200: Conversion to a color space with a smaller gamut should perform
gamut mapping
https://bugs.webkit.org/show_bug.cgi?id=236200

Attachment 451051: Patch

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




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

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

r=me but I assume you’ll be looking into the test failure on gtk-wk2

> Source/WebCore/platform/graphics/ColorConversion.cpp:44
>      float hue = rad2deg(atan2(b, a));

This converts to double and then back to float. Maybe we should use
std::rad2deg and std::atan2 to keep things as float?

> Source/WebCore/platform/graphics/ColorConversion.h:162
> +    template<typename ColorType> static auto mapToBoundedGamut(const
ColorType& color) -> typename ColorType::BoundedCounterpart

Do you need the "->" part? Maybe auto can deduce the type from the return value
in the function without it?

> Source/WebCore/platform/graphics/ColorConversion.h:180
> +    static constexpr const float JND = 0.02f;

No need for "const" here after "constexpr".

> Source/WebCore/platform/graphics/ColorConversion.h:198
> +	   if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness,
100.0f) || colorInOKLCHColorSpace.lightness > 100.0f)
> +	       return { 1.0, 1.0, 1.0, resolvedColor.alpha };
> +	   else if (WTF::areEssentiallyEqual(colorInOKLCHColorSpace.lightness,
0.0f))
> +	       return { 0.0f, 0.0f, 0.0f, resolvedColor.alpha };

Surprised at the need for the "WTF::" prefix here.