Bug 94020 – [CSS Shaders] Change the default compositing mode and the default CSS value for <fragmentShader>

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Max Vujovic]

The spec has changed:
- The default compositing mode is now source-atop, not source-over.
- The default value for fragmentShader is now "mix(<default-fragment-shader> normal source-atop)", not "<default-fragment-shader>".

https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#fragmentShader-attribute

[Michelangelo De Simone]

Created attachment 171304 [details]
Patatch not for formal review

[Max Vujovic]

Comment on attachment 171304 [details]
Patatch not for formal review

Thanks for the patch! A couple of comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=171304&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:924
> +                if (program->programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE) {

To avoid nesting this if statement, we could do something like:

if (!program->fragmentShader()) {
    shadersList->append(cssValuePool().createIdentifierValue(CSSValueNone));
} else if (program->programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE) {
    ...
} else {
    shadersList->append(program->fragmentShader()->cssValue());
}

It looks prettier, but it might be less clear than the original, so I'm not sure.

> Source/WebCore/css/StyleResolver.cpp:4780
> +    unsigned shadersListLength = shadersList->length();

Don't extract this into a local var unless you use it twice.

> Source/WebCore/css/StyleResolver.cpp:4788
> +    programType = PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE;

Set this default value when you declare the local var programType.

> LayoutTests/css3/filters/resources/empty-shader.fs:-1
> -void main()

Nice. It's good we don't need this shader anymore.

[WebKit Review Bot]

Comment on attachment 171304 [details]
Patatch not for formal review

Attachment 171304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14627279

New failing tests:
css3/filters/custom/custom-filter-property-computed-style.html

[Michelangelo De Simone]

(In reply to comment #2)

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:924
> > +                if (program->programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE) {
> To avoid nesting this if statement, we could do something like:
> 
> if (!program->fragmentShader()) {
>     shadersList->append(cssValuePool().createIdentifierValue(CSSValueNone));
> } else if (program->programType() == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE) {
>     ...
> } else {
>     shadersList->append(program->fragmentShader()->cssValue());
> }
> 
> It looks prettier, but it might be less clear than the original, so I'm not sure.

Neither am I. I'll leave it as it is for now, waiting for other feedbacks.

> > Source/WebCore/css/StyleResolver.cpp:4780
> > +    unsigned shadersListLength = shadersList->length();
> Don't extract this into a local var unless you use it twice.

It is indeed (assert and branch), that's the reason why I changed it.

> Set this default value when you declare the local var programType.

Oops, my fault, I just forgot it there.:)

> > LayoutTests/css3/filters/resources/empty-shader.fs:-1
> > -void main()
> Nice. It's good we don't need this shader anymore.

Getting rid of old stuff is always nice.:)

[Michelangelo De Simone]

Created attachment 171332 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171332 [details]
Patch

Clearing flags on attachment: 171332

Committed r132892: <http://trac.webkit.org/changeset/132892>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.