Bug 161856 – Replace RGBA32 with Color in member variables

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Replace RGBA32 with Color in member variables

[Radar WebKit Bug Importer]

<rdar://problem/28254324>

[Dean Jackson]

As part of https://bugs.webkit.org/show_bug.cgi?id=160520

Replace the use of RGBA32 values with actual Color instances. Now that colors can be outside of sRGB a simple 32 bit value isn't sufficient.

[Radar WebKit Bug Importer]

<rdar://problem/28254338>

[Dean Jackson]

Created attachment 288550 [details]
WIP Patch - NFR

[Dean Jackson]

Created attachment 288656 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 288656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288656&action=review

> Source/WebCore/html/canvas/CanvasStyle.cpp:87
> +CanvasStyle::CanvasStyle(Color color)
> +    : m_color(color)

Should we pass by reference now that Color contains a RefPtr? Copying will result in ref churn.

> Source/WebCore/page/PageOverlay.cpp:139
> +void PageOverlay::setBackgroundColor(const Color backgroundColor)

Why const here?

> Source/WebCore/page/PageOverlay.h:110
> +    void setBackgroundColor(const Color);

Why const?

> Source/WebCore/rendering/RenderTheme.h:415
> +    static NeverDestroyed<const Color> defaultTapHighlightColor = 0x66000000;

Don't think the const gives you anything.

[Dean Jackson]

Comment on attachment 288656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288656&action=review

>> Source/WebCore/html/canvas/CanvasStyle.cpp:87
>> +    : m_color(color)
> 
> Should we pass by reference now that Color contains a RefPtr? Copying will result in ref churn.

I'm going to do that separately because there are many hundreds of places to fix :(

>> Source/WebCore/page/PageOverlay.cpp:139
>> +void PageOverlay::setBackgroundColor(const Color backgroundColor)
> 
> Why const here?

No reason.

>> Source/WebCore/page/PageOverlay.h:110
>> +    void setBackgroundColor(const Color);
> 
> Why const?

No reason.

>> Source/WebCore/rendering/RenderTheme.h:415
>> +    static NeverDestroyed<const Color> defaultTapHighlightColor = 0x66000000;
> 
> Don't think the const gives you anything.

That's just the pattern that NeverDestroyed uses. I can remove it.

[Dean Jackson]

Created attachment 288729 [details]
Patch

[Dean Jackson]

Created attachment 288734 [details]
Patch

[Dean Jackson]

Created attachment 288741 [details]
Patch

[Dean Jackson]

Created attachment 288751 [details]
Patch

[Dean Jackson]

Committed r205892: <http://trac.webkit.org/changeset/205892>

[Darin Adler]

Comment on attachment 288741 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288741&action=review

Looks like you landed this already. I have a lot of comments that may be different from the initial ones you got.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1261
> +    setShadow(FloatSize(width, height), blur, Color(grayLevel, grayLevel, grayLevel, alpha));

In a new call site like this where there is no type conversion, I suggest the following syntax:

    setShadow({ width, height }, blur, { grayLevel, grayLevel, grayLevel, alpha });

This will give an error if types aren’t exactly right rather than converting types. If you want to be explicit about the aggregate types you could include them:

    setShadow(FloatSize { width, height }, blur, Color { grayLevel, grayLevel, grayLevel, alpha });

I think both of these are better than the syntax that looks like a function call.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:319
> +    void setShadow(const FloatSize& offset, float blur, Color);

What are the performance implications of using Color here rather than const Color&? Why make a different choice for this argument than the one we made for FloatSize, which uses const FloatSize&?

> Source/WebCore/html/canvas/CanvasStyle.cpp:93
> +    : m_color(Color(grayLevel, grayLevel, grayLevel, alpha))

Should be:

    : m_color { grayLevel, grayLevel, grayLevel, alpha }

No need for a temporary Color when creating a Color and best to use the { } syntax rather than () in new code.

> Source/WebCore/html/canvas/CanvasStyle.cpp:99
> +    : m_color(Color(r, g, b, a))

Should be:

    : m_color { r, g, b, a }

Same reason as above.

> Source/WebCore/html/canvas/CanvasStyle.cpp:105
> +    : m_cmyka(new CMYKAValues(Color(c, m, y, k, a), c, m, y, k, a))

Should be:

    : m_cmyka { new CMYKAValues { { c, m, y, k, a }, c, m, y, k, a } }

> Source/WebCore/html/canvas/CanvasStyle.cpp:203
> +    return m_color == Color(r, g, b, a);

Should be:

    return m_color == Color { r, g, b, a };

But also, is this a sufficiently efficient algorithm for this?

> Source/WebCore/html/canvas/CanvasStyle.h:45
> +        explicit CanvasStyle(Color);

This should take a Color&& and use WTFMove to avoid reference count churn.

> Source/WebCore/html/canvas/CanvasStyle.h:85
> +            CMYKAValues()
> +                : color(), c(0), m(0), y(0), k(0), a(0)
> +            { }

No need to mention color() directly; will work by just not mentioning it. Also, the other values could be specified below where the data members are defined rather than here in the constructor. I suggest writing this:

    CMYKAValues() = default;

And doing it all below with initialization syntax:

    float c { 0 };

> Source/WebCore/html/canvas/CanvasStyle.h:87
> +            CMYKAValues(Color color, float cyan, float magenta, float yellow, float black, float alpha)

