Refine and simplify some color-related code
Created attachment 260763 [details] Patch
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
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 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
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
Created attachment 261008 [details] Patch
Created attachment 261316 [details] Patch
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 on attachment 261316 [details] Patch Very nice!
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 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.
Committed r190003: <http://trac.webkit.org/changeset/190003>