Bug 214439 – Remove final vestiges of SimpleColor

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Remove final vestigates of SimpleColor

[Sam Weinig]

Created attachment 404520 [details]
Patch

[Sam Weinig]

Created attachment 404521 [details]
Patch

[Sam Weinig]

Created attachment 404529 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 404529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404529&action=review

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:170
> +    categories[MemoryCategory::JSJIT] = HistoricMemoryCategoryInfo(MemoryCategory::JSJIT, SRGBA<uint8_t> { 255, 96, 255 }, "JS JIT");
> +    categories[MemoryCategory::Gigacage] = HistoricMemoryCategoryInfo(MemoryCategory::Gigacage, SRGBA<uint8_t> { 101, 79, 240 }, "Gigacage");
> +    categories[MemoryCategory::Images] = HistoricMemoryCategoryInfo(MemoryCategory::Images, Color::yellow, "Images");
> +    categories[MemoryCategory::Layers] = HistoricMemoryCategoryInfo(MemoryCategory::Layers, Color::cyan, "Layers");
> +    categories[MemoryCategory::LibcMalloc] = HistoricMemoryCategoryInfo(MemoryCategory::LibcMalloc, Color::green, "libc malloc");
> +    categories[MemoryCategory::bmalloc] = HistoricMemoryCategoryInfo(MemoryCategory::bmalloc, SRGBA<uint8_t> { 255, 96, 96 }, "bmalloc");
> +    categories[MemoryCategory::IsoHeap] = HistoricMemoryCategoryInfo(MemoryCategory::IsoHeap, SRGBA<uint8_t> { 128, 159, 64 }, "IsoHeap");
> +    categories[MemoryCategory::Other] = HistoricMemoryCategoryInfo(MemoryCategory::Other, SRGBA<uint8_t> { 192, 255, 0 }, "Other");

In cases like this I'm not sure it's better to use named colors in the middle of a bunch of hex values.

[Darin Adler]

Comment on attachment 404529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404529&action=review

> Source/WTF/wtf/KeyValuePair.h:45
> +        : key(std::forward<KeyTypeArg>(key))
> +        , value(std::forward<ValueTypeArg>(value))

Is forward here correct rather than WTFMove? While this is a member of a class template, it’s not a function template.

> Source/WebCore/html/ColorInputType.cpp:76
> +    return SRGBA<uint8_t> { toASCIIHexValue(string[1], string[2]), toASCIIHexValue(string[3], string[4]), toASCIIHexValue(string[5], string[6]) };

An alternative here would be to use two sets of braces:

    return { { toASCIIHexValue ... } };

> Source/WebCore/html/HTMLElement.cpp:1027
> +        return SRGBA<uint8_t> { static_cast<uint8_t>(toASCIIHexValue(string[1]) * 0x11), static_cast<uint8_t>(toASCIIHexValue(string[2]) * 0x11), static_cast<uint8_t>(toASCIIHexValue(string[3]) * 0x11) };

Ditto.

> Source/WebCore/html/HTMLElement.cpp:1055
> +        return SRGBA<uint8_t> { toASCIIHexValue(digitBuffer[0]), toASCIIHexValue(digitBuffer[1]), toASCIIHexValue(digitBuffer[2]) };

Ditto.

> Source/WebCore/html/HTMLElement.cpp:1079
> +    return SRGBA<uint8_t> { redValue, greenValue, blueValue };

Ditto.

> Source/WebCore/inspector/InspectorOverlay.cpp:1071
> +    drawText(elementTagName, SRGBA<uint8_t> { 136, 18, 128 }); // Keep this in sync with XMLViewer.css (.tag)

I think instead we should change the local drawText lambda to take SRGBA<uint8_t> and then here we would just have an initializer list and not a type name.

> Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:163
> +    categories[MemoryCategory::JSJIT] = HistoricMemoryCategoryInfo(MemoryCategory::JSJIT, SRGBA<uint8_t> { 255, 96, 255 }, "JS JIT");

Same idea as above, can we make the type be SRGBA<uint8_t> and then just use braces here.

> Source/WebCore/platform/graphics/Color.h:149
> +    static constexpr auto black = ColorBuilder<SRGBA<uint8_t>> { 0, 0, 0 };

Maybe we should define a name for ColorBuilder<SRGBA<uint8_t>> with using?

> Source/WebCore/platform/graphics/Color.h:151
> +    static constexpr auto darkGray = ColorBuilder<SRGBA<uint8_t>> { 128, 128, 128 };

