RESOLVED FIXED 148961
Refine and simplify some color-related code
https://bugs.webkit.org/show_bug.cgi?id=148961
Summary Refine and simplify some color-related code
Darin Adler
Reported 2015-09-08 09:12:50 PDT
Refine and simplify some color-related code
Attachments
Patch (33.95 KB, patch)
2015-09-08 09:25 PDT, Darin Adler
no flags
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
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
Patch (34.86 KB, patch)
2015-09-11 10:08 PDT, Darin Adler
no flags
Patch (33.96 KB, patch)
2015-09-16 09:52 PDT, Darin Adler
andersca: review+
Darin Adler
Comment 1 2015-09-08 09:25:19 PDT
Build Bot
Comment 2 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
Build Bot
Comment 3 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
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Darin Adler
Comment 6 2015-09-11 10:08:26 PDT
Darin Adler
Comment 7 2015-09-16 09:52:00 PDT
Darin Adler
Comment 8 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.
Anders Carlsson
Comment 9 2015-09-18 08:57:17 PDT
Comment on attachment 261316 [details] Patch Very nice!
Said Abou-Hallawa
Comment 10 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).
Darin Adler
Comment 11 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.
Darin Adler
Comment 12 2015-09-18 18:25:51 PDT
Note You need to log in before you can comment on or make changes to this bug.