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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied 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 109393: Patch
https://bugs.webkit.org/attachment.cgi?id=109393&action=review

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


First round of review, nice patch Dean! I didn't check the tests extenisvely,
though they all look thoughtful and sane

> Source/WebCore/css/CSSParser.cpp:6383
> +	       

You can omit this newline.

> Source/WebCore/css/CSSParser.cpp:6387
> +

Ditto.

> Source/WebCore/css/CSSParser.cpp:6396
> +	       if (equalIgnoringCase(name, "grayscale(")) {
> +		   filterType = WebKitCSSFilterValue::GrayscaleFilterOperation;

> +	       } else if (equalIgnoringCase(name, "sepia(")) {
> +		   filterType = WebKitCSSFilterValue::SepiaFilterOperation;

This should go into a static inline helper function, to make the code more
readable!
Also the { .. } braces can be omitted everywhere.

> Source/WebCore/css/CSSParser.cpp:6421
> +	       // Create the new WebKitCSSFilterValue for this operation and
add it to our list.
> +	       RefPtr<WebKitCSSFilterValue> filterValue =
WebKitCSSFilterValue::create(filterType);
> +	       list->append(filterValue);

This should be moved below the next return 0 early return, to avoid creating
and destructing this immediately again.

> Source/WebCore/css/CSSParser.cpp:6431
> +

Unneeded newline.

> Source/WebCore/css/CSSParser.cpp:6432
> +		   // Check parameter types.

I think this whole method should be moved into a helper function. The  current
parseFilter() method is not really readable.

> Source/WebCore/css/CSSParser.cpp:6448
> +		   } else {
> +		       if (!validUnit(argument, FNumber, true))
> +			   return 0;
> +		   }

Can be a joined into else if (!validUnit).

> Source/WebCore/css/CSSParser.cpp:6451
> +
> +		   // Check parameter values.
> +		   if (filterType ==
WebKitCSSFilterValue::GrayscaleFilterOperation

Same here, that cries for another helper function :-)

> Source/WebCore/css/CSSParser.cpp:6484
> +		   argumentCount++;

I'd prefer pre-increment, but it's taste.

> Source/WebCore/css/CSSStyleSelector.cpp:5297
> +    case WebKitCSSFilterValue::ReferenceFilterOperation: return
FilterOperation::REFERENCE;

That's not our preferred style, each statement should go into a new line.

> Source/WebCore/css/CSSStyleSelector.cpp:5353
> +	       double amount = 1.0;

You should omit the .0 per our style guide, constants like 1 should be 1 not
1.0.

> Source/WebCore/css/CSSStyleSelector.cpp:5372
> +	       double amount = 1.0;

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:5382
> +	       double amplitude = 1.0;
> +	       double exponent = 1.0;
> +	       double offset = 0.0;

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:5413
> +	       double amount = 0.0;

Ditto.

> Source/WebCore/css/WebKitCSSFilterValue.cpp:32
> +#include "PlatformString.h"

This is deprecated, please use <wtf/text/WTFString.h>

> Source/WebCore/css/WebKitCSSFilterValue.cpp:37
> +WebKitCSSFilterValue::WebKitCSSFilterValue(FilterOperationType op)

s/op/type/ to avoid abbrevations.

> Source/WebCore/css/WebKitCSSFilterValue.cpp:52
> +	   result += "url(";

The usage of += here is inefficient, just use assignment, as the String is
empty initially.

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

result += CSSValueList::cssText() + ")" is potentially faster, as += always
reallocs, and our string concat operator + is optimized.