[webkit-reviews] review granted: [Bug 235496] Support interpolating colors with missing/none components via color-mix() : [Attachment 449757] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 23 11:55:55 PST 2022

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 235496: Support interpolating colors with missing/none components via

Attachment 449757: Patch


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

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

> Source/WebCore/platform/graphics/ColorInterpolation.h:37
> +template<AlphaPremultiplication alphaPremultiplication, typename

I suggest you omit the template argument name "alphaPremultiplication" here.

Also, if you wanted to make the next line of code way shorter, maybe you could
add ColorType as a third template argument with a default value of typename
InterpolationMethodColorSpace::ColorType? No caller would ever specify that
argument explicitly, so no actual code bloat, but it would make the argument
list below easier to read. Not sure it would work.

> Source/WebCore/platform/graphics/ColorInterpolation.h:47
> +// MARK: - Premultiplication agnostic interpolation helpers.

I think a hyphen is needed in the phrase "premultiplication-agnostic".

> Source/WebCore/platform/graphics/ColorInterpolation.h:65
> +float interpolateHue(InterpolationMethodColorSpace
interpolationMethodColorSpace, float componentFromColor1, double
color1Multiplier, float componentFromColor2, double color2Multiplier)

Why is it that interpolateComponent does not account for NaN by default, but
interpolateHue does, and so do many other functions below? Maybe
interpolateComponent should be named

> Source/WebCore/platform/graphics/ColorTypes.h:136
>  template<typename ComponentType>
>  constexpr bool constexprIsNaN(ComponentType value)

Probably should move this to MathExtras.h.

It seems like roughly speaking this is in a family with clzConstexpr,
ctzConstexpr, getLSBSetConstexpr, and getMSBSetConstexpr, in MathExtras.h,
strcmpConstExpr, in SortedArrayMap.h, and isSortedConstExpr and allOfConstExpr
in StdLibExtras.h.

We have three naming schemes here, and some functions that are in the Extras
headers, but others that are not.

More information about the webkit-reviews mailing list