Bug 93405 – [CSS Shaders] Invalid shaders should act as pass-through filters

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

If the CSS Shader fails to compile the FECustomFilter should act as a pass-through. Right now garbage/uninitialized memory is shown instead.

[Alexandru Chiculita]

Created attachment 157067 [details]
Patch V1

No CQ+ because this is going to be updated to unpremultiplied calls after 92758 lands.

[Alexandru Chiculita]

Created attachment 157068 [details]
Patch V2

Removed empty line.

[Dean Jackson]

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

[Alexandru Chiculita]

(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!

[Alexandru Chiculita]

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.

[WebKit Review Bot]

Comment on attachment 157317 [details]
Patch V3

Clearing flags on attachment: 157317

Committed r125128: <http://trac.webkit.org/changeset/125128>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.