Bug 235071 – CSS Gradients: interpolation mode should default to OKLab if any non-legacy color syntax colors are used in the stops

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

[Sam Weinig]

CSS Gradients: interpolation mode should default to OKLab if any non-legacy color syntax colors are used in the stops

[Sam Weinig]

Created attachment 448862 [details]
Patch

[Sam Weinig]

Created attachment 448877 [details]
Patch

[Sam Weinig]

Created attachment 449257 [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 449259 [details]
Patch

[Sam Weinig]

Created attachment 449262 [details]
Patch

[Darin Adler]

Comment on attachment 449262 [details]
Patch

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

> Source/WebCore/css/CSSGradientValue.cpp:651
> +        && m_defaultColorInterpolationMethod == other.m_defaultColorInterpolationMethod

Gosh can’t wait until we get enough C++20 support that we can stop writing these functions.

> Source/WebCore/css/CSSGradientValue.h:72
> -
> +    

Please don’t add the spaces here.

> Source/WebCore/css/CSSGradientValue.h:143
> +    static Ref<CSSLinearGradientValue> create(CSSGradientRepeat repeat, CSSGradientType gradientType, ColorInterpolationMethod colorInterpolationMethod, CSSGradientDefaultColorInterpolationMethod defaultColorInterpolationMethod, CSSGradientColorStopList stops)

Frustrating how long these argument names and types are.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3132
> +        downcast<CSSRadialGradientValue>(result.get())->setFirstRadius(WTFMove(firstRadius));
> +        downcast<CSSRadialGradientValue>(result.get())->setSecondRadius(WTFMove(secondRadius));

Can use (*result). here instead of (result.get())->

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3567
> +    auto [computedColorInterpolationMethod, computedDefaultColorInterpolationMethod] = computeGradientColorInterpolationMethod(context, colorInterpolationMethod, *stops);

Reading this over and over again makes me wonder why we don’t pass these two around together as a struct.

[Sam Weinig]

Created attachment 449280 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 449262 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449262&action=review
> 
> 
> > Source/WebCore/css/CSSGradientValue.h:72
> > -
> > +    
> 
> Please don’t add the spaces here.

Fixed.

> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3132
> > +        downcast<CSSRadialGradientValue>(result.get())->setFirstRadius(WTFMove(firstRadius));
> > +        downcast<CSSRadialGradientValue>(result.get())->setSecondRadius(WTFMove(secondRadius));
> 
> Can use (*result). here instead of (result.get())->

Fixed.

> > Source/WebCore/css/CSSGradientValue.h:143
> > +    static Ref<CSSLinearGradientValue> create(CSSGradientRepeat repeat, CSSGradientType gradientType, ColorInterpolationMethod colorInterpolationMethod, CSSGradientDefaultColorInterpolationMethod defaultColorInterpolationMethod, CSSGradientColorStopList stops)
> 
> Frustrating how long these argument names and types are.
> 
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3567
> > +    auto [computedColorInterpolationMethod, computedDefaultColorInterpolationMethod] = computeGradientColorInterpolationMethod(context, colorInterpolationMethod, *stops);
> 
> Reading this over and over again makes me wonder why we don’t pass these two
> around together as a struct.

Yeah, I agree. Fixed.

[EWS]

Committed r288071 (246091@main): <https://commits.webkit.org/246091@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/87645870>