[webkit-reviews] review granted: [Bug 233507] [CSS Color 4] Add support for oklab() and oklch() colors : [Attachment 445162] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 233507: [CSS Color 4] Add support for oklab() and oklch() colors
https://bugs.webkit.org/show_bug.cgi?id=233507

Attachment 445162: Patch

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




--- Comment #3 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 445162
  --> https://bugs.webkit.org/attachment.cgi?id=445162
Patch

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

I didn't review the tests closely.

> Source/WebCore/ChangeLog:25
> +	   Adds support for oklab() and oklch() css colors and as interpolation

"CSS colors"

> Source/WebCore/ChangeLog:56
> +	   (WebCore::N>::subset const):

(prepare-ChangeLog got confused here.)

> Source/WebCore/ChangeLog:97
> +	   Add OKLab and OKLCH to the list fo enumerated colorspaces and add
mappings to their

"list of"

> Source/WebCore/ChangeLog:109
> +	   Generalize isBlack and isWhite to have variant that works with Lab,
LCH, OKLab and OKLCH (as they

"to have a variant that works" or "to have variants that work"

> Source/WebCore/platform/graphics/ColorConversion.cpp:294
> +    // matrix with any subsequent matricies in the conversion. This would
mean teaching the main conversion about this matrix

"matrices"

> Source/WebCore/platform/graphics/ColorConversion.cpp:303
> +    static constexpr ColorMatrix<3, 3> LMSToXYZD65 {
> +	   1.2268798733741557f,  -0.5578149965554813f,	0.28139105017721583f,
> +	  -0.04057576262431372f,  1.1122868293970594f, -0.07171106666151701f,
> +	  -0.07637294974672142f, -0.4214933239627914f,	1.5869240244272418f
> +    };

Looks funny to write out these numbers with enough precision to be doubles but
actually be floats. But I guess it's better to have these transcribed from the
spec than rounded by hand.

Also, maybe the matrix names could be clearer about whether they are linear or
non-linear LMS?

> Source/WebCore/platform/graphics/ColorConversion.cpp:330
> +    // matrix with any previous matricies in the conversion. This would mean
teaching the main conversion about this matrix

"matricies"

> Source/WebCore/platform/graphics/ColorConversion.cpp:344
> +	   0.2104542553f,  0.7936177850f, -0.0040720468f,
> +	   1.9779984951f, -2.4285922050f,  0.4505937099f,
> +	   0.0259040371f,  0.7827717662f, -0.8086757660f

Why does this matrix have less precision (here and in the spec)? Doesn't matter
for us since we're using floats, but it looks odd.

> Source/WebCore/platform/graphics/ColorTypes.h:482
> +    static constexpr WhitePoint whitePoint = WhitePoint::D65;

This could be `static constexpr auto`, like how
ColorSpaceMapping<T>::colorSpace is defined.

> Source/WebCore/platform/graphics/ColorTypes.h:483
> +    using Reference =  XYZA<T, whitePoint>;

Nit: one too many spaces after "=".

> Source/WebCore/platform/graphics/ColorUtilities.h:57
> +template<typename ColorType, typename
std::enable_if_t<std::is_same_v<typename ColorType::Model, LabModel<typename
ColorType::ComponentType>> || std::is_same_v<typename ColorType::Model,
LCHModel<typename ColorType::ComponentType>>>* = nullptr> constexpr bool
isBlack(const ColorType&);

Maybe define `IsLabModel` and `IsLCHModel` template variables to make this
expression (which is repeated a few times) more readable?

> LayoutTests/fast/css/parsing-color-mix.html:75
> +	   // What should happen if you provide a negative percent?
https://github.com/w3c/csswg-drafts/issues/6047

That issue is resolved now. I think the test for negative percentages is
correct?