Bug 238208

Created attachment 455393 [details] Patch

Created attachment 455401 [details] Patch

Created attachment 455433 [details] Patch

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.

Created attachment 455548 [details] Patch

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.

Created attachment 455772 [details] Patch

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].

<rdar://problem/90846706>