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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=68473

Attachment 109741: Patch
https://bugs.webkit.org/attachment.cgi?id=109741&action=review

------- 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
"filterOperationForType"

> Source/WebCore/css/CSSStyleSelector.cpp:5359
> +	       AtomicString url = firstValue->getStringValue();
> +	      
operations.operations().append(ReferenceFilterOperation::create(url,
operationType));

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.