CSS parsing should use Color not RGBA32

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

CSS parsing should use Color not RGBA32

[Radar WebKit Bug Importer]

<rdar://problem/28766903>

[Dean Jackson]

Created attachment 291547 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291547 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasStyle.cpp:-51
> -    if (equalLettersIgnoringASCIICase(colorString, "currentcolor"))
> -        return ParsedCurrentColor;

Does CSSParser::parseColor() do this part now?

> Source/WebCore/platform/graphics/Color.h:159
> +    explicit Color(WTF::HashTableDeletedValueType)
> +    {
> +        m_colorData.rgbaAndFlags = 0xfffffffffffffffd;
> +        ASSERT(!isExtended());
> +    }
> +
> +    bool isHashTableDeletedValue() const
> +    {
> +        return m_colorData.rgbaAndFlags == 0xfffffffffffffffd;
> +    }
> +
> +    explicit Color(WTF::HashTableEmptyValueType)
> +    {
> +        m_colorData.rgbaAndFlags = 0xffffffffffffffb;
> +        ASSERT(!isExtended());
> +    }

Can you use your bit enums to mask off bits, rather than hardcoding them here?

[Dean Jackson]

Comment on attachment 291547 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasStyle.cpp:-51
>> -        return ParsedCurrentColor;
> 
> Does CSSParser::parseColor() do this part now?

It's down in parseColorOrCurrentColor now.

>> Source/WebCore/platform/graphics/Color.h:159
>> +    }
> 
> Can you use your bit enums to mask off bits, rather than hardcoding them here?

I could, but I'd still need to do some explicit bit-fudging. These are colors that can never really exist. Supposedly valid RGBA32s, so they don't get deleted as ExtendedColor, but with junk in the spare space.

[Dean Jackson]

Committed r207317: <http://trac.webkit.org/changeset/207317>

[Ryan Haddad]

As EWS predicted, this caused LayoutTest editing/execCommand/query-command-value-background-color.html to start failing on all platforms.

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r207337%20(650)/results.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2FexecCommand%2Fquery-command-value-background-color.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 163448

[Dean Jackson]

Committed r207361: <http://trac.webkit.org/changeset/207361>

[Darin Adler]

Comment on attachment 291547 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasGradient.cpp:60
> +    Color color = parseColorOrCurrentColor(colorString, 0 /*canvas*/);

Should we use nullptr instead of 0 for canvas?

> Source/WebCore/platform/graphics/Color.h:146
> +        m_colorData.rgbaAndFlags = 0xfffffffffffffffd;

I normally use uppercase hex; not sure if it’s a WebKit coding style agree-to thing, but it’s seen just below in createUnchecked.

I suggest a private named constant rather than repeating this three times. Eliminates the possibility that one of the three has a different number of "f" letters.

>>> Source/WebCore/platform/graphics/Color.h:159
>>> +    }
>> 
>> Can you use your bit enums to mask off bits, rather than hardcoding them here?
> 
> I could, but I'd still need to do some explicit bit-fudging. These are colors that can never really exist. Supposedly valid RGBA32s, so they don't get deleted as ExtendedColor, but with junk in the spare space.

Could use some static_assert to document some of these properties of the magic value.

> Source/WebCore/platform/graphics/Color.h:204
> +    uint64_t asUint64() const { return m_colorData.rgbaAndFlags; }

I would have preferred that you expose a hash function here rather than exposing this so the hash function can use it.

The capitalization of Uint64 seems peculiar.

> Source/WebCore/platform/graphics/ColorHash.h:34
> +    static unsigned hash(const WebCore::Color& key) { return intHash(key.asUint64()); }

When we add support for ExtendedColor, this hash function will no longer be correct. Also seems to me that if we are using asUint64 for the hash, we should also be using it directly in the equal function.

> Source/WebCore/platform/graphics/ColorHash.h:40
> +    typedef ColorHash Hash;

In new code we should use "using" instead of typedef.

    using Hash = ColorHash;

[Dean Jackson]

Comment on attachment 291547 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasGradient.cpp:60
>> +    Color color = parseColorOrCurrentColor(colorString, 0 /*canvas*/);
> 
> Should we use nullptr instead of 0 for canvas?

Done

>> Source/WebCore/platform/graphics/Color.h:146
>> +        m_colorData.rgbaAndFlags = 0xfffffffffffffffd;
> 
> I normally use uppercase hex; not sure if it’s a WebKit coding style agree-to thing, but it’s seen just below in createUnchecked.
> 
> I suggest a private named constant rather than repeating this three times. Eliminates the possibility that one of the three has a different number of "f" letters.

Fixed!

>>>> Source/WebCore/platform/graphics/Color.h:159
>>>> +    }
>>> 
>>> Can you use your bit enums to mask off bits, rather than hardcoding them here?
>> 
>> I could, but I'd still need to do some explicit bit-fudging. These are colors that can never really exist. Supposedly valid RGBA32s, so they don't get deleted as ExtendedColor, but with junk in the spare space.
> 
> Could use some static_assert to document some of these properties of the magic value.

Added some static_asserts. Disappointed I didn't think of this originally.

>> Source/WebCore/platform/graphics/Color.h:204
>> +    uint64_t asUint64() const { return m_colorData.rgbaAndFlags; }
> 
> I would have preferred that you expose a hash function here rather than exposing this so the hash function can use it.
> 
> The capitalization of Uint64 seems peculiar.

Done. Removed the incorrectly capitalized Uint64() and added the unsigned hash() const.

>> Source/WebCore/platform/graphics/ColorHash.h:34
>> +    static unsigned hash(const WebCore::Color& key) { return intHash(key.asUint64()); }
> 
> When we add support for ExtendedColor, this hash function will no longer be correct. Also seems to me that if we are using asUint64 for the hash, we should also be using it directly in the equal function.

Yep. I made a note of this in Color.h. (Same applies to operator==)

>> Source/WebCore/platform/graphics/ColorHash.h:40
>> +    typedef ColorHash Hash;
> 
> In new code we should use "using" instead of typedef.
> 
>     using Hash = ColorHash;

Fixed.