Bug 213820

Created attachment 403251 [details] Patch

Created attachment 403252 [details] Patch

Comment on attachment 403251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403251&action=review > Source/WebCore/ChangeLog:15 > + Now all color serialization calls one of the following three functions, depending on need: > + > + - serializationForCSS(...) > + - serializationForHTML(...) > + - serializationForRenderTreeAsText(...) Note that I moved parsing, the other side of this, into CSS entirely. Or maybe I had planned to do it and never landed the patch? You chose "serialization [of color]" rather than "[color] serialized". I think I like your choice better than that alternative, but I am not sure. > Source/WebCore/ChangeLog:17 > + These are overload for all three Color classes (Color, SimpleColor and ExtendedColor) to overloaded > Source/WebCore/html/canvas/CanvasStyle.h:113 > auto& color = WTF::holds_alternative<Color>(m_style) ? WTF::get<Color>(m_style) : WTF::get<CMYKAColor>(m_style).colorConvertedToSRGBA; > - return color.serialized(); > + return serializationForHTML(color); Maybe this should be a one-liner now. > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:360 > + // FIXME: Seems like this should be using serializationForCSS instead? Triumph of the good function names! > Source/WebCore/page/DragController.cpp:1448 > - image->setInlineStyleProperty(CSSPropertyBackgroundColor, Color(Color::black).colorWithAlpha(0.05).cssText()); > + image->setInlineStyleProperty(CSSPropertyBackgroundColor, serializationForCSS(Color::black.colorWithAlpha(13))); Kinda like how the old code was clearly 5%, seems like we lost a tiny bit here, although runtime-wise way better to have this constant-folded. Maybe we should take the constant folding even further and write "rgba(0, 0, 0, 0.05)"_s. > Source/WebCore/platform/graphics/ColorSerialization.cpp:45 > +static std::array<char, 4> fractionDigitsForFractionalAlphaValue(uint8_t alpha) Pretty proud of this function. > Source/WebCore/platform/graphics/ColorSerialization.cpp:119 > +String serializationForHTML(const ExtendedColor& color) > +{ > + return serializationForCSS(color); > +} > + > +String serializationForRenderTreeAsText(const ExtendedColor& color) > +{ > + return serializationForCSS(color); > +} Consider moving these function bodies to the header to facilitate additional inlining? > Source/WebCore/platform/graphics/ColorSerialization.h:41 > +WEBCORE_EXPORT String serializationForCSS(const SimpleColor&); > +WEBCORE_EXPORT String serializationForHTML(const SimpleColor&); > +WEBCORE_EXPORT String serializationForRenderTreeAsText(const SimpleColor&); Why not take SimpleColor by value instead of by reference?

Comment on attachment 403252 [details] Patch r=me, see my comments on the older version

