Bug 163557 - Allow creation of ExtendedColors and make Color immutable
Summary: Allow creation of ExtendedColors and make Color immutable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-17 12:16 PDT by Dean Jackson
Modified: 2016-10-17 23:57 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.10 KB, patch)
2016-10-17 12:29 PDT, Dean Jackson
hyatt: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.37 MB, application/zip)
2016-10-17 13:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.87 MB, application/zip)
2016-10-17 13:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (1022.90 KB, application/zip)
2016-10-17 13:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (6.57 MB, application/zip)
2016-10-17 14:06 PDT, Build Bot
no flags Details
EWS test (28.50 KB, patch)
2016-10-17 14:44 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (27.86 KB, patch)
2016-10-17 16:04 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2016-10-17 12:16:18 PDT
Allow creation of ExtendedColors and make Color immutable
Comment 1 Radar WebKit Bug Importer 2016-10-17 12:17:19 PDT
<rdar://problem/28805360>
Comment 2 Dean Jackson 2016-10-17 12:29:01 PDT
Created attachment 291857 [details]
Patch
Comment 3 WebKit Commit Bot 2016-10-17 12:31:40 PDT
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.
Comment 4 Dave Hyatt 2016-10-17 12:34:06 PDT
Comment on attachment 291857 [details]
Patch

r=me
Comment 5 Darin Adler 2016-10-17 12:38:58 PDT
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.
Comment 6 Dean Jackson 2016-10-17 13:10:59 PDT
(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.
Comment 7 Dean Jackson 2016-10-17 13:15:18 PDT
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.
Comment 8 Build Bot 2016-10-17 13:36:41 PDT
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
Comment 9 Build Bot 2016-10-17 13:36:44 PDT
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
Comment 10 Build Bot 2016-10-17 13:39:03 PDT
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
Comment 11 Build Bot 2016-10-17 13:39:06 PDT
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
Comment 12 Build Bot 2016-10-17 13:47:01 PDT
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
Comment 13 Build Bot 2016-10-17 13:47:04 PDT
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
Comment 14 Build Bot 2016-10-17 14:06:34 PDT
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
Comment 15 Build Bot 2016-10-17 14:06:37 PDT
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
Comment 16 Dean Jackson 2016-10-17 14:44:11 PDT
Created attachment 291879 [details]
EWS test

Checking EWS
Comment 17 WebKit Commit Bot 2016-10-17 14:45:37 PDT
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.
Comment 18 Dean Jackson 2016-10-17 16:04:07 PDT
Created attachment 291890 [details]
Patch

Checking EWS again
Comment 19 WebKit Commit Bot 2016-10-17 16:34:06 PDT
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.
Comment 20 Dean Jackson 2016-10-17 17:15:11 PDT
Committed r207442: <http://trac.webkit.org/changeset/207442>
Comment 21 Caitlin Potter (:caitp) 2016-10-17 20:36:49 PDT
FYI: Moving WebCore::Colour::setRGB() to private breaks compilation of Source/WebCore/rendering/RenderThemeEfl.cpp, which still uses those methods on three lines.