Bug 95238

Summary: [skia] Switch FEComponentTransfer to use skia's SkColorFilter
Product: WebKit Reporter: Stephen White <senorblanco>
Component: SVGAssignee: Stephen White <senorblanco>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dino, fmalita, krit, pdr, schenney, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-02
none
Patch
none
Patch
none
Patch none

Stephen White
Reported 2012-08-28 13:22:35 PDT
Switch FEComponentTransfer to use skia's SkColorFilter when compiling for Skia.
Attachments
Patch (9.65 KB, patch)
2012-08-28 13:46 PDT, Stephen White
no flags
Archive of layout-test-results from gce-cr-linux-02 (643.49 KB, application/zip)
2012-08-28 15:41 PDT, WebKit Review Bot
no flags
Patch (9.89 KB, patch)
2012-08-29 07:32 PDT, Stephen White
no flags
Patch (7.15 KB, patch)
2012-09-11 11:37 PDT, Stephen White
no flags
Patch (9.83 KB, patch)
2012-09-11 11:43 PDT, Stephen White
no flags
Stephen White
Comment 1 2012-08-28 13:46:27 PDT
WebKit Review Bot
Comment 2 2012-08-28 15:41:25 PDT
Comment on attachment 161049 [details] Patch Attachment 161049 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13652573 New failing tests: css3/filters/effect-reference-ordering.html css3/filters/effect-combined.html css3/filters/effect-opacity.html
WebKit Review Bot
Comment 3 2012-08-28 15:41:28 PDT
Created attachment 161074 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Stephen White
Comment 4 2012-08-29 07:32:37 PDT
Dirk Schulze
Comment 5 2012-08-29 18:00:59 PDT
I would like to have Philip or Stephen take a look at this first.
Stephen White
Comment 6 2012-09-11 09:23:46 PDT
schenney, pdr: ping?
Stephen Chenney
Comment 7 2012-09-11 10:46:02 PDT
Comment on attachment 161223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161223&action=review > Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 > + virtual bool platformApplySkia(); Where is this actually called? Did the patch fail to pick up a changed file? If this is a preparatory patch then the test expectations shouldn't be updated. > Source/WebCore/platform/graphics/filters/skia/FEComponentTransferSkia.cpp:39 > + if (!in) This is only invoked if there is an inputEffect, so no need to check this.
Florin Malita
Comment 8 2012-09-11 10:49:09 PDT
(In reply to comment #7) > > Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 > > + virtual bool platformApplySkia(); > > Where is this actually called? FilterEffect::apply()
Stephen Chenney
Comment 9 2012-09-11 10:53:34 PDT
(In reply to comment #8) > (In reply to comment #7) > > > Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 > > > + virtual bool platformApplySkia(); > > > > Where is this actually called? > > FilterEffect::apply() Not in this patch. :-)
Stephen White
Comment 10 2012-09-11 10:56:35 PDT
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 > > > > + virtual bool platformApplySkia(); > > > > > > Where is this actually called? > > > > FilterEffect::apply() > > Not in this patch. :-) Florin is correct. It's virtual, and the base class version returns false. This patch picks up the override for FEComponentTransfer.
Stephen Chenney
Comment 11 2012-09-11 11:01:01 PDT
Comment on attachment 161223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161223&action=review >> Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 >> + virtual bool platformApplySkia(); > > Where is this actually called? Did the patch fail to pick up a changed file? If this is a preparatory patch then the test expectations shouldn't be updated. Ah, now I remember how this stupid thing works. Everything defines platformApplySkia but the default returns false. Sorry. >> Source/WebCore/platform/graphics/filters/skia/FEComponentTransferSkia.cpp:39 >> + if (!in) > > This is only invoked if there is an inputEffect, so no need to check this. This is a nit. If a reviewer doesn't care then I don't care, but if I were a reviewer I would care having gotten grief about this kind of thing in the past.
Stephen White
Comment 12 2012-09-11 11:37:50 PDT
Stephen White
Comment 13 2012-09-11 11:43:52 PDT
Stephen White
Comment 14 2012-09-11 12:11:00 PDT
Comment on attachment 161223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161223&action=review Thanks for the review. >>> Source/WebCore/platform/graphics/filters/skia/FEComponentTransferSkia.cpp:39 >>> + if (!in) >> >> This is only invoked if there is an inputEffect, so no need to check this. > > This is a nit. If a reviewer doesn't care then I don't care, but if I were a reviewer I would care having gotten grief about this kind of thing in the past. Fixed.
Stephen Chenney
Comment 15 2012-09-11 13:26:50 PDT
Comment on attachment 163414 [details] Patch I would certainly R+ this. Dirk, over to you, 'cause you have the power.
Dirk Schulze
Comment 16 2012-09-11 13:44:37 PDT
Comment on attachment 163414 [details] Patch r=me
WebKit Review Bot
Comment 17 2012-09-11 13:53:43 PDT
Comment on attachment 163414 [details] Patch Clearing flags on attachment: 163414 Committed r128223: <http://trac.webkit.org/changeset/128223>
WebKit Review Bot
Comment 18 2012-09-11 13:53:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.