Bug 108351

Summary: [CSS Shaders] Parse custom filter function with the at-rule reference syntax
Product: WebKit Reporter: Max Vujovic <mvujovic>
Component: CSSAssignee: Max Vujovic <mvujovic>
Status: RESOLVED FIXED    
Severity: Normal CC: achicu, allan.jensen, cmarcelo, dino, macpherson, menard, michelangelo, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71392    
Attachments:
Description Flags
Patch
dino: review+
Patch for Landing none

Description Max Vujovic 2013-01-30 10:02:46 PST
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
Comment 1 Max Vujovic 2013-01-30 11:35:36 PST
Created attachment 185526 [details]
Patch
Comment 2 Dean Jackson 2013-01-30 14:00:54 PST
Did you mean to r? this?
Comment 3 Max Vujovic 2013-01-30 14:13:35 PST
(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 :)
Comment 4 Dean Jackson 2013-01-30 18:50:51 PST
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 :)
Comment 5 Max Vujovic 2013-01-31 10:53:34 PST
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.
Comment 6 Max Vujovic 2013-01-31 11:04:41 PST
Created attachment 185816 [details]
Patch for Landing

Running EWS again- I'd like to see this pass on EFL before landing.
Comment 7 Max Vujovic 2013-01-31 13:25:53 PST
Comment on attachment 185816 [details]
Patch for Landing

EFL is green. Setting cq+.
Comment 8 WebKit Review Bot 2013-01-31 13:53:38 PST
Comment on attachment 185816 [details]
Patch for Landing

Clearing flags on attachment: 185816

Committed r141480: <http://trac.webkit.org/changeset/141480>
Comment 9 WebKit Review Bot 2013-01-31 13:53:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexandru Chiculita 2013-02-07 09:16:18 PST
(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.
Comment 11 Max Vujovic 2013-02-07 09:31:33 PST
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.