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
Created attachment 185526 [details] Patch
Did you mean to r? this?
(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 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 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.
Created attachment 185816 [details] Patch for Landing Running EWS again- I'd like to see this pass on EFL before landing.
Comment on attachment 185816 [details] Patch for Landing EFL is green. Setting cq+.
Comment on attachment 185816 [details] Patch for Landing Clearing flags on attachment: 185816 Committed r141480: <http://trac.webkit.org/changeset/141480>
All reviewed patches have been landed. Closing bug.
(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 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.