Part 2 of SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them.
Created attachment 403571 [details] Checkpoint A bit of noodling.
Goals for this stage: - Remove uses of SimpleColor class that are not implementation details of Color. - Mostly replacing them with direct use of SRGBA<uint8_t> either explicitly or by using auto. - Remove implicit conversions from ARGB8888 uint32_ts to SimpleColor - Replaced by explicit use of asSRGBA(Packed::ARGB { value }) to get a SRGBA<uint8_t> where the value is not a constant - Replaced by either SRGBA<uint8_t>(uint8_t, uint8_t, uint8_t, uint8_t) or the very incorrectly named makeSimpleColor() (as it now returns a SRGBA<uint8_t>). - Current change has makeSimpleColor(SRGBA<float>) being replaced by convertToComponentBytesLossy(SRGBA<float>). I think the latter needs a better name and to look more cast-like. I want to keep the "Lossy", as I think many of these sites are unnecessarily getting rid of the precision. Perhaps convertToLossy<SRGBA<uint8_t>>(SRGBA<float>)? Maybe convertTo<SRGBA<uint8_t>>(SRGBA<float>) is nicer? - As noted above, the current change still has makeSimpleColor(int, int, int, int), but it now returns SRGBA<uint8_t>. Many callers, specifically the ones passing constants, could probably switch to just using SRGBA<uint8_t> { int, int, int, int }, which has the benefit of being very clear what color space and component type is being created. Non-constant users should likely switch to clampToComponentBytes<SRGBA<uint8_t>>(int, int, int, int). - To keep support for the Color::yellow.colorWithAlpha(...) and switching on Color::white.value() idioms, I added a new type, tentatively called ConstexprColor which wraps a SRGBA<uint8_t> and exposes the needed functionality. This needs more iteration. I think the ideal solution would be to make Color itself constexpr-able, and have Color::yellow return a Color. Not sure how feasible that is. Regardless of all this, probably want to break this up a bit. I think maybe a good first step here will be just removing the SimpleColor constructor that takes an unsigned, and updating all callers to pass each component or convert to SRGBA<uint8_t> first.
Created attachment 403650 [details] Patch
Created attachment 403651 [details] Patch
I decided to focus on getting rid of the ARGB heritage of SimpleColor for this change. That means: - Remove SimpleColor constructor taking a uint32_t. - Remove SimpleColor functions value() and valueAsARGB() (both did the same thing). - Replace internal representation with SRGBA<uint8_t>. In practice, this meant finding all the call sites that used long hex constants to hard code a color, and switching them to calling makeSimpleColor with the individual components instead.
Created attachment 403655 [details] Patch
Created attachment 403700 [details] Patch
Comment on attachment 403700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403700&action=review Seems like a step in the right direction. > Source/WebCore/ChangeLog:12 > + - Replaces internal representation of SimpleColor based on an ARGB uint32_t with SRGBA<uint8_t>. > + - Removes SimpleColor constructor taking a uint32_t. Callers that still need to convert from ARGB > + now do makeSimpleColor(asSRGBA(Packed::ARGB { value })). > + - Removes value() and valueAsARGB() member functions from SimpleColor. Callers that need a packed > + representation now do Packed::ARGB { simpleColor.asSRGBA<uint8_t>() }.value. Not sure these all 3 had to happen in a single patch. This is not huge, but a Iittle bigger than the idea size to review. > Source/WebCore/editing/CompositionHighlight.h:45 > + static constexpr auto defaultCompositionFillColor = makeSimpleColor(0xE1, 0xDD, 0x55, 0xFF); No need for the explicit 0xFF alpha. > Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:131 > + HistoricMemoryCategoryInfo(unsigned category, SimpleColor simpleColor, String name, bool subcategory = false) I totally would have named the argument just "color"; the harmless name conflict would not have caused trouble. But I’m not sure I want to convince you to do it. > Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:174 > + categories[MemoryCategory::JSJIT] = HistoricMemoryCategoryInfo(MemoryCategory::JSJIT, makeSimpleColor(0xFF, 0x60, 0xFF, 0xFF), "JS JIT"); > + categories[MemoryCategory::Gigacage] = HistoricMemoryCategoryInfo(MemoryCategory::Gigacage, makeSimpleColor(0x65, 0x4F, 0xF0, 0xFF), "Gigacage"); > + categories[MemoryCategory::Images] = HistoricMemoryCategoryInfo(MemoryCategory::Images, makeSimpleColor(0xFF, 0xFF, 0x00, 0xFF), "Images"); > + categories[MemoryCategory::Layers] = HistoricMemoryCategoryInfo(MemoryCategory::Layers, makeSimpleColor(0x00, 0xFF, 0xFF, 0xFF), "Layers"); > + categories[MemoryCategory::LibcMalloc] = HistoricMemoryCategoryInfo(MemoryCategory::LibcMalloc, makeSimpleColor(0x00, 0xFF, 0x00, 0xFF), "libc malloc"); > + categories[MemoryCategory::bmalloc] = HistoricMemoryCategoryInfo(MemoryCategory::bmalloc, makeSimpleColor(0xFF, 0x60, 0x60, 0xFF), "bmalloc"); > + categories[MemoryCategory::IsoHeap] = HistoricMemoryCategoryInfo(MemoryCategory::IsoHeap, makeSimpleColor(0x80, 0x9F, 0x40, 0xFF), "IsoHeap"); > + categories[MemoryCategory::Other] = HistoricMemoryCategoryInfo(MemoryCategory::Other, makeSimpleColor(0xC0, 0xFF, 0x00, 0xFF), "Other"); > > // Sub categories (e.g breakdown of bmalloc tag.) > - categories[MemoryCategory::GCHeap] = HistoricMemoryCategoryInfo(MemoryCategory::GCHeap, 0xFFA0A0FF, "GC heap", true); > - categories[MemoryCategory::GCOwned] = HistoricMemoryCategoryInfo(MemoryCategory::GCOwned, 0xFFFFC060, "GC owned", true); > + categories[MemoryCategory::GCHeap] = HistoricMemoryCategoryInfo(MemoryCategory::GCHeap, makeSimpleColor(0xA0, 0xA0, 0xFF, 0xFF), "GC heap", true); > + categories[MemoryCategory::GCOwned] = HistoricMemoryCategoryInfo(MemoryCategory::GCOwned, makeSimpleColor(0xFF, 0xC0, 0x60, 0xFF), "GC owned", true); No need for the explicit 0xFF alpha. > Source/WebCore/platform/graphics/Color.cpp:40 > +static constexpr auto lightenedBlack = makeSimpleColor(0x54, 0x54, 0x54, 0xFF); > +static constexpr auto darkenedWhite = makeSimpleColor(0xAB, 0xAB, 0xAB, 0xFF); No need for the explicit 0xFF alpha. > Source/WebCore/platform/graphics/SimpleColor.h:110 > +constexpr SimpleColor makeSimpleColor(int r, int g, int b) This clamps silently. Consider changing the argument types to uint8_t. Perhaps will need overloading so we don’t silently chop instead of clamping? > Source/WebCore/platform/graphics/SimpleColor.h:115 > +constexpr SimpleColor makeSimpleColor(int r, int g, int b, int a) Ditto. > Source/WebCore/platform/mock/MockRealtimeVideoSource.cpp:271 > + static constexpr auto magenta = makeSimpleColor(0xFF, 0x00, 0xFF, 0xFF); > + static constexpr auto blue = makeSimpleColor(0x00, 0x00, 0xFF, 0xFF); > + static constexpr auto red = makeSimpleColor(0xFF, 0x00, 0x00, 0xFF); > + static constexpr auto darkGreen = makeSimpleColor(0x00, 0x80, 0x00, 0xFF); No need for the explicit 0xFF alpha. > Source/WebCore/rendering/RenderObject.cpp:1809 > + constexpr float baseDarkColorLuminance { 0.014443844f }; // Luminance of SRGBA<uint8_t> { 0x20, 0x20, 0x20, 0xFF } > + constexpr float baseLightColorLuminance { 0.83077f }; // Luminance of SRGBA<uint8_t> { 0xEB, 0xEB, 0xEB, 0xFF } No need for the explicit 0xFF alpha. > Source/WebCore/rendering/RenderThemeMac.mm:2426 > +constexpr auto attachmentTitleInactiveBackgroundColor = makeSimpleColor(204, 204, 204, 255); > +constexpr auto attachmentTitleInactiveTextColor = makeSimpleColor(100, 100, 100, 255); No need for the explicit 255 alpha. > Source/WebCore/rendering/RenderThemeMac.mm:2430 > +constexpr auto attachmentSubtitleTextColor = makeSimpleColor(82, 145, 214, 255); No need for the explicit 255 alpha. > Source/WebKitLegacy/win/FullscreenVideoController.cpp:81 > +static constexpr auto textColor = makeSimpleColor(0xFF, 0xFF, 0xFF); No need for the explicit 0XFF alpha.
Created attachment 403713 [details] Patch
Created attachment 403717 [details] Patch
Comment on attachment 403700 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403700&action=review >> Source/WebCore/rendering/RenderObject.cpp:1809 >> + constexpr float baseLightColorLuminance { 0.83077f }; // Luminance of SRGBA<uint8_t> { 0xEB, 0xEB, 0xEB, 0xFF } > > No need for the explicit 0xFF alpha. Heh, this particular explicit alpha is in a comment, and in place where it’s not optional, I think!
Comment on attachment 403717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403717&action=review > Source/WebCore/platform/graphics/ColorUtilities.h:57 > +constexpr uint8_t clampToComponentByte(int c) Word instead of letter? > Source/WebCore/platform/graphics/ColorUtilities.h:59 > + return static_cast<uint8_t>(std::clamp(c, 0, 0xFF)); I think this can be written: return clampTo<uint8_t>(c); We have this clampTo template in our MathExtras.h file (or some other WTF file).
Committed r264050: <https://trac.webkit.org/changeset/264050> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403717 [details].
<rdar://problem/65201889>
(In reply to Darin Adler from comment #12) > Comment on attachment 403717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403717&action=review > > > Source/WebCore/platform/graphics/ColorUtilities.h:57 > > +constexpr uint8_t clampToComponentByte(int c) > > Word instead of letter? > > > Source/WebCore/platform/graphics/ColorUtilities.h:59 > > + return static_cast<uint8_t>(std::clamp(c, 0, 0xFF)); > > I think this can be written: > > return clampTo<uint8_t>(c); > > We have this clampTo template in our MathExtras.h file (or some other WTF > file). Oops, sorry, didn't see these before landing. Will address these in the next part.