Bug 223901 – Generalize ColorComponents to support an arbritrary number of components (though really we will only ever want 3, 4, or 5)

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Generalize ColorComponents to support an arbritrary number of components (though really we will only ever want 3, 4, or 5)

[Sam Weinig]

Created attachment 424571 [details]
Patch

[Darin Adler]

Comment on attachment 424571 [details]
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?

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 424571 [details]
> 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.

I keep wavering on this one. I went in pretty hard for it in ColorTypes.h, but sometimes it just looks weird.

    template<size_t I>
    constexpr T get() const;

    template<size_t Start, size_t End>
    constexpr ColorComponents<T, End - Start> subset() const;

vs

    template<size_t I> constexpr T get() const;

    template<size_t Start, size_t End> constexpr ColorComponents<T, End - Start> subset() const;

I think maybe it's the return types not lining up? There is also the problem of template functions in template classes, like:

template<typename T, size_t N>
template<size_t Start, size_t End>
constexpr ColorComponents<T, End - Start> ColorComponents<T, N>::subset() const
{
    ColorComponents<T, End - Start> result;
    for (std::remove_const_t<decltype(T::Size)> i = Start; i < End; ++i)
        result[i - Start] = components[i];
    return result;
}

where splitting the two templates onto separate lines helps (in my view) to clarify the structure.

> 
> > 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?

the point of this class is to avoid ever having color components in a raw std::array, so thankfully that hasn't come up yet.

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

Yeah, but in a follow up this function will be generalized to work with non-4 component ColorComponents, so that will go away.

> 
> > 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?

Yeah, it's required since the constructor is explicit, which I did on purpose to make sure people have to specify which type it is. It's a bit redundant in cases where you are returning, but overall I think being more explicit here is better.

[EWS]

Committed r275206: <https://commits.webkit.org/r275206>

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

[Radar WebKit Bug Importer]

<rdar://problem/76006720>