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

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Sam Weinig]

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

[Sam Weinig]

Created attachment 403571 [details]
Checkpoint

A bit of noodling.

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 403650 [details]
Patch

[Sam Weinig]

Created attachment 403651 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

Created attachment 403655 [details]
Patch

[Sam Weinig]

Created attachment 403700 [details]
Patch

[Darin Adler]

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.

[Sam Weinig]

Created attachment 403713 [details]
Patch

[Sam Weinig]

Created attachment 403717 [details]
Patch

[Darin Adler]

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!

[Darin Adler]

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).

[EWS]

Committed r264050: <https://trac.webkit.org/changeset/264050>

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

[Radar WebKit Bug Importer]

<rdar://problem/65201889>

[Sam Weinig]

(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.