Bug 162878 – Add preliminary support for extended colors to WebCore::Color

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Add preliminary support for extended colors to WebCore::Color

[Radar WebKit Bug Importer]

<rdar://problem/28596413>

[Dean Jackson]

Created attachment 290521 [details]
Patch

[WebKit Commit Bot]

Attachment 290521 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 290527 [details]
Patch

[WebKit Commit Bot]

Attachment 290527 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

Created attachment 290530 [details]
Patch

[WebKit Commit Bot]

Attachment 290530 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:34:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 290527 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:620
> +    if (!isExtended())
> +        return nullptr;
> +    return m_extendedColor;

A common pattern in our code is to make these asFoo() functions ASSERT that the condition is true (e.g. ASSERT(isExtended()).  There can be good reasons to not do that (for instance, combining null checks for perf reasons). Was this intentional?

> Source/WebCore/platform/graphics/ExtendedColor.cpp:36
> +    ExtendedColor* color = new ExtendedColor(r, g, b, a, colorSpace);
> +    return color;

This could be on one line.

> Source/WebCore/platform/graphics/ExtendedColor.h:32
> +class ExtendedColor {

Can this be RefCounted?

> Source/WebCore/platform/graphics/ExtendedColor.h:34
> +    static ExtendedColor* create(float r, float g, float b, float a, const ColorSpace& colorSpace = ColorSpace::ColorSpaceSRGB);

This should return a Ref<ExtendedColor>.

[Dean Jackson]

Created attachment 290535 [details]
Patch

[Dean Jackson]

Comment on attachment 290527 [details]
Patch

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

>> Source/WebCore/platform/graphics/Color.cpp:620
>> +    return m_extendedColor;
> 
> A common pattern in our code is to make these asFoo() functions ASSERT that the condition is true (e.g. ASSERT(isExtended()).  There can be good reasons to not do that (for instance, combining null checks for perf reasons). Was this intentional?

Nope. Added the ASSERT.

>> Source/WebCore/platform/graphics/ExtendedColor.cpp:36
>> +    return color;
> 
> This could be on one line.

Done.

>> Source/WebCore/platform/graphics/ExtendedColor.h:32
>> +class ExtendedColor {
> 
> Can this be RefCounted?

Done.

>> Source/WebCore/platform/graphics/ExtendedColor.h:34
>> +    static ExtendedColor* create(float r, float g, float b, float a, const ColorSpace& colorSpace = ColorSpace::ColorSpaceSRGB);
> 
> This should return a Ref<ExtendedColor>.

Done.

[WebKit Commit Bot]

Attachment 290535 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:37:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:37:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 290535 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:275
> +    m_rgbaAndFlags = other.m_rgbaAndFlags;
> +    if (isExtended())
> +        m_extendedColor->ref();

It's a little confusing that assigning to m_rgbaAndFlags also updates m_extendedColor (because unions). Can you name the union, and just copy via its name?

> Source/WebCore/platform/graphics/Color.cpp:604
> +void Color::setExtended()

I think this would read better as setIsExtended

> Source/WebCore/platform/graphics/Color.h:179
> +    int red() const { return redChannel(rgb()); }
> +    int green() const { return greenChannel(rgb()); }
> +    int blue() const { return blueChannel(rgb()); }
> +    int alpha() const { return alphaChannel(rgb()); }

Should these assert if it's an extended color? Now they'll return garbage.

> Source/WebCore/platform/graphics/Color.h:185
> +        m_rgbaAndFlags = (static_cast<uint64_t>(rgb) << 32);

Is it OK that this clobbers any extended color?

> Source/WebCore/platform/graphics/Color.h:252
> +    static const uint64_t extendedColor = 0x0;
> +    static const uint64_t invalidRGBAColor = 0x1;
> +    static const uint64_t validRGBAColorBit = 0x2;
> +    static const uint64_t validRGBAColor = 0x3;

This makes things a bit confusing. How about:

enum {
  rgbaColorBit = 0;
  validColorBit = 1;
};


enum {
  rgbaColorMask = 1 << rgbaColorBit,
  validColorMask = 1 << validColorBit

  extendedColorMask = rgbaColorMask | validColorMask
}

bool isExtended() const
{
  return !(m_rgbaAndFlags & extendedColorMask);
}

[Simon Fraser (smfr)]

This should be:
  validRgbaColorMask = rgbaColorMask | validColorMask
or something

[Darin Adler]

Comment on attachment 290521 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:145
> +    WEBCORE_EXPORT Color(const Color&);

I suggest implementing the move constructor right from the start even for this first version. It’s a valuable optimization when dealing with a reference counted object to avoid churn.

> Source/WebCore/platform/graphics/Color.h:187
> +    void setRGB(RGBA32 rgb)
> +    {
> +        m_rgbaAndFlags = (static_cast<uint64_t>(rgb) << 32);
> +        setValid();
> +    }

If this is going to be multi-line then I suggest putting the actual inline body after the class definition instead of having it here. Also, no need for those parentheses, i think.

> Source/WebCore/platform/graphics/Color.h:243
> +    WEBCORE_EXPORT Color& operator=(const Color&);

I suggest implementing the move version of the assignment operator right from the start even for this first version. It’s a valuable optimization when dealing with a reference counted object to avoid churn.

> Source/WebCore/platform/graphics/Color.h:259
> +        uint64_t m_rgbaAndFlags { invalidRGBAColor };
> +        ExtendedColor* m_extendedColor;

Will this do the right thing under 32-bit in both kinds of endian-ness and compiler differences? If I set m_extendedColor, and it’s a 32-bit pointer, what guarantees that the flag bits in m_rgbaAndFlags will be set to 0?

> Source/WebCore/platform/graphics/ExtendedColor.h:32
> +class ExtendedColor {

This class should use the RefCounted template, unless there is a good reason why not to.

> Source/WebCore/platform/graphics/ExtendedColor.h:34
> +    static ExtendedColor* create(float r, float g, float b, float a, const ColorSpace& colorSpace = ColorSpace::ColorSpaceSRGB);

This should return a Ref<ExtendedColor> rather than a raw pointer.

> Source/WebCore/platform/graphics/ExtendedColor.h:43
> +    const ColorSpace& colorSpace() const { return m_colorSpace; }

Since ColorSpace is a scalar, return type can just be ColorSpace instead of a const&.

> Source/WebCore/platform/graphics/ExtendedColor.h:49
> +    ExtendedColor(int r, int g, int b, int a, const ColorSpace& colorSpace)

Seems like these should be float, not int!

[Dean Jackson]

Comment on attachment 290521 [details]
Patch

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

>> Source/WebCore/platform/graphics/Color.h:145
>> +    WEBCORE_EXPORT Color(const Color&);
> 
> I suggest implementing the move constructor right from the start even for this first version. It’s a valuable optimization when dealing with a reference counted object to avoid churn.

Done.

>> Source/WebCore/platform/graphics/Color.h:187
>> +    }
> 
> If this is going to be multi-line then I suggest putting the actual inline body after the class definition instead of having it here. Also, no need for those parentheses, i think.

Done.

>> Source/WebCore/platform/graphics/Color.h:259
>> +        ExtendedColor* m_extendedColor;
> 
> Will this do the right thing under 32-bit in both kinds of endian-ness and compiler differences? If I set m_extendedColor, and it’s a 32-bit pointer, what guarantees that the flag bits in m_rgbaAndFlags will be set to 0?

I assumed it would, but I'm not sure. I'll ask around.

My assumption was that any assignment to (what is now) m_colorData.extendedColor would set the entire 64 bits, even when on a 32-bit device. However, I can probably guarantee this when I assign to extendedColor by first setting rgbaAndFlags to 0. It's the next patch that starts using the extendedColor part.

>> Source/WebCore/platform/graphics/ExtendedColor.h:32
>> +class ExtendedColor {
> 
> This class should use the RefCounted template, unless there is a good reason why not to.

Yes, sorry. This patch is out of date. It uses RefCounted now.

>> Source/WebCore/platform/graphics/ExtendedColor.h:34
>> +    static ExtendedColor* create(float r, float g, float b, float a, const ColorSpace& colorSpace = ColorSpace::ColorSpaceSRGB);
> 
> This should return a Ref<ExtendedColor> rather than a raw pointer.

Same here.

>> Source/WebCore/platform/graphics/ExtendedColor.h:43
>> +    const ColorSpace& colorSpace() const { return m_colorSpace; }
> 
> Since ColorSpace is a scalar, return type can just be ColorSpace instead of a const&.

Done.

>> Source/WebCore/platform/graphics/ExtendedColor.h:49
>> +    ExtendedColor(int r, int g, int b, int a, const ColorSpace& colorSpace)
> 
> Seems like these should be float, not int!

Yep. This got me for about 10 minutes in a later patch, when I was trying to work out why all my extended colors had 0 or 1 values in each channel :(

[Dean Jackson]

Created attachment 291330 [details]
Patch

[WebKit Commit Bot]

Attachment 291330 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ExtendedColor.h:37:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Jackson]

(In reply to comment #12)

> This makes things a bit confusing. How about:
> 
> enum {
>   rgbaColorBit = 0;
>   validColorBit = 1;
> };
> 
> 
> enum {
>   rgbaColorMask = 1 << rgbaColorBit,
>   validColorMask = 1 << validColorBit
> 
>   extendedColorMask = rgbaColorMask | validColorMask
> }

I tried a version of this and it was equally confusing to read. The problem is that you need versions of 0, 1, 2 and 3.

0 - if the lowest bit is 0, then it is extended color
1 - if the lowest bit is 1, then it is an RGBA32
2 - this is the bit mask used for validity of RGBA32s
3 - if the lowest two bits are 1, then it is a valid RGBA32

I couldn't come up with a much better way to explain this by name.

[Dean Jackson]

Committed r207265: <http://trac.webkit.org/changeset/207265>