Summary: | [CSS Shaders] Parse custom filter function with the at-rule reference syntax | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Max Vujovic <mvujovic> | ||||||
Component: | CSS | Assignee: | 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
Max Vujovic
2013-01-30 10:02:46 PST
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. |