Bug 214521 – Remove ColorBuilder

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

[Sam Weinig]

Remove ColorBuilder

[Sam Weinig]

Created attachment 404644 [details]
Patch

[Darin Adler]

Comment on attachment 404644 [details]
Patch

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

> Source/WebCore/platform/graphics/ColorTypes.h:45
> +template<typename Parent> struct ColorType {

Seems like this should be called CanApplyAlpha or AlphaHelper or something. The generic name ColorType doesn't truly express what it currently does. But maybe in future it will do more.

> Source/WebCore/platform/graphics/ColorTypes.h:48
> +    constexpr Parent colorWithAlpha(uint8_t overrideAlpha) const

Because this always takes uint8_t with no overloading, it is dangerous to pass a 1.0 to this function. It will set alpha to 1/255, I think, without necessarily warning.

> Source/WebCore/platform/graphics/ColorTypes.h:53
> +        copy.alpha = overrideAlpha;

Will this do the right thing if the color type’s component type is float?

> Source/WebCore/platform/graphics/ColorTypes.h:106
> +    constexpr LinearSRGBA(T red, T green, T blue, T alpha = ComponentTraits<T>::maxValue)

Super unhappy that we have to write all these constructors.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 404644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404644&action=review
> 
> > Source/WebCore/platform/graphics/ColorTypes.h:45
> > +template<typename Parent> struct ColorType {
> 
> Seems like this should be called CanApplyAlpha or AlphaHelper or something.
> The generic name ColorType doesn't truly express what it currently does. But
> maybe in future it will do more.
> 
> > Source/WebCore/platform/graphics/ColorTypes.h:48
> > +    constexpr Parent colorWithAlpha(uint8_t overrideAlpha) const
> 
> Because this always takes uint8_t with no overloading, it is dangerous to
> pass a 1.0 to this function. It will set alpha to 1/255, I think, without
> necessarily warning.

Yeah, alpha handling and Color are a bit messed up still. The Color class is not even consistent about this yet. 

- Color::alpha() returns 0-255 uint8_t
- Color::alphaAsFloat() returns 0-1 float
- Color::colorWithAlpha()/Color::invertedColorWithAlpha() take alpha as 1-0 float.

My intuition is that the more we can treat alpha as a 0-1 float consistently, the better off we are, but I could be off base. When I proposed a few options for SRGBA<uint8_t> literal syntax to Simon a few days ago, one I suggested was:

SRGBA<uint8_t> = srgba({0-255 uint8_t value},{0-255 uint8_t value},{0-255 uint8_t value}, {0-1 float value})

and he was generally against it, and I ended up going with the following for now:

SRGBA<uint8_t> = SRGBA<uint8_t> { {0-255 uint8_t value},{0-255 uint8_t value},{0-255 uint8_t value}, 0-255 uint8_t value})

That said, perhaps there is some naming that we could do to make the specific case of colorWithAlpha() consistent. For instance, maybe we could a whole different direction and do:

SRGBA<uint8_t>::colorWithAlphaPercentage(0-100 float).

Or, just be okay with the slight inconsistency and make it match Color::colorWithAlpha(), despite being different than the constructor / literal form:

SRGBA<uint8_t>::colorWithAlpha(0-1 float)


> 
> > Source/WebCore/platform/graphics/ColorTypes.h:53
> > +        copy.alpha = overrideAlpha;
> 
> Will this do the right thing if the color type’s component type is float?

No, it won't. But the static_assert above it won't allow that to happen.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:106
> > +    constexpr LinearSRGBA(T red, T green, T blue, T alpha = ComponentTraits<T>::maxValue)
> 
> Super unhappy that we have to write all these constructors.

:(

[Darin Adler]

While considering different interfaces, seems we could name the function colorWithAlphaByte for now. And Color::alpha could be renamed alphaByte for now.

Then if you want to add colorWithAlpha(0-1 float), could do that later.

[Sam Weinig]

> > 
> > > Source/WebCore/platform/graphics/ColorTypes.h:106
> > > +    constexpr LinearSRGBA(T red, T green, T blue, T alpha = ComponentTraits<T>::maxValue)
> > 
> > Super unhappy that we have to write all these constructors.
> 
> :(

I believe we could get rid of both the constructors if we were ok with removing the functionality of one them (though I haven't tested that c++ will actually allow this in practice). That is, if we were ok with not having the behavior of

LinearSRGBA { } == LinearSRGBA { 0, 0, 0, 0 }

We could remove both constructors and just set T alpha = ComponentTraits<T>::maxValue at the member definition.

Or, if we are ok with not having the behavior of:

LinearSRGBA { r, g, b } == LinearSRGBA { r, g, b, ComponentTraits<T>::maxValue }

We could remove both constructors and just set all the members to default to ComponentTraits<T>::minValue.

But I can't think of a way to maintain both behaviors without both constructors.

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> While considering different interfaces, seems we could name the function
> colorWithAlphaByte for now. And Color::alpha could be renamed alphaByte for
> now.
> 
> Then if you want to add colorWithAlpha(0-1 float), could do that later.

Yeah, that seems like a good idea. Will do.

[Sam Weinig]

(In reply to Sam Weinig from comment #6)
> (In reply to Darin Adler from comment #4)
> > While considering different interfaces, seems we could name the function
> > colorWithAlphaByte for now. And Color::alpha could be renamed alphaByte for
> > now.
> > 
> > Then if you want to add colorWithAlpha(0-1 float), could do that later.
> 
> Yeah, that seems like a good idea. Will do.

Also fun, since it kind sounds like alphabet when I say it fast in my head :).

[Darin Adler]

(In reply to Sam Weinig from comment #5)
> I believe we could get rid of both the constructors if we were ok with
> removing the functionality of one them (though I haven't tested that c++
> will actually allow this in practice).

Something that might help with this: Have a type sRGB<uint8_t> as well as sRGBA<uint8_t>. Maybe that would decrease the need for a constructor for sRGBA<uint8_t> with default values. I would still suggest initializing to 0 since uninitialized structure members are unpleasant.

There are at least some places that would use sRGB<uint8_t>; for one thing it exactly corresponds to HTML's "simple color".

[Sam Weinig]

Created attachment 404676 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #8)
> (In reply to Sam Weinig from comment #5)
> > I believe we could get rid of both the constructors if we were ok with
> > removing the functionality of one them (though I haven't tested that c++
> > will actually allow this in practice).
> 
> Something that might help with this: Have a type sRGB<uint8_t> as well as
> sRGBA<uint8_t>. Maybe that would decrease the need for a constructor for
> sRGBA<uint8_t> with default values. I would still suggest initializing to 0
> since uninitialized structure members are unpleasant.
> 
> There are at least some places that would use sRGB<uint8_t>; for one thing
> it exactly corresponds to HTML's "simple color".

Good idea. 

As an aside, I am pretty unhappy with the name SRGBA<>/SRGB<> (capital S), and would much prefer sRGBA/sRGB, but have resisted for two reasons:

1) WebCore style indicates classes/structs begin with capital letters.
2) Unclear what to do with helper functions like toSRGBA() (my initial thought is I would be tempted to change toSRGBA() to a more generic things that take the class, like convertTo<sRGBA>())

Where do you stand on this controversial idea?

[Darin Adler]

(In reply to Sam Weinig from comment #10)
> Where do you stand on this controversial idea?

I say we stick with capital S for now.

[Sam Weinig]

Created attachment 404678 [details]
Patch

[Darin Adler]

Comment on attachment 404678 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityTable.cpp:288
> +                && tableBGColor != cellColor && cellColor.alphaByte() != 1) // FIXME (https://bugs.webkit.org/show_bug.cgi?id=214537): This comparison to 1 is likely incorrect. It likely should be checking !cellColor.isOpaque().

Renaming FTW! Someone needs to add a test case now.

> Source/WebCore/rendering/RenderThemeIOS.mm:337
>      if (isChecked(box)) {
> -        ASSERT(style.visitedDependentColor(CSSPropertyBorderTopColor).alpha() % 255 == 0);
> -        ASSERT(style.visitedDependentColor(CSSPropertyBorderRightColor).alpha() % 255 == 0);
> -        ASSERT(style.visitedDependentColor(CSSPropertyBorderBottomColor).alpha() % 255 == 0);
> -        ASSERT(style.visitedDependentColor(CSSPropertyBorderLeftColor).alpha() % 255 == 0);
> +        ASSERT(style.visitedDependentColor(CSSPropertyBorderTopColor).alphaByte() % 255 == 0);
> +        ASSERT(style.visitedDependentColor(CSSPropertyBorderRightColor).alphaByte() % 255 == 0);
> +        ASSERT(style.visitedDependentColor(CSSPropertyBorderBottomColor).alphaByte() % 255 == 0);
> +        ASSERT(style.visitedDependentColor(CSSPropertyBorderLeftColor).alphaByte() % 255 == 0);
>      }

Could rewrite these to a less silly style some day.

[EWS]

Committed r264584: <https://trac.webkit.org/changeset/264584>

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

[Radar WebKit Bug Importer]

<rdar://problem/65795418>