Bug 238208 – Add css-typed-om color IDL skeletons

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

[Alex Christensen]

Add css-typed-om color IDL skeletons

[Alex Christensen]

Created attachment 455393 [details]
Patch

[Alex Christensen]

Created attachment 455401 [details]
Patch

[Alex Christensen]

Created attachment 455433 [details]
Patch

[Sam Weinig]

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

[Alex Christensen]

Created attachment 455548 [details]
Patch

[Sam Weinig]

Comment on attachment 455548 [details]
Patch

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

> Source/WebCore/css/typedom/color/CSSColor.h:42
> +    void setAlpha(CSSNumberish&& alpha) { m_alpha = WTFMove(alpha); }

You don't need (or I would argue want) the && here. (Same for all the other uses outside of the template create use case. In general, most places really don't require it).

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

Missing newline.

[Alex Christensen]

Created attachment 455772 [details]
Patch

[EWS]

Committed r291867 (248873@main): <https://commits.webkit.org/248873@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/90846706>