Remove final vestigates of SimpleColor
Created attachment 404520 [details] Patch
Created attachment 404521 [details] Patch
Created attachment 404529 [details] Patch
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.
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?
Created attachment 404630 [details] Patch
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.
Committed r264565: <https://trac.webkit.org/changeset/264565> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404630 [details].
<rdar://problem/65756193>
(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.
(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.