[webkit-reviews] review denied: [Bug 238208] Add css-typed-om color IDL skeletons : [Attachment 455433] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 238208: Add css-typed-om color IDL skeletons
https://bugs.webkit.org/show_bug.cgi?id=238208

Attachment 455433: Patch

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




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 455433
  --> https://bugs.webkit.org/attachment.cgi?id=455433
Patch

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

> Source/WebCore/ChangeLog:10
> +

This should be more filled out and probably include a link to the spec.

> Source/WebCore/ChangeLog:108
> +	   * css/typedom/color/CSSOKLab.idl: Copied from
Source/WebCore/css/typedom/CSSKeywordValue.idl.

Not sure these "Copied from ..." texts are useful.

> Source/WebCore/css/typedom/color/CSSColor.h:37
> +    static Ref<CSSColor> create(CSSKeywordish&& colorSpace,
Vector<CSSColorPercent>&& channels, CSSNumberish&& alpha)

I don't think there is much value in making these explicitly r-values. I prefer
the patter of making the parameters normal values, and moving into them. This
gives more flexibility for the same level of performance. Same for the
constructor and setters.

> Source/WebCore/css/typedom/color/CSSColor.idl:30
> +[

I've been trying to encourage linking to the IDL in the spec for each IDL file.

> Source/WebCore/css/typedom/color/CSSColor.idl:31
> +    EnabledAtRuntime=CSSTypedOMEnabled,

Trying to remove EnabledAtRuntime (since it is a singleton) so this should use
EnabledBySettings instead (same for all the others).

> Source/WebCore/css/typedom/color/CSSColor.idl:38
> +    // FIXME: Add support for ObservableArray and add this.

Worth adding a bugzilla URL for this.

> Source/WebCore/css/typedom/color/CSSColorValue.h:45
> +    RefPtr<CSSKeywordValue> colorSpace() { return nullptr; }
> +    RefPtr<CSSColorValue> to(CSSKeywordish&&) { return nullptr; }

Are these correct? Or still to be implemented?

> Source/WebCore/css/typedom/color/CSSHSL.cpp:41
> +    , m_alpha(WTFMove(alpha)) { }

Our style usually puts the { } on separate lines like:

{
}

> Source/WebCore/css/typedom/color/CSSHSL.h:47
> +    void setAlpha(CSSColorPercent&& alpha) { m_alpha = WTFMove(alpha); }
> +private:

Please add a newline between these.

> Source/WebCore/css/typedom/color/CSSHWB.h:52
> +    Ref<CSSNumericValue> m_h;
> +    CSSNumberish m_w;
> +    CSSNumberish m_b;

In the other classes, you have used the full name for the members, so these
should probably be m_hue, m_whiteness, m_blackness.

> Source/WebCore/css/typedom/color/CSSLCH.h:37
> +    template<typename... Args> static Ref<CSSLCH> create(Args&&... args) {
return adoptRef(*new CSSLCH(std::forward<Args>(args)...)); }

For this one, using && is correct, since this is not an r-value, but rather a
"universal reference" since Args is a template parameter.

> Source/WebCore/css/typedom/color/CSSLCH.h:50
> +    CSSColorPercent m_luminance;

This should be called m_lightness (that is what it is called for
Lab/LCH/OKLab/OKLCH)

> Source/WebCore/css/typedom/color/CSSOKLCH.h:50
> +    CSSColorPercent m_luminance;

This should be called m_lightness (that is what it is called for
Lab/LCH/OKLab/OKLCH)

> Source/WebCore/css/typedom/color/CSSRGB.h:54
> +    CSSColorRGBComp m_r;
> +    CSSColorRGBComp m_g;
> +    CSSColorRGBComp m_b;

I would rename these to m_red, m_green, m_blue to match the other classes
members.