[webkit-reviews] review granted: [Bug 235496] Support interpolating colors with missing/none components via color-mix() : [Attachment 449757] 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 235496: Support interpolating colors with missing/none components via
color-mix()
https://bugs.webkit.org/show_bug.cgi?id=235496

Attachment 449757: Patch

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




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

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

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

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
interpolateComponetWithoutAccountingForNaN?

> 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.