Bug 148961 – Refine and simplify some color-related code

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

Refine and simplify some color-related code

[Darin Adler]

Created attachment 260763 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

Created attachment 261008 [details]
Patch

[Darin Adler]

Created attachment 261316 [details]
Patch

[Darin Adler]

Passing all the tests now, just need a review. Added a few of the people I discussed this work with to the CC list.

[Anders Carlsson]

Comment on attachment 261316 [details]
Patch

Very nice!

[Said Abou-Hallawa]

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).

[Darin Adler]

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.

[Darin Adler]

Committed r190003: <http://trac.webkit.org/changeset/190003>