Summary: | [skia] Switch FEComponentTransfer to use skia's SkColorFilter | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen White <senorblanco> | ||||||||||||
Component: | SVG | Assignee: | 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
Stephen White
2012-08-28 13:22:35 PDT
Created attachment 161049 [details]
Patch
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 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
Created attachment 161223 [details]
Patch
I would like to have Philip or Stephen take a look at this first. schenney, pdr: ping? 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. (In reply to comment #7) > > Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:83 > > + virtual bool platformApplySkia(); > > Where is this actually called? FilterEffect::apply() (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. :-) (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. 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. Created attachment 163412 [details]
Patch
Created attachment 163414 [details]
Patch
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. Comment on attachment 163414 [details]
Patch
I would certainly R+ this. Dirk, over to you, 'cause you have the power.
Comment on attachment 163414 [details]
Patch
r=me
Comment on attachment 163414 [details] Patch Clearing flags on attachment: 163414 Committed r128223: <http://trac.webkit.org/changeset/128223> All reviewed patches have been landed. Closing bug. |