Bug 235496 – Support interpolating colors with missing/none components via color-mix()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Support interpolating colors with missing/none components

[Sam Weinig]

Created attachment 449756 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Sam Weinig]

Created attachment 449757 [details]
Patch

[Darin Adler]

Comment on attachment 449757 [details]
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.

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 449757 [details]
> 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.

Fixed.

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

Fixed.

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

I'll do this in a follow up. Do you have a preferred naming convention? I kind of prefer "Constexpr" over "ConstExpr" just because the name of the keyword is all the same case, but I don't feel strongly.

[Sam Weinig]

Created attachment 449769 [details]
Patch

[Darin Adler]

(In reply to Sam Weinig from comment #5)
> I'll do this in a follow up. Do you have a preferred naming convention? I
> kind of prefer "Constexpr" over "ConstExpr" just because the name of the
> keyword is all the same case, but I don't feel strongly.

I thought it was a good choice that you put constexpr first in the name of your function because it could be recognizably the keyword since it's still all lowercase. Sadly there are other disadvantages to that approach; leaving "constexpr" unmolested requires capitalizing the function name, which won’t work well for "ctz".

I slightly prefer ConstExpr; neither Constexpr nor ConstExpr looks right to me, but the latter looks more correct (admit this is wholly subjective).

[Darin Adler]

We also change the function name to our style, so is_sorted becomes isSorted. Can imagine it being is_sorted_constexpr, but this way lies madness?

[Darin Adler]

Hey, I’m also curious what your thoughts are on the other half of my comments, the ones you didn’t respond to:

- third template argument
- interpolateComponetWithoutAccountingForNaN

Not insisting on anything, but would love to know what you think about those two.

[EWS]

Committed r288427 (246318@main): <https://commits.webkit.org/246318@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449769 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87947990>

[Darin Adler]

Oh no, now I see you took my suggestion and used my name:

    interpolateComponetWithoutAccountingForNaN

Typo, and all! I wrote componet!