Something I did a while back in my attempt at this color work was:

- Use named colors for anything that happens to be the same as a CSS named color. We may have a few more opportunities for the, or maybe you already covered it. When touching all these places with numeric color constants, I was thinking that’s a good time to look up and see if any happen to be CSS named colors.

- Rename any of these that are not consistent with CSS named color values. I seem to remember there were some of those. Maybe you already did that?

> Source/WebCore/platform/graphics/Color.h:154
> +    static constexpr auto transparent = ColorBuilder<SRGBA<uint8_t>> { 0, 0, 0, 0 };

In my patch I had renamed this transparentBlack; I think that’s better than just transparent. Especially for a case like "== transparentBlack", where "== transparent" is definitely misleading/wrong. And it’s not like this color name is used *so* often. It’s the default-constructed value of SRBA<uint8_t>, so in some cases it’s probably written that way.

In fact, here maybe omit the { 0, 0, 0, 0 }, because it’s not needed?

> Source/WebCore/platform/graphics/ColorTypes.h:63
> +    constexpr SRGBA(T red, T green, T blue, T alpha)
> +        : red { red }
> +        , green { green }
> +        , blue { blue }
> +        , alpha { alpha }
> +    {
> +    }
> +
> +    constexpr SRGBA(T red, T green, T blue)
> +        : SRGBA { red, green, blue, ComponentTraits<T>::maxValue }
> +    {
> +    }
> +
> +    constexpr SRGBA()
> +        : SRGBA { ComponentTraits<T>::minValue, ComponentTraits<T>::minValue, ComponentTraits<T>::minValue, ComponentTraits<T>::minValue }
> +    {
> +    }

Could we achieve the same result with default values rather than constructors? I’m guessing the answer is no.

> Source/WebCore/platform/graphics/ColorUtilities.h:63
> +template<template<typename> typename ColorType> constexpr ColorType<uint8_t> clampToComponentBytes(int r, int g, int b);
>  template<template<typename> typename ColorType> constexpr ColorType<uint8_t> clampToComponentBytes(int r, int g, int b, int a);

Tiny refinement thought: Since these functions take int and have no overloads, then they will silently chop something like a uint64_t or a uint32_t to an int and *then* clamp, giving the wrong result and no compilation error. Is there an idiom for writing this so it won’t do that?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerBuffer.cpp:116
> +        texmapGL.drawSolidColor(targetRect, modelViewMatrix, Color::transparent, false);

This though shows why I dislike "invalid color" in Color so much. We could just pass { } here, and it would be "invalid", not "transparent black", but seems like that would be OK. Ugh.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:202
> +    static constexpr auto spellingPatternColor = Color::red;
> +    static constexpr auto grammarPatternColor = Color::darkGreen;

This will yield color builders; guess that ends up being OK/good.

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:80
> +constexpr auto replacementTextRoundedRectPressedColor = SRGBA<uint8_t> { 105, 105, 105, 242 };
> +constexpr auto replacementTextRoundedRectColor = SRGBA<uint8_t> { 125, 125, 125, 242 };
> +constexpr auto replacementTextColor = SRGBA<uint8_t> { 240, 240, 240 };
> +constexpr auto unavailablePluginBorderColor = Color::white.colorWithAlpha(216);

Add static? Not sure when that matters.

> Source/WebCore/rendering/RenderFrameSet.cpp:73
> -constexpr auto borderStartEdgeColor = makeSimpleColor(170, 170, 170);
> +constexpr auto borderStartEdgeColor = SRGBA<uint8_t> { 170, 170, 170 };
>  constexpr auto borderEndEdgeColor = Color::black;
> -constexpr auto borderFillColor = makeSimpleColor(208, 208, 208);
> +constexpr auto borderFillColor = SRGBA<uint8_t> { 208, 208, 208 };

Ditto.

Maybe move this to the top of the file?

[Sam Weinig]

Created attachment 404630 [details]
Patch

[Darin Adler]

Comment on attachment 404630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404630&action=review

