WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
213820
Split Color serialization out of Color classes
https://bugs.webkit.org/show_bug.cgi?id=213820
Summary
Split Color serialization out of Color classes
Sam Weinig
Reported
2020-06-30 16:27:57 PDT
Split Color serialization out of Color classes
Attachments
Patch
(62.22 KB, patch)
2020-06-30 16:42 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(62.19 KB, patch)
2020-06-30 17:02 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Patch
(62.78 KB, patch)
2020-06-30 18:58 PDT
,
Sam Weinig
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2020-06-30 16:42:07 PDT
Created
attachment 403251
[details]
Patch
Sam Weinig
Comment 2
2020-06-30 17:02:33 PDT
Created
attachment 403252
[details]
Patch
Darin Adler
Comment 3
2020-06-30 17:12:08 PDT
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?
Darin Adler
Comment 4
2020-06-30 17:12:29 PDT
Comment on
attachment 403252
[details]
Patch r=me, see my comments on the older version
Sam Weinig
Comment 5
2020-06-30 18:47:30 PDT
(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.
Sam Weinig
Comment 6
2020-06-30 18:58:28 PDT
Created
attachment 403258
[details]
Patch
EWS
Comment 7
2020-06-30 22:53:25 PDT
Committed
r263788
: <
https://trac.webkit.org/changeset/263788
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 403258
[details]
.
Radar WebKit Bug Importer
Comment 8
2020-06-30 22:54:15 PDT
<
rdar://problem/64972755
>
Aakash Jain
Comment 9
2020-07-01 06:44:11 PDT
(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
Sam Weinig
Comment 10
2020-07-01 07:26:55 PDT
(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.
Darin Adler
Comment 11
2020-07-01 09:52:32 PDT
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);
Sam Weinig
Comment 12
2020-07-01 10:39:04 PDT
(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)
Darin Adler
Comment 13
2020-07-01 10:45:10 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug