Split Color serialization out of Color classes

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Split Color serialization out of Color classes

[Sam Weinig]

Created attachment 403251 [details]
Patch

[Sam Weinig]

Created attachment 403252 [details]
Patch

[Darin Adler]

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 on attachment 403252 [details]
Patch

r=me, see my comments on the older version

[Sam Weinig]

(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]

Created attachment 403258 [details]
Patch

[EWS]

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]

<rdar://problem/64972755>

[Aakash Jain]

(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]

(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 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]

(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]

(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.