(In reply to Darin Adler from comment #3) > Comment on attachment 403251 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403251&action=review > > > Source/WebCore/ChangeLog:15 > > + Now all color serialization calls one of the following three functions, depending on need: > > + > > + - serializationForCSS(...) > > + - serializationForHTML(...) > > + - serializationForRenderTreeAsText(...) > > Note that I moved parsing, the other side of this, into CSS entirely. Or > maybe I had planned to do it and never landed the patch? You moved them. I kept these in platform/ for now because of the three variants. They likely don't make sense there long term, but I couldn't decide on which directory they should go in (or if they should be split up). > > You chose "serialization [of color]" rather than "[color] serialized". I > think I like your choice better than that alternative, but I am not sure. The naming is entirely stolen from SimpleColor. > > > Source/WebCore/ChangeLog:17 > > + These are overload for all three Color classes (Color, SimpleColor and ExtendedColor) to > > overloaded > > > Source/WebCore/html/canvas/CanvasStyle.h:113 > > auto& color = WTF::holds_alternative<Color>(m_style) ? WTF::get<Color>(m_style) : WTF::get<CMYKAColor>(m_style).colorConvertedToSRGBA; > > - return color.serialized(); > > + return serializationForHTML(color); > > Maybe this should be a one-liner now. Good call. > > > Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:360 > > + // FIXME: Seems like this should be using serializationForCSS instead? > > Triumph of the good function names! > > > Source/WebCore/page/DragController.cpp:1448 > > - image->setInlineStyleProperty(CSSPropertyBackgroundColor, Color(Color::black).colorWithAlpha(0.05).cssText()); > > + image->setInlineStyleProperty(CSSPropertyBackgroundColor, serializationForCSS(Color::black.colorWithAlpha(13))); > > Kinda like how the old code was clearly 5%, seems like we lost a tiny bit > here, although runtime-wise way better to have this constant-folded. Maybe > we should take the constant folding even further and write "rgba(0, 0, 0, > 0.05)"_s. I agree on both points. I think I am going to have to write a constexpr std::lround() myself, as https://wg21.link/p0533r6 was not accepted into c++20. But I think that may also have to wait for c++20 anyway, as lround() no doubt requires bitcasting the float to a uint32_t, which is something that you can't do in a constexpr until c++20. I maybe could write a really dumb version, that only guaranteed to work for values between 0 - 255: constexpr long dumb_lroundf(float x) { long truncatedToLong = static_cast<long>(x); float castedBackToFloat = static_cast<float>(truncatedToLong); if (castedBackToFloat == x) return truncatedToLong; float remainder = x - castedBackToFloat; if (remainder >= 0.5) return truncatedToLong + 1; return truncatedToLong; } That seems to work with my test cases. Of course, I don't know if we can conditionally use a constexpr function like this until we have support for std::is_constant_evaluated(), which is also c++ 20. Will keep thinking Also do kinda want to write the constexpr parser for "rgba(0, 0, 0, 0.05"_color when I figure out the rounding. > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:45 > > +static std::array<char, 4> fractionDigitsForFractionalAlphaValue(uint8_t alpha) > > Pretty proud of this function. * chefs kiss * > > > Source/WebCore/platform/graphics/ColorSerialization.cpp:119 > > +String serializationForHTML(const ExtendedColor& color) > > +{ > > + return serializationForCSS(color); > > +} > > + > > +String serializationForRenderTreeAsText(const ExtendedColor& color) > > +{ > > + return serializationForCSS(color); > > +} > > Consider moving these function bodies to the header to facilitate additional > inlining? > > > Source/WebCore/platform/graphics/ColorSerialization.h:41 > > +WEBCORE_EXPORT String serializationForCSS(const SimpleColor&); > > +WEBCORE_EXPORT String serializationForHTML(const SimpleColor&); > > +WEBCORE_EXPORT String serializationForRenderTreeAsText(const SimpleColor&); > > Why not take SimpleColor by value instead of by reference? Oh good call. Thanks for the review.

Created attachment 403258 [details] Patch

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

<rdar://problem/64972755>

(In reply to EWS from comment #7) > Committed r263788: <https://trac.webkit.org/changeset/263788> This seems to have broken windows build. Tracked in https://bugs.webkit.org/show_bug.cgi?id=213841

(In reply to Aakash Jain from comment #9) > (In reply to EWS from comment #7) > > Committed r263788: <https://trac.webkit.org/changeset/263788> > This seems to have broken windows build. Tracked in > https://bugs.webkit.org/show_bug.cgi?id=213841 Fixing now.

Comment on attachment 403251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403251&action=review >> Source/WebCore/page/DragController.cpp:1448 >> + image->setInlineStyleProperty(CSSPropertyBackgroundColor, serializationForCSS(Color::black.colorWithAlpha(13))); > > Kinda like how the old code was clearly 5%, seems like we lost a tiny bit here, although runtime-wise way better to have this constant-folded. Maybe we should take the constant folding even further and write "rgba(0, 0, 0, 0.05)"_s. I think you misunderstood my suggestions here. I suggest we come back here and change it to this: image->setInlineStyleProperty(CSSPropertyBackgroundColor, "rgba(0, 0, 0, 0.05)"_s);

(In reply to Darin Adler from comment #11) > Comment on attachment 403251 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403251&action=review > > >> Source/WebCore/page/DragController.cpp:1448 > >> + image->setInlineStyleProperty(CSSPropertyBackgroundColor, serializationForCSS(Color::black.colorWithAlpha(13))); > > > > Kinda like how the old code was clearly 5%, seems like we lost a tiny bit here, although runtime-wise way better to have this constant-folded. Maybe we should take the constant folding even further and write "rgba(0, 0, 0, 0.05)"_s. > > I think you misunderstood my suggestions here. I suggest we come back here > and change it to this: > > image->setInlineStyleProperty(CSSPropertyBackgroundColor, "rgba(0, 0, 0, > 0.05)"_s); Oh, heh. Yeah. That is of course much more reasonable than the current code :). Makes me actually want to go further, and see if there is a way we can just create the parsed CSSProperty, why parse it at all! (I still do want to write a constexpr color parser at some point, just because)

(In reply to Sam Weinig from comment #12) > Makes me actually want to go further, and see if there is a way we can > just create the parsed CSSProperty, why parse it at all! I feel the same. This is a recurring pattern in WebKit, creating content that we then parse. And maybe sometimes it’s the best way, but probably not often! > (I still do want to write a constexpr color parser at some point, just > because) Yes, quite relevant to the above. Let people feel free to use markup as long as it’s parsed at compile time.