Bug 214158

Created attachment 403938 [details] Patch

Biggest SimpleColor-related thing left to do after this is figure out what to do with the makeSimpleColor() family of functions. They currently are used for the following: 1) Construction of SRGBA<uint8_t> from constants: e.g. static constexpr auto cyan = makeSimpleColor(0, 255, 255); (in Color.h) 2) Construction of SRGBA<uint8_t> from known uint8_t values: e.g. makeSimpleColor(colorArray[0], colorArray[1], colorArray[2], alphaComponent) (parseRGBParameters in CSSPropertyParserHelpers.cpp) 3) Construction of SRGBA<uint8_t> from ints that could be out of range and therefore need to be clamped: e.g. makeSimpleColor(r, g, b); (AXIsolatedObject::initializeAttributeData in AXIsolatedObject.cpp) 4) Construction of SRGBA<uint8_t> from a SRGBA<float>. in theory, these should only need to be multiplied by 255 (per component) but currently also get clamped. e.g. makeSimpleColor(SRGBA { r, g, b, a }) (CanvasRenderingContext2DBase::setShadow in CanvasRenderingContext2DBase.cpp)

Created attachment 403942 [details] Patch

Comment on attachment 403942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403942&action=review > Source/WebCore/platform/graphics/Color.h:294 > + > + > + if constexpr (std::is_same_v<T, float>) Excess blank line here. I would consider omitting the blank lines entirely. > Source/WebCore/platform/graphics/Color.h:296 > + else if constexpr (std::is_same_v<T, uint8_t>) No need for else here. Also, it seems logical to me to check uint8_t first, before float. > Source/WebCore/platform/graphics/ColorUtilities.h:97 > +template<typename ColorType, typename Functor> ColorType colorByModifingEachNonAlphaComponent(const ColorType& color, Functor&& functor) I still prefer a style where we have exposition, declarations of the functions and what they are for, followed by implementation, function definitions. This file seems to mix the two and I think we could do better. Given this calls the function and does not take ownership, would const Functor& be better than Functor&&? > Source/WebCore/platform/graphics/ColorUtilities.h:99 > + // FIXME: This should be made to work with color's that don't use the names red, green, and blue for their channels. "color's" -> "colors". > Source/WebCore/platform/graphics/ColorUtilities.h:104 > + ColorType copy = color; > + copy.red = functor(color.red); > + copy.green = functor(color.green); > + copy.blue = functor(color.blue); > + return copy; Does this generate efficient code? Seems like colors are first copied and then overwritten, but maybe the compiler can optimize that away. I Guess there are other examples below doing the same. Also, for this and other cases below I suggest: auto copy = color; Instead of writing out ColorType. > Source/WebCore/platform/graphics/ColorUtilities.h:133 > + ColorType copy = color; auto > Source/WebCore/platform/graphics/ColorUtilities.h:140 > + ColorType copy = color; auto > Source/WebCore/platform/graphics/ColorUtilities.h:153 > +constexpr uint8_t invertComponent(uint8_t component) > +{ > + return 255 - component; > +} > + > +constexpr float invertComponent(float component) > +{ > + return 1.0f - component; > +} This kind of overloading, where numeric values depend on type, still doesn’t seem quite right to me. What happens with the 255.0f floating point code we have in some places? > Source/WebCore/platform/graphics/ColorUtilities.h:157 > + ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto component) { auto > Source/WebCore/platform/graphics/ColorUtilities.h:166 > + ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto component) { auto > Source/WebCore/platform/graphics/cg/ColorCG.cpp:54 > +static Optional<SRGBA<uint8_t>> makeSimpleColorFromCGColor(CGColorRef color) Seems like this function needs a name that better communicates its limitations, because it: - Reinterprets color components as either gray scale or sRGBA, based only on component *count*, ignoring color space. That's a bad bug. I guess that's what "FIXME: ExtendedColor - needs to handle color spaces" means, but it seems like an understatement. - Clamps to the sRGBA gamut, loses anything outside the 0.0-1.0, ruining colors for HDR. That makes sense, I suppose since the return type is SRGBA<uint8_t>, but no word "clamp" in the title. - Rounds color channels to 0-255 integers, losing any precision beyond that. Makes sense since the return type is SRGBA<uint8_t>. I suppose "SimpleColor" is saying that? Also, no need for "FromCGColor" in this function name. This is C++, not C. > Source/WebCore/platform/graphics/cg/ColorCG.cpp:58 > // FIXME: ExtendedColor - needs to handle color spaces. > + if (!color) > + return WTF::nullopt; Should leave a blank line after the comment since it’s about the whole function, not the first paragraph. > Source/WebCore/platform/graphics/cg/ColorCG.cpp:87 > + : Color(makeSimpleColorFromCGColor(color)) Not obvious to me that this will always be the right way to handle conversion from all CGColorRef objects. Some might need representation as extended colors. So probably needs to be a named constructor? > Source/WebCore/platform/graphics/cg/ColorCG.cpp:92 > + : Color(makeSimpleColorFromCGColor(color), tag) Ditto.

(In reply to Darin Adler from comment #7) > Comment on attachment 403942 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403942&action=review > > > Source/WebCore/platform/graphics/Color.h:294 > > + > > + > > + if constexpr (std::is_same_v<T, float>) > > Excess blank line here. I would consider omitting the blank lines entirely. Done. > > > Source/WebCore/platform/graphics/Color.h:296 > > + else if constexpr (std::is_same_v<T, uint8_t>) > > No need for else here. Also, it seems logical to me to check uint8_t first, > before float. Done. > > > Source/WebCore/platform/graphics/ColorUtilities.h:97 > > +template<typename ColorType, typename Functor> ColorType colorByModifingEachNonAlphaComponent(const ColorType& color, Functor&& functor) > > I still prefer a style where we have exposition, declarations of the > functions and what they are for, followed by implementation, function > definitions. This file seems to mix the two and I think we could do better. Yeah, fixed. > > Given this calls the function and does not take ownership, would const > Functor& be better than Functor&&? This is another of those universal references (since Functor is a deduced type), not really an r-value. > > > Source/WebCore/platform/graphics/ColorUtilities.h:99 > > + // FIXME: This should be made to work with color's that don't use the names red, green, and blue for their channels. > > "color's" -> "colors". Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:104 > > + ColorType copy = color; > > + copy.red = functor(color.red); > > + copy.green = functor(color.green); > > + copy.blue = functor(color.blue); > > + return copy; > > Does this generate efficient code? Seems like colors are first copied and > then overwritten, but maybe the compiler can optimize that away. I Guess > there are other examples below doing the same. From my testing, the compiler generates the same code as if you passed in a reference. > > Also, for this and other cases below I suggest: > > auto copy = color; > > Instead of writing out ColorType. Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:133 > > + ColorType copy = color; > > auto Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:140 > > + ColorType copy = color; > > auto Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:153 > > +constexpr uint8_t invertComponent(uint8_t component) > > +{ > > + return 255 - component; > > +} > > + > > +constexpr float invertComponent(float component) > > +{ > > + return 1.0f - component; > > +} > > This kind of overloading, where numeric values depend on type, still doesn’t > seem quite right to me. What happens with the 255.0f floating point code we > have in some places? I hear you, but we really only have the 255.0f floating point code in two places: 1) for a brief moment when parsing css colors and we parsed a percentage, but we immediately convert that to a uint8_t via convertPrescaledToComponentByte. 2) In the intermediate state of the animation functions of SVGAnimationColorFunction. I think we can and should do better here, but I think the problem is relatively contained in the interim. > > > Source/WebCore/platform/graphics/ColorUtilities.h:157 > > + ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto component) { > > auto Fixed. > > > Source/WebCore/platform/graphics/ColorUtilities.h:166 > > + ColorType copy = colorByModifingEachNonAlphaComponent(color, [] (auto component) { > > auto Fixed. > > > Source/WebCore/platform/graphics/cg/ColorCG.cpp:54 > > +static Optional<SRGBA<uint8_t>> makeSimpleColorFromCGColor(CGColorRef color) > > Seems like this function needs a name that better communicates its > limitations, because it: > > - Reinterprets color components as either gray scale or sRGBA, based only on > component *count*, ignoring color space. That's a bad bug. I guess that's > what "FIXME: ExtendedColor - needs to handle color spaces" means, but it > seems like an understatement. > > - Clamps to the sRGBA gamut, loses anything outside the 0.0-1.0, ruining > colors for HDR. That makes sense, I suppose since the return type is > SRGBA<uint8_t>, but no word "clamp" in the title. > > - Rounds color channels to 0-255 integers, losing any precision beyond that. > Makes sense since the return type is SRGBA<uint8_t>. > > I suppose "SimpleColor" is saying that? Also, no need for "FromCGColor" in > this function name. This is C++, not C. Renamed to roundAndClampToSRGBALossy() for now. Will want to keep iterating on this name (and the other makeSimpleColor() names). > > > Source/WebCore/platform/graphics/cg/ColorCG.cpp:58 > > // FIXME: ExtendedColor - needs to handle color spaces. > > + if (!color) > > + return WTF::nullopt; > > Should leave a blank line after the comment since it’s about the whole > function, not the first paragraph. Haha. I went back and forth on this blank lines a few times. You are definitely right, but for some reason the blank space kept bugging me (I'll get over it). > > > Source/WebCore/platform/graphics/cg/ColorCG.cpp:87 > > + : Color(makeSimpleColorFromCGColor(color)) > > Not obvious to me that this will always be the right way to handle > conversion from all CGColorRef objects. Some might need representation as > extended colors. So probably needs to be a named constructor? I think this current model is in fact the wrong way to go long term, and we instead maintain the color space and gamut of the CGColorRef (and by extension NSColor/UIColor) by default. It's hard for me to imagine a situation where we would explicitly want to lose the precision. Thanks.

Comment on attachment 403942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403942&action=review >>> Source/WebCore/platform/graphics/ColorUtilities.h:97 >>> +template<typename ColorType, typename Functor> ColorType colorByModifingEachNonAlphaComponent(const ColorType& color, Functor&& functor) >> >> I still prefer a style where we have exposition, declarations of the functions and what they are for, followed by implementation, function definitions. This file seems to mix the two and I think we could do better. >> >> Given this calls the function and does not take ownership, would const Functor& be better than Functor&&? > > Yeah, fixed. Let me re-ask a question: Given this calls the function and does not take ownership, would const Functor& be better than Functor&&? How do we benefit from using a universal reference rather than a const&?

(In reply to Darin Adler from comment #9) > Comment on attachment 403942 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403942&action=review > > >>> Source/WebCore/platform/graphics/ColorUtilities.h:97 > >>> +template<typename ColorType, typename Functor> ColorType colorByModifingEachNonAlphaComponent(const ColorType& color, Functor&& functor) > >> > >> I still prefer a style where we have exposition, declarations of the functions and what they are for, followed by implementation, function definitions. This file seems to mix the two and I think we could do better. > >> > >> Given this calls the function and does not take ownership, would const Functor& be better than Functor&&? > > > > Yeah, fixed. > > Let me re-ask a question: > > Given this calls the function and does not take ownership, would const > Functor& be better than Functor&&? How do we benefit from using a universal > reference rather than a const&? This is a good question, and I am afraid I am guilty of cargo-culting here without truly understanding the deep fundamentals. In looking at examples that seem similar-ish from the standard library, like std::apply() or std::invoke(), that take their function objects as Functor&&, I latched on. But that is not a good reason to do it. Thinking a bit more deeply about it, the example I can come up with where using Function&& would work, but const Function& would not, is that of a mutable functor like the following: struct SampleFunctor { int value = 0; void operator()() { printf("Value is %d\n", value++); } }; template<typename Functor> decltype(auto) callFunctor(const Functor& f) { f(); f(); } int main (int argc, char const *argv[]) { callFunctor(SampleFunctor()); return 0; } This fails to compile with the following errors: universal.cpp:13:5: error: no matching function for call to object of type 'const SampleFunctor' f(); ^ universal.cpp:19:5: note: in instantiation of function template specialization 'callFunctor<SampleFunctor>' requested here callFunctor(SampleFunctor()); ^ universal.cpp:5:10: note: candidate function not viable: 'this' argument has type 'const SampleFunctor', but method is not marked const void operator()() Changing the type to Functor&& works as you might expect. This might be too contrived an example to worry about the case, I'm not entirely sure. There might be intrinsic value for the project to map type&& to the concept taking ownership and avoiding universal references unless really needed (for things like perfect forwarding).

Created attachment 403973 [details] Patch

(In reply to Sam Weinig from comment #10) > In looking at examples > that seem similar-ish from the standard library, like std::apply() or > std::invoke(), that take their function objects as Functor&&, I latched on. That’s pretty good circumstantial evidence that this is the correct pattern. Would be great to have a deeper understanding of why; not sure I am *quite* there yet.

Committed r264230: <https://trac.webkit.org/changeset/264230> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403973 [details].

<rdar://problem/65349835>