Bug 148961 - Refine and simplify some color-related code
Summary: Refine and simplify some color-related code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-08 09:12 PDT by Darin Adler
Modified: 2015-09-18 18:25 PDT (History)
8 users (show)

See Also:


Attachments
Patch (33.95 KB, patch)
2015-09-08 09:25 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (1.10 MB, application/zip)
2015-09-08 10:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (1.06 MB, application/zip)
2015-09-08 10:32 PDT, Build Bot
no flags Details
Patch (34.86 KB, patch)
2015-09-11 10:08 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (33.96 KB, patch)
2015-09-16 09:52 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2015-09-08 09:12:50 PDT
Refine and simplify some color-related code
Comment 1 Darin Adler 2015-09-08 09:25:19 PDT
Created attachment 260763 [details]
Patch
Comment 2 Build Bot 2015-09-08 10:15:20 PDT
Comment on attachment 260763 [details]
Patch

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

New failing tests:
svg/animations/additive-type-by-animation.html
svg/animations/animateColor-additive-2d.svg
svg/animations/animateColor-additive-2b.svg
svg/animations/animate-color-fill-from-by.html
imported/mozilla/svg/smil/style/anim-css-fill-1-by-ident-hex.svg
svg/animations/animateColor-additive-2c.svg
imported/mozilla/svg/smil/style/anim-css-color-1-by-ident-hex.svg
Comment 3 Build Bot 2015-09-08 10:15:22 PDT
Created attachment 260766 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 4 Build Bot 2015-09-08 10:32:18 PDT
Comment on attachment 260763 [details]
Patch

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

New failing tests:
svg/animations/additive-type-by-animation.html
svg/animations/animateColor-additive-2d.svg
svg/animations/animateColor-additive-2b.svg
svg/animations/animate-color-fill-from-by.html
imported/mozilla/svg/smil/style/anim-css-fill-1-by-ident-hex.svg
svg/animations/animateColor-additive-2c.svg
imported/mozilla/svg/smil/style/anim-css-color-1-by-ident-hex.svg
Comment 5 Build Bot 2015-09-08 10:32:21 PDT
Created attachment 260767 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 6 Darin Adler 2015-09-11 10:08:26 PDT
Created attachment 261008 [details]
Patch
Comment 7 Darin Adler 2015-09-16 09:52:00 PDT
Created attachment 261316 [details]
Patch
Comment 8 Darin Adler 2015-09-18 07:16:16 PDT
Passing all the tests now, just need a review. Added a few of the people I discussed this work with to the CC list.
Comment 9 Anders Carlsson 2015-09-18 08:57:17 PDT
Comment on attachment 261316 [details]
Patch

Very nice!
Comment 10 Said Abou-Hallawa 2015-09-18 09:19:01 PDT
Comment on attachment 261316 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        * svg/ColorDistance.h: Removed.

Can't we incorporate the functions in ColorDistance in Color? Some like Color::operator+() and Color::distance()?

> Source/WebCore/platform/graphics/Color.h:67
> +class RGBA {

Now we have RGBA32 which is unsigned int and RGBA which is class contains a member of type unsigned it. Since they basically do the same thing, can't we just have RGBA32 to be the new class and use its members red(), green(), blue() and alpha() instead of the bitwise operations we do in the Color class?

> Source/WebCore/platform/graphics/Color.h:236
> +    return m_integer >> 16;

Can't we use redChannel()? Why is not this function defined inline in the RGBA class like Color::red()?

> Source/WebCore/platform/graphics/Color.h:241
> +    return m_integer >> 8;

Ditto.

> Source/WebCore/platform/graphics/Color.h:246
> +    return m_integer;

Ditto.

> Source/WebCore/platform/graphics/Color.h:251
> +    return m_integer >> 24;

Ditto.

> Source/WebCore/platform/graphics/Color.h:256
> +    return (m_integer & 0xFF000000) != 0xFF000000;

Why do not we use the alpha() function instead of repeating the calculation? Color uses the formula: alpha() < 255.

> Source/WebCore/svg/SVGAnimatedColor.cpp:51
> +        roundAndClampColorChannel(toColor.blue() + fromColor.blue()) };

Can't we define Color::operator+() and use it here? The color components have to be clamped to the maximum after addition.

> Source/WebCore/svg/SVGAnimatedColor.cpp:114
> +    return sqrtf(red * red + green * green + blue * blue);

Should we define this operation in the Color class itself? Some like Color Color::distance(const Color&other).
Comment 11 Darin Adler 2015-09-18 18:24:10 PDT
Comment on attachment 261316 [details]
Patch

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

>> Source/WebCore/ChangeLog:21
>> +        * svg/ColorDistance.h: Removed.
> 
> Can't we incorporate the functions in ColorDistance in Color? Some like Color::operator+() and Color::distance()?

We could if they are complex enough and used more than one place. I don’t want to build them into Color until I am sure of that.

Also, these are operations that don’t make sense on two colors with arbitrary color spaces, so I am not sure they belong in the new Color class, which should be including the color space.

>> Source/WebCore/platform/graphics/Color.h:67
>> +class RGBA {
> 
> Now we have RGBA32 which is unsigned int and RGBA which is class contains a member of type unsigned it. Since they basically do the same thing, can't we just have RGBA32 to be the new class and use its members red(), green(), blue() and alpha() instead of the bitwise operations we do in the Color class?

RGBA32 is going away. That’s what it means above when I say “deprecated”. Sorry I didn’t discuss this with you in person. I’d love to do that some time.

>> Source/WebCore/platform/graphics/Color.h:236
>> +    return m_integer >> 16;
> 
> Can't we use redChannel()? Why is not this function defined inline in the RGBA class like Color::red()?

I am going to be getting rid of redChannel. I plan to move the other function definitions outside of the classes eventually. This makes the class easier to read.

>> Source/WebCore/platform/graphics/Color.h:256
>> +    return (m_integer & 0xFF000000) != 0xFF000000;
> 
> Why do not we use the alpha() function instead of repeating the calculation? Color uses the formula: alpha() < 255.

I believe this is more efficient than that. But I should probably test that it is. The function in Color will eventually be deleted as I complete my refactoring.
Comment 12 Darin Adler 2015-09-18 18:25:51 PDT
Committed r190003: <http://trac.webkit.org/changeset/190003>