Bug 108351 – [CSS Shaders] Parse custom filter function with the at-rule reference syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Max Vujovic]

The custom function syntax has changed in the spec. Instead of including all of the custom filter information inline, the custom function can now reference an @filter rule by name. The custom function can still accept a list of parameters.

The syntax is defined as the following, where IDENT is the name of the @filter rule:
filter: custom(IDENT [, <param>]*)

In practice, it can look like this:
filter: custom(page-curl, direction 90, amount 0.5);

Spec: https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#customident-ltparamgt

[Max Vujovic]

Created attachment 185526 [details]
Patch

[Dean Jackson]

Did you mean to r? this?

[Max Vujovic]

(In reply to comment #2)
> Did you mean to r? this?

I asked Alex to take a first look at it before bugging you, but it is ready for your review if you're so inclined :)

[Dean Jackson]

Comment on attachment 185526 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:8309
> +        argsList->next();
> +
> +        if (!(arg = argsList->current()))

You could write this as:
arg = argsList->next();
if (!arg)

Maybe I just dislike assignments in conditionals :)

[Max Vujovic]

Comment on attachment 185526 [details]
Patch

Thank you very much for the review!

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

>> Source/WebCore/css/CSSParser.cpp:8309
>> +        if (!(arg = argsList->current()))
> 
> You could write this as:
> arg = argsList->next();
> if (!arg)
> 
> Maybe I just dislike assignments in conditionals :)

That's much easier on the eyes. I'll do it instead :)

> Source/WebCore/css/CSSParser.cpp:8327
> +            while ((arg = argsList->current())) {

In the spirit of avoiding assignments in conditionals, I'll also refactor this to:

arg = argsList->current();
while (arg) {
    ...
    arg = argsList->next();
}

I won't touch the code that was moved to parseCustomFilterFunctionWithInlineSyntax, since that code is on its way out.

[Max Vujovic]

Created attachment 185816 [details]
Patch for Landing

Running EWS again- I'd like to see this pass on EFL before landing.

[Max Vujovic]

Comment on attachment 185816 [details]
Patch for Landing

EFL is green. Setting cq+.

[WebKit Review Bot]

Comment on attachment 185816 [details]
Patch for Landing

Clearing flags on attachment: 185816

Committed r141480: <http://trac.webkit.org/changeset/141480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 185526 [details])
> Thank you very much for the review!
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=185526&action=review
> 
> >> Source/WebCore/css/CSSParser.cpp:8309
> >> +        if (!(arg = argsList->current()))
> > 
> > You could write this as:
> > arg = argsList->next();
> > if (!arg)
> > 
> > Maybe I just dislike assignments in conditionals :)
> 
> That's much easier on the eyes. I'll do it instead :)
> 
> > Source/WebCore/css/CSSParser.cpp:8327
> > +            while ((arg = argsList->current())) {
> 
> In the spirit of avoiding assignments in conditionals, I'll also refactor this to:
> 
> arg = argsList->current();
> while (arg) {
>     ...
>     arg = argsList->next();
> }
> 
That examples looks like a good candidate for a "for" statement instead.

for (arg = argList->current(); arg; arg = argList->next()) .. this way "continue" statements that might be added would not need to do their own next() call.

[Max Vujovic]

Comment on attachment 185526 [details]
Patch

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

>>>> Source/WebCore/css/CSSParser.cpp:8309
>>>> +        if (!(arg = argsList->current()))
>>> 
>>> You could write this as:
>>> arg = argsList->next();
>>> if (!arg)
>>> 
>>> Maybe I just dislike assignments in conditionals :)
>> 
>> That's much easier on the eyes. I'll do it instead :)
> 
> That examples looks like a good candidate for a "for" statement instead.
> 
> for (arg = argList->current(); arg; arg = argList->next()) .. this way "continue" statements that might be added would not need to do their own next() call.

Yeah, I think that could work. I'll look into it next time we touch this area of code.