Now that we have SRGBA<uint8_t>, SimpleColor feels a bit redundant. Let's see what it will take to replace SimpleColor with SRGBA<uint8_t>.
Created attachment 403499 [details] Part 1
Comment on attachment 403499 [details] Part 1 Part 1: Removes red, green, and blue component accessors from SimpleColor, replacing it with easier access to SRGBA<uint8_t> from Color itself. This is done by replacing toSRGBASimpleColorLossy() and toSRGBALossy() on Color, with one templatized toSRGBALossy<>() that can do both. As a result of this, many call sites that used to deal with SimpleColors now deal with SRGBA<uint8_t>, so utilities like premulitiply/unpremultiply now should be in terms of SRGBA<uint8_t> rather than SimpleColor.
Created attachment 403500 [details] Patch
Created attachment 403501 [details] Patch
Created attachment 403502 [details] Part 1
Created attachment 403503 [details] Patch
Created attachment 403504 [details] Patch
Created attachment 403514 [details] Patch
Created attachment 403521 [details] Patch
Created attachment 403523 [details] Patch
Created attachment 403529 [details] Part 1
Ok, I think Part 1 is ready for review.
Comment on attachment 403529 [details] Part 1 View in context: https://bugs.webkit.org/attachment.cgi?id=403529&action=review Love the direction this is going. > Source/WebCore/ChangeLog:9 > + Begin converging SimpleColor and SRGBA<uint8_t>, starting with removing usages that > + were getting SimpleColors to access or operate on the color's components. Converging seems great. The strangest thing about SimpleColor is probably how it can seems to prefer to represent all 4 channels in a single integer. > Source/WebCore/ChangeLog:12 > + - Replace toSRGBASimpleColorLossy() with toSRGBALossy<uint8_t> > + - Replace toSRGBALossy() with toSRGBALossy<float>(). It worries me that the integer vs. floating point type also implies [0-255] vs. [0-1]. Kind of wish there was a "uint8_t to mean [0-1]" type instead of actual uint8_t, but that would also ruin the code I guess. But it would be good to prevent accidental conversion between that and floating point. Also ignores the fact that floating point [0-255] is a thing. > Source/WebCore/platform/graphics/Color.cpp:152 > + return { makeSimpleColor(toSRGBALossy<uint8_t>()), Semantic }; This isn’t so pretty. The makeSimpleColor call here seems strange. > Source/WebCore/platform/graphics/ColorTypes.h:243 > +struct ARGB { Not sure if this is a specific enough name to imply "stored as a 32-bit integer with 8-bit components". Seems like either 32 or 8 might need to be in the name. SRGBA also defines the color space more precisely by including the letter "S".
(In reply to Darin Adler from comment #13) > Comment on attachment 403529 [details] > Part 1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403529&action=review > > Love the direction this is going. > > > Source/WebCore/ChangeLog:9 > > + Begin converging SimpleColor and SRGBA<uint8_t>, starting with removing usages that > > + were getting SimpleColors to access or operate on the color's components. > > Converging seems great. The strangest thing about SimpleColor is probably > how it can seems to prefer to represent all 4 channels in a single integer. > > > Source/WebCore/ChangeLog:12 > > + - Replace toSRGBASimpleColorLossy() with toSRGBALossy<uint8_t> > > + - Replace toSRGBALossy() with toSRGBALossy<float>(). > > It worries me that the integer vs. floating point type also implies [0-255] > vs. [0-1]. Kind of wish there was a "uint8_t to mean [0-1]" type instead of > actual uint8_t, but that would also ruin the code I guess. But it would be > good to prevent accidental conversion between that and floating point. Also > ignores the fact that floating point [0-255] is a thing. When you wrote "uint8_t to mean [0-1]" did you mean "uint8_t to mean [0-255]"? Or maybe I am misunderstanding. Either way, one idea for how to better abstract this was to use something like the c++ std::byte type - https://en.cppreference.com/w/cpp/types/byte (not it specifically, but something akin to it). And require something like std::to_integer to convert it to a usable integer. Going to play around with that more after this round of chances. > > > Source/WebCore/platform/graphics/Color.cpp:152 > > + return { makeSimpleColor(toSRGBALossy<uint8_t>()), Semantic }; > > This isn’t so pretty. The makeSimpleColor call here seems strange. Yeah, follow up will make this quite a bit nicer by allowing Color to be constructed from typed colors like SRGBA<uint8_t> directly. > > > Source/WebCore/platform/graphics/ColorTypes.h:243 > > +struct ARGB { > > Not sure if this is a specific enough name to imply "stored as a 32-bit > integer with 8-bit components". Seems like either 32 or 8 might need to be > in the name. SRGBA also defines the color space more precisely by including > the letter "S". Yeah, I am not completely happy with this naming yet. Going to keep iterating on it. Thanks for the review.
Committed r263941: <https://trac.webkit.org/changeset/263941> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403529 [details].
<rdar://problem/65098023>
(In reply to Sam Weinig from comment #14) > When you wrote "uint8_t to mean [0-1]" did you mean "uint8_t to mean > [0-255]"? I meant what you thought; just didn’t say the words right. A uint8_t that uses integer values in the range 0-255 to represent channel values in the abstract range 0-1.