[webkit-reviews] review granted: [Bug 68473] Parse '-webkit-filter' property syntax : [Attachment 109741] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 5 02:14:07 PDT 2011

Nikolas Zimmermann <zimmermann at kde.org> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 68473: Parse '-webkit-filter' property syntax

Attachment 109741: Patch

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109741&action=review

Looks great, r=me with some minor comments. Unfortunately the patch doesn't
apply at the moment, so we have no EWS results.
If you think it's safe and if you want to watch the bots to fixup any breakage,
feel free to land as-is - if you're not upload a new updated patch, and I'll r+
that as well once we got positive EWS results.

> Source/WebCore/css/CSSParser.cpp:6371
> +    if (equalIgnoringCase(name, "grayscale("))

If this ever shows up hot in a profile, we could check char by char here, but
for now it's fine as-is.

> Source/WebCore/css/CSSParser.cpp:6395
> +bool CSSParser::validFilterArgument(CSSParserValue* argument,
WebKitCSSFilterValue::FilterOperationType& filterType, unsigned argumentCount)

isValidFilterArgument? Seems to be preferred here.

> Source/WebCore/css/CSSStyleSelector.cpp:5294
> +static FilterOperation::OperationType
getFilterOperationType(WebKitCSSFilterValue::FilterOperationType type)

The prefix get here violates the style guide - you should sth. like

> Source/WebCore/css/CSSStyleSelector.cpp:5359
> +	       AtomicString url = firstValue->getStringValue();
> +	      

I'd avoid the local here, seems there's no gain.

> Source/WebCore/css/WebKitCSSFilterValue.cpp:89
> +    result += CSSValueList::cssText() + ")";
> +    return result;

Thinking about this again, I should have suggested to use "return result +
CSSValueList::cssText() + ")";" directly, it's even more efficient.

More information about the webkit-reviews mailing list