[webkit-reviews] review granted: [Bug 223901] Generalize ColorComponents to support an arbritrary number of components (though really we will only ever want 3, 4, or 5) : [Attachment 424571] 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 223901: Generalize ColorComponents to support an arbritrary number of
components (though really we will only ever want 3, 4, or 5)
https://bugs.webkit.org/show_bug.cgi?id=223901

Attachment 424571: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 424571
  --> https://bugs.webkit.org/attachment.cgi?id=424571
Patch

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

> Source/WebCore/platform/graphics/ColorComponents.h:66
> +    template<size_t I>
>      constexpr T get() const;

Classic example that I would find more readable on a single line instead of
split across two. But maybe this is a losing battle since so many template
lines get too long if you do that.

> Source/WebCore/platform/graphics/ColorComponents.h:71
> +    std::array<T, N> components;

Funny that this really is "just an array" yet can you even construct it from an
array?

> Source/WebCore/platform/graphics/ColorMatrix.h:125
> +    static_assert(ColorComponents<float, 4>::Size >= RowCount);

Seems like we might need a typedef for ColorComponents<float, 4>. Honestly,
ColorComponents<float, 4>::Size is a funny way to write "4"; with a typedef
short name in this function it might read nicely.

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:98
> +    return PackedColor::RGBA {
makeFromComponents<SRGBA<uint8_t>>(components) };

Do you have to write the type name here? Can’t we just use braces?