Bug 163557 – Allow creation of ExtendedColors and make Color immutable

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Allow creation of ExtendedColors and make Color immutable

[Radar WebKit Bug Importer]

<rdar://problem/28805360>

[Dean Jackson]

Created attachment 291857 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/Color.h:162:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/Color.h:162:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:44:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:60:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:76:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:95:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:114:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:130:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:156:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 9 in 13 files


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

[Dave Hyatt]

Comment on attachment 291857 [details]
Patch

r=me

[Darin Adler]

Comment on attachment 291857 [details]
Patch

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

Exciting!

I believe that both the hash function and the == function for Color are incorrect when extended colors are involved. I believe no two extended colors will compare as equal to each other and I also believe that extended colors can never be equal to non-extended colors, but that seems wrong when they two describe exactly the same color. There are multiple solutions to these problems.

> Source/WebCore/css/parser/CSSParser.cpp:7553
> +    return Color(name);

I prefer this syntax:

    return Color { name };

I won’t comment about that at every call site below, but I recommend using that style.

> Source/WebCore/dom/Document.cpp:823
> +    m_activeLinkColor = Color("red");

Could we do this with an RGB value instead of parsing the string "red"?

> Source/WebCore/html/HTMLElement.cpp:1040
> +    Color color = Color(colorString);

I would write this:

    Color color { colorString };

> Source/WebCore/platform/graphics/Color.cpp:230
> +        return 0;

nullptr

> Source/WebCore/platform/graphics/Color.cpp:233
> +        if (!c || c > 0x7F)

Could use !isASCII(c) instead of "c > 0x7F"

> Source/WebCore/platform/graphics/Color.cpp:234
> +            return 0;

nullptr

> Source/WebCore/platform/graphics/Color.cpp:256
> +        const NamedColor* foundColor = findNamedColor(name);
> +        if (foundColor)

I would write:

    if (auto* foundColor = findNamedColor(name))

> Source/WebCore/platform/graphics/Color.cpp:333
> +        return asExtended()->cssText();

Should change asExtended() to return a reference instead of a pointer. Unless it’s legal to call it on a non-extended color, and if that is so we should just call asExtended(), not both functions, here.

> Source/WebCore/platform/graphics/Color.h:161
> +    // This creates an ExtendedColor.

But should it always do that? What if the colorspace is sRGB and the values exactly match what we can make with integers?

> Source/WebCore/platform/graphics/Color.h:265
>  private:
>  
> +    void setRGB(int r, int g, int b) { setRGB(makeRGB(r, g, b)); }

Lets get rid of that blank line.

[Dean Jackson]

(In reply to comment #5)
> Comment on attachment 291857 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291857&action=review
> 
> Exciting!
> 
> I believe that both the hash function and the == function for Color are
> incorrect when extended colors are involved. I believe no two extended
> colors will compare as equal to each other and I also believe that extended
> colors can never be equal to non-extended colors, but that seems wrong when
> they two describe exactly the same color. There are multiple solutions to
> these problems.

Yes, I'll need to fix that. I'm sorry I hadn't noted it before.

I haven't decided whether it will be better to do component equality, or something more fancy where the Color constructor has a hash of existing extended colors and just copies it.

The hash use at the moment is in the CSSParser, for CSSPrimitiveValues. While wasteful, it's ok that otherwise equal extended colors won't hash correctly - we'll just create new ones. Those are quickly turned into Colors used elsewhere.

[Dean Jackson]

Comment on attachment 291857 [details]
Patch

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

>> Source/WebCore/css/parser/CSSParser.cpp:7553
>> +    return Color(name);
> 
> I prefer this syntax:
> 
>     return Color { name };
> 
> I won’t comment about that at every call site below, but I recommend using that style.

Fixed - at least for all the places touched in this patch.

>> Source/WebCore/dom/Document.cpp:823
>> +    m_activeLinkColor = Color("red");
> 
> Could we do this with an RGB value instead of parsing the string "red"?

Fixed to use 255, 0, 0. I was going to put a static red on Color, but of course there is already a red() function to get that channel.

>> Source/WebCore/platform/graphics/Color.cpp:233
>> +        if (!c || c > 0x7F)
> 
> Could use !isASCII(c) instead of "c > 0x7F"

Fixed.

>> Source/WebCore/platform/graphics/Color.cpp:256
>> +        if (foundColor)
> 
> I would write:
> 
>     if (auto* foundColor = findNamedColor(name))

Fixed.

>> Source/WebCore/platform/graphics/Color.cpp:333
>> +        return asExtended()->cssText();
> 
> Should change asExtended() to return a reference instead of a pointer. Unless it’s legal to call it on a non-extended color, and if that is so we should just call asExtended(), not both functions, here.

It's not legal, so I'm now returning an ExtendedColor&.

I assert if this is called on a non-extended color, but otherwise it's the caller's risk. You should always call isExtended() first.

>> Source/WebCore/platform/graphics/Color.h:161
>> +    // This creates an ExtendedColor.
> 
> But should it always do that? What if the colorspace is sRGB and the values exactly match what we can make with integers?

I think that it should create a non-extended color in this case. I've added a FIXME.

[Build Bot]

Comment on attachment 291857 [details]
Patch

Attachment 291857 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2306219

New failing tests:
fast/dom/attribute-legacy-colors.html

[Build Bot]

Created attachment 291863 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291857 [details]
Patch

Attachment 291857 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2306211

New failing tests:
fast/dom/attribute-legacy-colors.html

[Build Bot]

Created attachment 291864 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291857 [details]
Patch

Attachment 291857 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2306268

New failing tests:
fast/dom/attribute-legacy-colors.html

[Build Bot]

Created attachment 291866 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 291857 [details]
Patch

Attachment 291857 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2306263

New failing tests:
fast/dom/attribute-legacy-colors.html

[Build Bot]

Created attachment 291871 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Dean Jackson]

Created attachment 291879 [details]
EWS test

Checking EWS

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/Color.h:164:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/Color.h:164:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:44:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:60:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:76:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:95:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:114:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:130:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:156:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 9 in 15 files


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

[Dean Jackson]

Created attachment 291890 [details]
Patch

Checking EWS again

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/Color.h:164:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/graphics/Color.h:164:  The parameter name "colorSpace" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:44:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:60:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:76:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:95:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:114:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:130:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WebCore/ExtendedColor.cpp:156:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 9 in 15 files


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

[Dean Jackson]

Committed r207442: <http://trac.webkit.org/changeset/207442>

[Caitlin Potter (:caitp)]

FYI: Moving WebCore::Colour::setRGB() to private breaks compilation of Source/WebCore/rendering/RenderThemeEfl.cpp, which still uses those methods on three lines.