Bug 68473

Summary: Parse '-webkit-filter' property syntax
Product: WebKit Reporter: Dean Jackson <dino>
Component: CSSAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, krit, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 68474, 68675    
Bug Blocks: 68469, 68475, 68477    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch zimmermann: review+

Description Dean Jackson 2011-09-20 14:14:52 PDT
https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/publish/Filters.html

has a new filter property syntax. We need to support an arbitrary list of filters.
Comment 1 Radar WebKit Bug Importer 2011-09-20 14:15:10 PDT
<rdar://problem/10155710>
Comment 2 Dean Jackson 2011-09-30 19:18:59 PDT
Created attachment 109392 [details]
Patch
Comment 3 Dean Jackson 2011-09-30 19:21:04 PDT
Patch uploaded. This intentionally leaves drop-shadow to a followup. Unfortunately adding the new type for WebKitCSSFilterValue meant I had to copy some test results into the platform-specific area, while ENABLE(CSS_FILTERS) is only turned on for those ports.
Comment 4 Early Warning System Bot 2011-09-30 19:31:43 PDT
Comment on attachment 109392 [details]
Patch

Attachment 109392 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9893832
Comment 5 Gyuyoung Kim 2011-09-30 19:34:09 PDT
Comment on attachment 109392 [details]
Patch

Attachment 109392 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9903452
Comment 6 Dean Jackson 2011-09-30 19:40:10 PDT
Created attachment 109393 [details]
Patch
Comment 7 Nikolas Zimmermann 2011-10-02 03:43:49 PDT
Comment on attachment 109393 [details]
Patch

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.
Comment 8 Dean Jackson 2011-10-04 14:22:59 PDT
Created attachment 109688 [details]
Patch
Comment 9 Dean Jackson 2011-10-04 14:25:33 PDT
(In reply to comment #7)
> (From update of attachment 109393 [details])
> 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
> 

Thanks Nikolas. I fixed everything you mentioned. The biggest changes are the two helper functions (one static, one unfortunately on CSSParser in order to access validUnit) in the parseFilter.
Comment 10 Dean Jackson 2011-10-04 21:08:06 PDT
Created attachment 109741 [details]
Patch
Comment 11 Nikolas Zimmermann 2011-10-05 02:14:08 PDT
Comment on attachment 109741 [details]
Patch

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.
Comment 12 Dean Jackson 2011-10-05 16:28:37 PDT
Thanks Nikolas.

I'm watching the bots.

http://trac.webkit.org/changeset/96764
Comment 13 Dirk Schulze 2011-11-16 23:09:55 PST
Will '-webkit-filter' replace the 'filter' property that we already have?
Comment 14 Dean Jackson 2011-11-16 23:36:09 PST
(In reply to comment #13)
> Will '-webkit-filter' replace the 'filter' property that we already have?

Eventually. That's what the spec says. We'll wait until CR I guess.

(Not sure what to do when we want -webkit-filter to apply to SVG)
Comment 15 Dirk Schulze 2011-11-17 03:10:41 PST
(In reply to comment #14)
> (In reply to comment #13)
> > Will '-webkit-filter' replace the 'filter' property that we already have?
> 
> Eventually. That's what the spec says. We'll wait until CR I guess.
> 
> (Not sure what to do when we want -webkit-filter to apply to SVG)

It depends how how you move forward with implementing filters for HTML, I don't see any problems with CSS Filters for SVG elements, as long as the code is not HTML dependent. The next step for SVG might be to use RenderLayers for filtered elements (for instance for CSS Shaders).