Summary: | Remove ColorBuilder | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, annulen, apinheiro, cdumez, cfleizach, changseok, darin, dmazzoni, eric.carlson, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, hi, jcraig, jdiggs, jer.noble, joepeck, kondapallykalyan, mifenton, mmaxfield, pdr, philipj, ryuan.choi, samuel_white, sergio, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sam Weinig
2020-07-18 11:36:14 PDT
Created attachment 404644 [details]
Patch
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. (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. :( 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. > >
> > > 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.
(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. (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 :). (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". Created attachment 404676 [details]
Patch
(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? (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. Created attachment 404678 [details]
Patch
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. Committed r264584: <https://trac.webkit.org/changeset/264584> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404678 [details]. |