<rdar://problem/10155710>

Created attachment 109392 [details] Patch

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 on attachment 109392 [details] Patch Attachment 109392 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9893832

Comment on attachment 109392 [details] Patch Attachment 109392 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9903452

Created attachment 109393 [details] Patch

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.

Created attachment 109688 [details] Patch

(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.

Created attachment 109741 [details] Patch

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.

Thanks Nikolas. I'm watching the bots. http://trac.webkit.org/changeset/96764

Will '-webkit-filter' replace the 'filter' property that we already have?

(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)

(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).