[webkit-reviews] review granted: [Bug 212396] Extended Color: Experiment with strongly typed ColorComponents : [Attachment 401786] 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 212396: Extended Color: Experiment with strongly typed ColorComponents
https://bugs.webkit.org/show_bug.cgi?id=212396

Attachment 401786: Patch

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




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

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

> Source/WebCore/platform/graphics/Color.cpp:245
> +    return { ColorSpace::SRGB,
asColorComponents(asSimple().asSRGBA<float>()) };

my head is spinning about to vs. as

> Source/WebCore/platform/graphics/ColorTypes.h:33
> +template<typename T>
> +struct SRGBA {

I always prefer these on the same line, but I guess I haven’t convinced you
yet.

> Source/WebCore/platform/graphics/ColorUtilities.cpp:156
> +    // NOTE: This is the equivilent of `toXYZ(toLinearSRGBA(color)).y`

spelling error I noticed in the existing comment here: "equivilent". Also the
use of backquote here ... ?

> Source/WebCore/platform/graphics/ColorUtilities.h:28
> +#include "ColorTypes.h"

Seems like this dependency could be satisfied with a forward declaration?

> Source/WebCore/platform/graphics/ExtendedColor.h:30
> +#include "ColorTypes.h"

Seems like this dependency could be satisfied with a forward declaration?

> Source/WebCore/platform/graphics/SimpleColor.h:76
> -    constexpr ColorComponents<float> asSRGBFloatComponents() const
> +    template<typename T>
> +    constexpr SRGBA<T> asSRGBA() const
>      {
> -	   return { convertToComponentFloat(redComponent()),
convertToComponentFloat(greenComponent()),
convertToComponentFloat(blueComponent()), 
convertToComponentFloat(alphaComponent()) };
> +	   if constexpr (std::is_floating_point_v<T>)
> +	       return { convertToComponentFloat(redComponent()),
convertToComponentFloat(greenComponent()),
convertToComponentFloat(blueComponent()), 
convertToComponentFloat(alphaComponent()) };
> +	   else
> +	       return { redComponent(), greenComponent(), blueComponent(),
alphaComponent() };
>      }

Consider moving these longer function bodies out of the class definition?

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:160
> +	       return 1.0f - (oneMinusAmount + component * (m_amount -
oneMinusAmount));

I find this math truly mysterious, but at least it’s not changing.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:167
> +	       return std::clamp<float>(intercept + m_amount * component, 0.0f,
1.0f);

Surprised that <float> is needed here.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:173
> +	       return std::max<float>(m_amount * component, 0.0f);

Surprised that <float> is needed here.

> Source/WebCore/platform/graphics/filters/FilterOperation.cpp:225
> +bool InvertLightnessFilterOperation::transformColor(SRGBA<float>& color)
const

So sad that these use bool plus out argument. Obviously we want these to be
Optional instead some day.