> Source/WebCore/platform/graphics/ColorBuilder.h:32
>  template<typename ColorType> struct ColorBuilder : public ColorType {

should come back here at some point and remove the redundant "public"

> Source/WebCore/platform/graphics/ColorBuilder.h:42
> -        static_assert(std::is_same_v<decltype(ColorType().alpha), uint8_t>, "Only uint8_t based color types are supported.");
> +        static_assert(std::is_same_v<typename ColorType::ComponentType, uint8_t>, "Only uint8_t based color types are supported.");

Kind of surprised about this change.

[EWS]

Committed r264565: <https://trac.webkit.org/changeset/264565>

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

[Radar WebKit Bug Importer]

<rdar://problem/65756193>

[Sam Weinig]

(In reply to Darin Adler from comment #5)
> Comment on attachment 404529 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404529&action=review
> 
> > Source/WTF/wtf/KeyValuePair.h:45
> > +        : key(std::forward<KeyTypeArg>(key))
> > +        , value(std::forward<ValueTypeArg>(value))
> 
> Is forward here correct rather than WTFMove? While this is a member of a
> class template, it’s not a function template.

Changed to WTFMove().

> 
> > Source/WebCore/html/ColorInputType.cpp:76
> > +    return SRGBA<uint8_t> { toASCIIHexValue(string[1], string[2]), toASCIIHexValue(string[3], string[4]), toASCIIHexValue(string[5], string[6]) };
> 
> An alternative here would be to use two sets of braces:

Nice. Changed to use that.

> 
>     return { { toASCIIHexValue ... } };
> 
> > Source/WebCore/html/HTMLElement.cpp:1027
> > +        return SRGBA<uint8_t> { static_cast<uint8_t>(toASCIIHexValue(string[1]) * 0x11), static_cast<uint8_t>(toASCIIHexValue(string[2]) * 0x11), static_cast<uint8_t>(toASCIIHexValue(string[3]) * 0x11) };
> 
> Ditto.

Ditto.

> 
> > Source/WebCore/html/HTMLElement.cpp:1055
> > +        return SRGBA<uint8_t> { toASCIIHexValue(digitBuffer[0]), toASCIIHexValue(digitBuffer[1]), toASCIIHexValue(digitBuffer[2]) };
> 
> Ditto.

Ditto.
> 
> > Source/WebCore/html/HTMLElement.cpp:1079
> > +    return SRGBA<uint8_t> { redValue, greenValue, blueValue };
> 
> Ditto.

Ditto.
> 
> > Source/WebCore/inspector/InspectorOverlay.cpp:1071
> > +    drawText(elementTagName, SRGBA<uint8_t> { 136, 18, 128 }); // Keep this in sync with XMLViewer.css (.tag)
> 
> I think instead we should change the local drawText lambda to take
> SRGBA<uint8_t> and then here we would just have an initializer list and not
> a type name.

Done.

> 
> > Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm:163
> > +    categories[MemoryCategory::JSJIT] = HistoricMemoryCategoryInfo(MemoryCategory::JSJIT, SRGBA<uint8_t> { 255, 96, 255 }, "JS JIT");
> 
> Same idea as above, can we make the type be SRGBA<uint8_t> and then just use
> braces here.

Done. (it was already taking a SRGBA<uint8_t>, so the name was redundant already).

> 
> > Source/WebCore/platform/graphics/Color.h:149
> > +    static constexpr auto black = ColorBuilder<SRGBA<uint8_t>> { 0, 0, 0 };
> 
> Maybe we should define a name for ColorBuilder<SRGBA<uint8_t>> with using?
> 
> > Source/WebCore/platform/graphics/Color.h:151
> > +    static constexpr auto darkGray = ColorBuilder<SRGBA<uint8_t>> { 128, 128, 128 };
> 
> Something I did a while back in my attempt at this color work was:
> 
> - Use named colors for anything that happens to be the same as a CSS named
> color. We may have a few more opportunities for the, or maybe you already
> covered it. When touching all these places with numeric color constants, I
> was thinking that’s a good time to look up and see if any happen to be CSS
> named colors.
> 
> - Rename any of these that are not consistent with CSS named color values. I
> seem to remember there were some of those. Maybe you already did that?

I like that idea.

> 
> > Source/WebCore/platform/graphics/Color.h:154
> > +    static constexpr auto transparent = ColorBuilder<SRGBA<uint8_t>> { 0, 0, 0, 0 };
> 
> In my patch I had renamed this transparentBlack; I think that’s better than
> just transparent. Especially for a case like "== transparentBlack", where
> "== transparent" is definitely misleading/wrong. And it’s not like this
> color name is used *so* often. It’s the default-constructed value of
> SRBA<uint8_t>, so in some cases it’s probably written that way.
> 
> In fact, here maybe omit the { 0, 0, 0, 0 }, because it’s not needed?

Omitted the 0s. Will rename in another patch.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:63
> > +    constexpr SRGBA(T red, T green, T blue, T alpha)
> > +        : red { red }
> > +        , green { green }
> > +        , blue { blue }
> > +        , alpha { alpha }
> > +    {
> > +    }
> > +
> > +    constexpr SRGBA(T red, T green, T blue)
> > +        : SRGBA { red, green, blue, ComponentTraits<T>::maxValue }
> > +    {
> > +    }
> > +
> > +    constexpr SRGBA()
> > +        : SRGBA { ComponentTraits<T>::minValue, ComponentTraits<T>::minValue, ComponentTraits<T>::minValue, ComponentTraits<T>::minValue }
> > +    {
> > +    }
> 
> Could we achieve the same result with default values rather than
> constructors? I’m guessing the answer is no.

I was able to get rid of of the middle one. But the others are needed due to weird default constructor being deleted for structs behavior. 

> 
> > Source/WebCore/platform/graphics/ColorUtilities.h:63
> > +template<template<typename> typename ColorType> constexpr ColorType<uint8_t> clampToComponentBytes(int r, int g, int b);
> >  template<template<typename> typename ColorType> constexpr ColorType<uint8_t> clampToComponentBytes(int r, int g, int b, int a);
> 
> Tiny refinement thought: Since these functions take int and have no
> overloads, then they will silently chop something like a uint64_t or a
> uint32_t to an int and *then* clamp, giving the wrong result and no
> compilation error. Is there an idiom for writing this so it won’t do that?

Something like this should work (and support a future where some color types have more than four components).

template<typename ComponentType> constexpr uint8_t clampToComponentByte(ComponentType component)
{
    return std::clamp(component, static_cast<ComponentType>(0), static_cast<ComponentType>(255));
}

template<template<typename> typename ColorType, typename... ComponentType> constexpr ColorType<uint8_t> clampToComponentBytes(ComponentType... components)
{
    return { clampToComponentByte(components)... };
}


> 
> > Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:202
> > +    static constexpr auto spellingPatternColor = Color::red;
> > +    static constexpr auto grammarPatternColor = Color::darkGreen;
> 
> This will yield color builders; guess that ends up being OK/good.

Yeah, that's fine. ColorBuilders are structurally identical to the thing they build. 

> 
> > Source/WebCore/rendering/RenderEmbeddedObject.cpp:80
> > +constexpr auto replacementTextRoundedRectPressedColor = SRGBA<uint8_t> { 105, 105, 105, 242 };
> > +constexpr auto replacementTextRoundedRectColor = SRGBA<uint8_t> { 125, 125, 125, 242 };
> > +constexpr auto replacementTextColor = SRGBA<uint8_t> { 240, 240, 240 };
> > +constexpr auto unavailablePluginBorderColor = Color::white.colorWithAlpha(216);
> 
> Add static? Not sure when that matters.

Done. I would also like to figure out a better rule of thumb of when we should be using static for constexpr things.

> 
> > Source/WebCore/rendering/RenderFrameSet.cpp:73
> > -constexpr auto borderStartEdgeColor = makeSimpleColor(170, 170, 170);
> > +constexpr auto borderStartEdgeColor = SRGBA<uint8_t> { 170, 170, 170 };
> >  constexpr auto borderEndEdgeColor = Color::black;
> > -constexpr auto borderFillColor = makeSimpleColor(208, 208, 208);
> > +constexpr auto borderFillColor = SRGBA<uint8_t> { 208, 208, 208 };
> 
> Ditto.
> 
> Maybe move this to the top of the file?

Done.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 404630 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404630&action=review
> 
> > Source/WebCore/platform/graphics/ColorBuilder.h:32
> >  template<typename ColorType> struct ColorBuilder : public ColorType {
> 
> should come back here at some point and remove the redundant "public"

Good point.

> 
> > Source/WebCore/platform/graphics/ColorBuilder.h:42
> > -        static_assert(std::is_same_v<decltype(ColorType().alpha), uint8_t>, "Only uint8_t based color types are supported.");
> > +        static_assert(std::is_same_v<typename ColorType::ComponentType, uint8_t>, "Only uint8_t based color types are supported.");
> 
> Kind of surprised about this change.

Yeah, I wanted to use Color::ComponentType consistently in one pass and this was an accidental holder over I guess. Will revert when fixing the unnecessary public.