First argument should be Color&& and use WTFMove to avoid reference count churn.

> Source/WebCore/html/canvas/CanvasStyle.h:107
>          union {
> -            RGBA32 m_rgba;
> +            Color m_color;

Changing this union so that it now has a member that has a destructor requires a more changes to the code using these fields to call the destructor and constructor when setting the value of the union. The compiler won’t call the constructor and destructor automatically and we have to carefully do it ourselves. To quote <http://en.cppreference.com/w/cpp/language/union>:

"If members of a union are classes with user-defined constructors and destructors, to switch the active member, explicit destructor and placement new are generally needed"

We might get away with this if Color doesn’t yet have a destructor, but as soon as we add one we will need to deal with it.

Cleanest design would be to replace the union and m_type with a std::experimental::variant. That will probably have some other significant advantages. We won’t need to allocate everything on the heap and do all the manual reference counting.

> Source/WebCore/html/track/TextTrackCueGeneric.h:64
>      Color foregroundColor() const { return m_foregroundColor; }

Return type should be const Color&.

> Source/WebCore/html/track/TextTrackCueGeneric.h:65
> +    void setForegroundColor(Color color) { m_foregroundColor = color; }

Type should be Color&& and use WTFMove.

> Source/WebCore/html/track/TextTrackCueGeneric.h:67
>      Color backgroundColor() const { return m_backgroundColor; }

Ditto.

> Source/WebCore/html/track/TextTrackCueGeneric.h:68
> +    void setBackgroundColor(Color color) { m_backgroundColor = color; }

Ditto.

> Source/WebCore/html/track/TextTrackCueGeneric.h:70
>      Color highlightColor() const { return m_highlightColor; }

Ditto.

> Source/WebCore/html/track/TextTrackCueGeneric.h:71
> +    void setHighlightColor(Color color) { m_highlightColor = color; }

Ditto.

> Source/WebCore/page/PageOverlay.h:109
> +    Color backgroundColor() const { return m_backgroundColor; }

Return type should be const Color&.

> Source/WebCore/page/PageOverlay.h:110
> +    void setBackgroundColor(Color);

Type should be Color&& and use WTFMove.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:89
>      Color foregroundColor() const { return m_foregroundColor; }

Return type should be const Color&.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:90
> +    void setForegroundColor(Color color) { m_foregroundColor = color; }

Type should be Color&& and use WTFMove.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:92
>      Color backgroundColor() const { return m_backgroundColor; }

Ditto.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:93
> +    void setBackgroundColor(Color color) { m_backgroundColor = color; }

Ditto.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:95
>      Color highlightColor() const { return m_highlightColor; }

Ditto.

> Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h:96
> +    void setHighlightColor(Color color) { m_highlightColor = color; }

Ditto.

> Source/WebCore/platform/graphics/mac/ColorMac.h:50
> +    Color oldAquaFocusRingColor();

Consider having the return type be const Color&.

> Source/WebCore/platform/graphics/mac/ColorMac.mm:37
> +Color oldAquaFocusRingColor()

Consider having the return type be const Color& and using a NeverDestroyed global.

> Source/WebCore/rendering/RenderTheme.cpp:1340
> +Color RenderTheme::platformTapHighlightColor() const

Since return type is Color rather than const Color&, we will run the constructor and destructor every time. Would be better to have a more efficient.

> Source/WebCore/rendering/RenderTheme.cpp:1344
> +    static NeverDestroyed<const Color> defaultTapHighlightColor = Color(0, 0, 0, 102);

Probably can use a modern syntax with { } instead of function call style and maybe even omit the second mention of the name Color.

> Source/WebCore/rendering/style/BorderValue.h:36
>      BorderValue()

Should convert file to use #pragma once when touching it.

> Source/WebCore/rendering/style/BorderValue.h:67
>      void setColor(const Color& color)

Should change type to Color&& and use WTFMove.

> Source/WebCore/rendering/style/BorderValue.h:72
> +    Color color() const { return m_color; }

Should change type to const Color&.

> Source/WebCore/rendering/style/CollapsedBorderValue.h:42
>      CollapsedBorderValue(const BorderValue& border, const Color& color, EBorderPrecedence precedence)

Should change argument type to Color&& and use WTFMove.

> Source/WebCore/rendering/style/CollapsedBorderValue.h:54
> +    Color color() const { return m_color; }

Should change type to const Color&.

> Source/WebKit/mac/Misc/WebKitNSStringExtras.mm:106
> +        graphicsContext.setFillColor(Color(static_cast<float>(red * 255), static_cast<float>(green * 255.0f), static_cast<float>(blue * 255.0f), static_cast<float>(alpha * 255.0f)));

I suggest you try this:

    graphicsContext.setFillColor({ static_cast<float>(red * 255), static_cast<float>(green * 255), static_cast<float>(blue * 255), static_cast<float>(alpha * 255) });

No reason to use a float 255 to multiple by a float or a double; the integer 255 will do the same thing.

Too bad we have to do all that casting, but I can’t think of a cleaner way to convert "either double or float" to float. I thought at one time we had some kind of "narrow" function, but I don’t remember what it was called.

[Darin Adler]

Looks like some of the comments were things you planned to fix later anyway.

[Darin Adler]

I just realized this patch is a month old. Oops!