Bug 29001

Created attachment 39129 [details] Patch v1

Comment on attachment 39129 [details] Patch v1 I'm not sure I understand why pixel tests are needed for some of these?

Or stated otherwise: we prefer dump-as-text tests whenever possible. Maybe that's not possible here. I'm not sure what a bogus in value would do?

Currently, a bogus in="" attribute causes the filter not to be applied. The DRT output still mentions the filter in this case, though. I want to test that the filter is indeed applied, whether or not an in="" attribute is on the <feFlood>.

Comment on attachment 39129 [details] Patch v1 LGTM.

Comment on attachment 39129 [details] Patch v1 Rejecting patch 39129 from commit-queue. This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=39129 from bug 29001 failed to download and apply.

can't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: LayoutTests/svg/dom/SVGFEFloodElement/resources/TEMPLATE.html |=================================================================== |--- LayoutTests/svg/dom/SVGFEFloodElement/resources/TEMPLATE.html (revision 0) |+++ LayoutTests/svg/dom/SVGFEFloodElement/resources/TEMPLATE.html (working copy) -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored Can't open 'LayoutTests/svg/dom/SVGFEFloodElement/resources/TEMPLATE.html': No such file or directory at /Users/eseidel/Projects/WebKit2/WebKitTools/Scripts/svn-apply line 256, <> line 766. I think we'll need a new patch.

(In reply to comment #7) > I think we'll need a new patch. The fix in bug 29059 should let svn-apply work on this patch, too.

Comment on attachment 39129 [details] Patch v1 Rejecting patch 39129 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

Comment on attachment 39129 [details] Patch v1 compositing/color-matching/image-color-matching.html -> timed out The media layout test suffering never ends. bug 28624

Comment on attachment 39129 [details] Patch v1 Actually wrong patch. This one failed from: patching file LayoutTests/svg/filters/feFlood.svg error: pathspec 'LayoutTests/platform/mac/svg/filters/feFlood-expected.png' did not match any file(s) known to git. Did you forget to 'git add'? error: pathspec 'LayoutTests/platform/mac/svg/filters/feFlood-invalid-in-expected.png' did not match any file(s) known to git. Did you forget to 'git add'? Which I think is bug 29100.

Comment on attachment 39129 [details] Patch v1 Rejecting patch 39129 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=39129 from bug 29001 failed to download and apply.

I'll take a look when I'm in the office.

Ahha! The patch is now out of date because TEMPLATE.html, etc. have moved out of resources/ and into script-tests/. Please update the patch, at which point commit-queue+ should work fine. (You also might already have your commit bit by then too.)

(In reply to comment #5) > (From update of attachment 39129 [details]) > LGTM. I'm not sure but to drop: void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) seems wrong to me. Should'nt it be: void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) {   SVGFilterPrimitiveStandardAttributes::parseMappedAttribute(attr); } ??? The rest is ok.

(In reply to comment #15) > I'm not sure but to drop: > > void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) > > seems wrong to me. > > Should'nt it be: > > void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) > { >   SVGFilterPrimitiveStandardAttributes::parseMappedAttribute(attr); > } SVGFEFloodElement inherits from SVGFilterPrimitiveStandardAttributes. Shouldn't dropping the function be equivalent to explicitly calling the one from its parent class?

(In reply to comment #16) > SVGFEFloodElement inherits from SVGFilterPrimitiveStandardAttributes. > Shouldn't dropping the function be equivalent to explicitly calling the one > from its parent class? Yup. Removing it is cleaner.

(In reply to comment #16) > (In reply to comment #15) > > I'm not sure but to drop: > > > > void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) > > > > seems wrong to me. > > > > Should'nt it be: > > > > void SVGFEFloodElement::parseMappedAttribute(MappedAttribute* attr) > > { > >   SVGFilterPrimitiveStandardAttributes::parseMappedAttribute(attr); > > } > > SVGFEFloodElement inherits from SVGFilterPrimitiveStandardAttributes. > Shouldn't dropping the function be equivalent to explicitly calling the one > from its parent class? Yes, sorry.

We have many feFlood pixel-tests in LayoutTests/svg/filters/ Your pixel test doesn't test new functionality or the bug itself. I think feFlood-invalid-in.js is enough. Another point is, that you made the tests with an filters enabled build. Filters are disabled by default and the build-bots will fail with your test results. In my opinion it's the best to put feFlood-invalid-in.js test to LayoutTests/svg/dom/script-tests/feFlood-no-in1.js create the html file with make-script-test-wrappers automaticly and update the test result. Do you want to commit a new patch? I can update and commit the patch in the next days, if you want.

(In reply to comment #19) > Do you want to commit a new patch? I can update and commit the patch in the > next days, if you want. Thanks. I probably won't have time to work on updating this patch, so please feel free to take it on.

landed in r49441. Thanks Cameron.