[webkit-reviews] review granted: [Bug 213981] Part 2 of SimpleColor and SRGBA<uint8_t> are essentially the same - let's converge them : [Attachment 403700] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 213981: Part 2 of SimpleColor and SRGBA<uint8_t> are essentially the same -
let's converge them
https://bugs.webkit.org/show_bug.cgi?id=213981

Attachment 403700: Patch

https://bugs.webkit.org/attachment.cgi?id=403700&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 403700
  --> https://bugs.webkit.org/attachment.cgi?id=403700
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.