[webkit-reviews] review granted: [Bug 93405] [CSS Shaders] Invalid shaders should act as pass-through filters : [Attachment 157068] Patch V2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 7 18:51:32 PDT 2012
Dean Jackson <dino at apple.com> has granted Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 93405: [CSS Shaders] Invalid shaders should act as pass-through filters
https://bugs.webkit.org/show_bug.cgi?id=93405
Attachment 157068: Patch V2
https://bugs.webkit.org/attachment.cgi?id=157068&action=review
------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=157068&action=review
> LayoutTests/css3/filters/custom/invalid-custom-filter-shader-expected.html:10
> + function runTest()
> + {
> + // We need to run the tests after the downloading succeeded
or fails.
> + if (window.testRunner)
> + window.testRunner.notifyDone();
> + }
Why do we need this in the ref test? (I don't really understand how this works
- you don't wait here)
> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:130
> +void FECustomFilter::cleanupShaderResult()
I don't like the name "cleanup" (it's actually two words, so would be
"cleanUp"). Maybe "reset" or "clear"? Sorry, I don't have a good suggestion.
> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:142
> +bool FECustomFilter::tryToApplyShader()
I don't like this name much either :) Saying "try to" is strange.
Maybe just "applyShader"? That would make the platformApplySoftware into
something like:
if (!applyShader())
clearShaderResult();
More information about the webkit-reviews
mailing list