Bug 93405

Summary: [CSS Shaders] Invalid shaders should act as pass-through filters
Product: WebKit Reporter: Alexandru Chiculita <achicu>
Component: Layout and RenderingAssignee: Alexandru Chiculita <achicu>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch V1
none
Patch V2
dino: review+
Patch V3 none

Description Alexandru Chiculita 2012-08-07 15:56:37 PDT
If the CSS Shader fails to compile the FECustomFilter should act as a pass-through. Right now garbage/uninitialized memory is shown instead.
Comment 1 Alexandru Chiculita 2012-08-07 18:14:39 PDT
Created attachment 157067 [details]
Patch V1

No CQ+ because this is going to be updated to unpremultiplied calls after 92758 lands.
Comment 2 Alexandru Chiculita 2012-08-07 18:16:31 PDT
Created attachment 157068 [details]
Patch V2

Removed empty line.
Comment 3 Dean Jackson 2012-08-07 18:51:57 PDT
Comment on attachment 157068 [details]
Patch V2

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();
Comment 4 Alexandru Chiculita 2012-08-08 09:38:41 PDT
(In reply to comment #3)
> (From update of attachment 157068 [details])
> 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)
I think we need it so that the images are all loaded by the time the snapshot is taken. I forgot the waitUntilDone call and it seems it worked without it. I suppose it would have made the test flacky.

> 
> > 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();

Thanks, will use that.

Thanks for the review!
Comment 5 Alexandru Chiculita 2012-08-08 16:00:56 PDT
Created attachment 157317 [details]
Patch V3

Posting it back for review because I've added more code for Safari Mac. It seems like the generated expected result had slightly different colors. I suppose that's coming from the FilterEffect data conversion back to an ImageBuffer. I've added a "grayscale(0)" effect on the images in the expected result, just to trigger the same kind of errors on the generated image.
Comment 6 WebKit Review Bot 2012-08-08 17:56:29 PDT
Comment on attachment 157317 [details]
Patch V3

Clearing flags on attachment: 157317

Committed r125128: <http://trac.webkit.org/changeset/125128>
Comment 7 WebKit Review Bot 2012-08-08 17:56:33 PDT
All reviewed patches have been landed.  Closing bug.