[webkit-reviews] review granted: [Bug 235071] CSS Gradients: interpolation mode should default to OKLab if any non-legacy color syntax colors are used in the stops : [Attachment 449262] 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 235071: CSS Gradients: interpolation mode should default to OKLab if any
non-legacy color syntax colors are used in the stops
https://bugs.webkit.org/show_bug.cgi?id=235071

Attachment 449262: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 449262
  --> https://bugs.webkit.org/attachment.cgi?id=449262
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(firstRad
ius));
> +	  
downcast<CSSRadialGradientValue>(result.get())->setSecondRadius(WTFMove(secondR
adius));

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.