Bug 94226

Created attachment 160814 [details] (Not for review) Patch

Comment on attachment 160814 [details] (Not for review) Patch Attachment 160814 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13636067 New failing tests: css3/filters/custom/custom-filter-property-parsing.html

Created attachment 160840 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 161096 [details] Patch not for review

Comment on attachment 161096 [details] Patch not for review Attachment 161096 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13681329 New failing tests: css3/filters/custom/custom-filter-property-parsing.html

Created attachment 161313 [details] Patch

Comment on attachment 161313 [details] Patch Attachment 161313 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13693569 New failing tests: css3/filters/custom/custom-filter-property-parsing.html

Created attachment 161623 [details] Patch

Comment on attachment 161623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161623&action=review Looks good! Some minor comments. > Source/WebCore/css/CSSParser.cpp:7460 > + if (!validUnit(arrayArgParserValue, FNumber, CSSStrictMode) && !isComma(arrayArgParserValue)) I think this can be simplified. > Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:40 > +: CSSValueList(WebKitCSSArrayFunctionValueClass, CommaSeparator) nit: indent here. > Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:45 > +: CSSValueList(cloneFrom) nit: indent here. > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:95 > +testInvalidFilterRule("One comma in array()", "custom(none url(shader), id array(,))"); Should we add a test for array(none) or any other invalid unit. Maybe another one with array(1px). > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:168 > + "custom(none url(fragment.shader), firstTransform rotate(0deg), secondTransform rotate(0deg))", Did you add a comment in the changelog about this? > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing.js:184 > + "custom(none url(fragment.shader), t3 array(1, -2.2, 3.14, 0.4, 5))", Can you do the same thing as you did for transforms? Have more arrays in the same filter.

Created attachment 162119 [details] Patch

Comment on attachment 162119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162119&action=review Quick comments: > Source/WebCore/WebCore.xcodeproj/project.pbxproj:25507 > FBD6AF8C15EF2604008B7110 /* BasicShapes.h in Headers */, > + 150B923A15F08DC400E10986 /* WebKitCSSArrayFunctionValue.h in Headers */, > ); Please sort the build section, it is very helpful for rebaseline. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:28598 > 377A3A9015EFCE9B0059F5C7 /* BitmapImageCG.cpp in Sources */, > + 150B923915F08DC400E10986 /* WebKitCSSArrayFunctionValue.cpp in Sources */, > ); Ditto. > Source/WebCore/css/CSSParser.cpp:7457 > + // array() values are comma separated Comments are sentences, the period is missing. > Source/WebCore/css/CSSParser.cpp:7461 > + // We parse pairs <Value, Comma> at each step Ditto. > Source/WebCore/css/CSSParser.cpp:7469 > + // A valid value is supposed to be followed by a comma, or null, if the > + // ParserValueList is over Ditto. I am not sure this comment is useful, the code bellow is pretty straightforward. > Source/WebCore/css/CSSParser.cpp:7479 > + arrayArgParserValue = arrayArgsParserValueList->next(); > + if (!arrayArgParserValue) > + return 0; This while loop has a strange construct. The condition arrayArgParserValue it tested here and at the beginning. I think you should use a more direct flow. For example: while (true) { if (!arrayArgParserValue) return 0; ... } > Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:51 > + return "array(" + CSSValueList::customCssText() + ")"; ")" -> ')'

Created attachment 162151 [details] Patch

Comment on attachment 162151 [details] Patch Thanks Benjamin for the great review! Patch looks good for me. r=me.

Comment on attachment 162151 [details] Patch Rejecting attachment 162151 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: aining/Source/WebCore/css/WebKitCSSFilterValue.o Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp: In member function 'void WebCore::WebKitCSSArrayFunctionValue::reportDescendantMemoryUsage(WebCore::MemoryObjectInfo*) const': Source/WebCore/css/WebKitCSSArrayFunctionValue.cpp:61: error: 'CSS' is not a member of 'WebCore::MemoryInstrumentation' make: *** [out/Release/obj.target/webcore_remaining/Source/WebCore/css/WebKitCSSArrayFunctionValue.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/13743945

Created attachment 162286 [details] Patch for landing

(In reply to comment #15) > Created an attachment (id=162286) [details] > Patch for landing Nice of you. There are so many cowboys with webkit-patch land...

(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=162286) [details] [details] > > Patch for landing > > Nice of you. There are so many cowboys with webkit-patch land... Actually we agreed to land patches our selfs a long time ago. So don't call them cow boys, they were asked to use webkit-patch land. And I didn't hear another statement since that time.

> Actually we agreed to land patches our selfs a long time ago. So don't call them cow boys, they were asked to use webkit-patch land. And I didn't hear another statement since that time. Oh, I did not intend to point fingers! I am not sure who you are thinking about here. You know what I mean...those non-trivial patches that lands before all bots were green. :)

Comment on attachment 162286 [details] Patch for landing Clearing flags on attachment: 162286 Committed r127682: <http://trac.webkit.org/changeset/127682>

All reviewed patches have been landed. Closing bug.