Bug 214082 – Part 3 of SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them

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

[Sam Weinig]

Part 3 of SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them

[Sam Weinig]

Created attachment 403782 [details]
Patch

[Darin Adler]

Comment on attachment 403782 [details]
Patch

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

SRGBA<uint8_t> sure is more explicit, and also *much* less human readable than "simple color".

> Source/WebCore/ChangeLog:8
> +        - Replaces all uses of SimpleColor that are not implementation details of Color, with SRGBA<uint8_t>.

Why is SimpleColor needed as an implementation detail of Color?

> Source/WebCore/css/parser/CSSParserFastPaths.h:39
>  class CSSValue;
> -class SimpleColor;
> +template<typename> struct SRGBA;

We typically use blank lines, to separate paragraphs, between a paragraph of forward-declared classes and a paragraph of forward-declared struct templates. Might even nice to have a canonical order.

> Source/WebCore/platform/graphics/Color.h:57
>      Color() { }

= default

> Source/WebCore/platform/graphics/Color.h:199
> +    Color(SimpleColor color)
> +        : Color(color.asSRGBA<uint8_t>())
> +    {
> +    }

This is yet another, but multi-line function bodies really detract from the exposition qualities of a class definition.

> Source/WebCore/platform/graphics/ColorBlending.cpp:123
> -    return makeSimpleColor(
> +    return clampToComponentBytes<SRGBA>(

This seems unfortunate. After interpolating uint8_t values then we should not need to clamp.

> Source/WebCore/platform/graphics/ColorBuilder.h:33
> +template<typename ColorType>
> +struct ColorBuilder : public ColorType {

Put on a single line? Omit explicit "public" since this is a struct?

> Source/WebCore/platform/graphics/ColorBuilder.h:44
> +    constexpr ColorBuilder colorWithAlpha(uint8_t overrideAlpha) const

Not sure I like the name, since the starting color might also have alpha. Might need the name "override" in the function, or could go even more terse for this "builder" idiom and just name this "alpha", "withAlpha", "setAlpha" or something nutty like that.

> Source/WebCore/platform/graphics/SimpleColor.h:35
>  class SimpleColor {

I think this class’s days are numbered!

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2557
> -    return showingBorders ? makeSimpleColor(255, 122, 251) : Color();
> +    return showingBorders ? makeSimpleColor(255, 122, 251) : Color { };

Gratuitous!

[Darin Adler]

One of my goals was to eventually wean WebKit off the gratuitous features of Color:

- built-in alternate transparent black null value, with yet another idiom for nullity ("is valid")
    - so annoying that we have nullptr, nullopt, null string, and other nulls all with their own strategies
- two non-equal ways to represent the identical color, simple and extended, for all simple colors
- <new> special "semantic" bit
- unpredictable which algorithms preserve color space, precision, semantic-ness, and nullity, and which ones do not, clamping to SRGBA<uint8_t>
- can't be constexpr because of reference counting

I wanted SimpleColor to be attractive because I was thinking that there was a substantial part of the code that is manipulating SRGBA<uint8_t> and even SRGB<uint8_t>. I figured we’d use Optional<SRGBA<uint8_t>> in some places instead of Color, and so I wanted it to have an attractive name.

I also wanted to minimize the number of times one would need to use Color just to access some useful algorithm like serialization or parsing.

Just wanted to reiterate those thoughts/goals in case they still have effect on our path forward.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 403782 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403782&action=review
> 
> SRGBA<uint8_t> sure is more explicit, and also *much* less human readable
> than "simple color".

My latest thinking here is that this might end up with:

using SimpleColor = SRGBA<uint8_t>;

after all the dust settles.

> 
> > Source/WebCore/ChangeLog:8
> > +        - Replaces all uses of SimpleColor that are not implementation details of Color, with SRGBA<uint8_t>.
> 
> Why is SimpleColor needed as an implementation detail of Color?

It's not. It was just a good checkpoint along the way to avoid the change ballooning.

> 
> > Source/WebCore/css/parser/CSSParserFastPaths.h:39
> >  class CSSValue;
> > -class SimpleColor;
> > +template<typename> struct SRGBA;
> 
> We typically use blank lines, to separate paragraphs, between a paragraph of
> forward-declared classes and a paragraph of forward-declared struct
> templates. Might even nice to have a canonical order.
> 
> > Source/WebCore/platform/graphics/Color.h:57
> >      Color() { }
> 
> = default

Done.

> 
> > Source/WebCore/platform/graphics/Color.h:199
> > +    Color(SimpleColor color)
> > +        : Color(color.asSRGBA<uint8_t>())
> > +    {
> > +    }
> 
> This is yet another, but multi-line function bodies really detract from the
> exposition qualities of a class definition.

It will go away in the next change. Really need to remove all the multiline function bodies from Color.

> 
> > Source/WebCore/platform/graphics/ColorBlending.cpp:123
> > -    return makeSimpleColor(
> > +    return clampToComponentBytes<SRGBA>(
> 
> This seems unfortunate. After interpolating uint8_t values then we should
> not need to clamp.

The WebCore::blend it calls currently uses/returns ints, and since progress can be greater than 1 (and probably negative), this can lead to it returning values greater than 255 (or less than 0), hence the clamping. makeSimpleColor did as well. We probably want to have a WebCore::blend overload for uint8_t that properly saturates rather than overflow.

> 
> > Source/WebCore/platform/graphics/ColorBuilder.h:33
> > +template<typename ColorType>
> > +struct ColorBuilder : public ColorType {
> 
> Put on a single line? Omit explicit "public" since this is a struct?

Erk, how do I keep messing this one up!

> 
> > Source/WebCore/platform/graphics/ColorBuilder.h:44
> > +    constexpr ColorBuilder colorWithAlpha(uint8_t overrideAlpha) const
> 
> Not sure I like the name, since the starting color might also have alpha.
> Might need the name "override" in the function, or could go even more terse
> for this "builder" idiom and just name this "alpha", "withAlpha", "setAlpha"
> or something nutty like that.

I liked the name you used in a earlier change. It was either colorWithOverrideAlpha or colorWithOverriddenAlpha (I somewhat prefer the later). But, since this was added to avoid churn in this change, I am going to keep it matching SimpleColor for now, and follow up with better names. 

> 
> > Source/WebCore/platform/graphics/SimpleColor.h:35
> >  class SimpleColor {
> 
> I think this class’s days are numbered!

Oh yes. 

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2557
> > -    return showingBorders ? makeSimpleColor(255, 122, 251) : Color();
> > +    return showingBorders ? makeSimpleColor(255, 122, 251) : Color { };
> 
> Gratuitous!

The function right above it looks like this:

static Color contentsLayerDebugBorderColor(bool showingBorders)
{
    return showingBorders ? makeSimpleColor(0, 0, 128, 180) : Color { };
}

I couldn't leave them not matching!

Thanks for the review.

[Sam Weinig]

Created attachment 403809 [details]
Patch

[EWS]

Committed r264136: <https://trac.webkit.org/changeset/264136>

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

[Radar WebKit Bug Importer]

<rdar://problem/65242137>