RESOLVED FIXED 29001
Drop in="" from <feFlood>
https://bugs.webkit.org/show_bug.cgi?id=29001
Summary Drop in="" from <feFlood>
Cameron McCormack (:heycam)
Reported Monday, September 7, 2009 1:39:02 AM UTC
The in="" attribute on <feFlood> is useless, and has been dropped from SVG 1.1 Second Edition: http://www.w3.org/2003/01/REC-SVG11-20030114-errata#feflood-attribute
Attachments
Patch v1 (44.64 KB, patch)
2009-09-06 19:00 PDT, Cameron McCormack (:heycam)
eric: review+
commit-queue: commit-queue-
Cameron McCormack (:heycam)
Comment 1 Monday, September 7, 2009 3:00:22 AM UTC
Created attachment 39129 [details] Patch v1
Eric Seidel (no email)
Comment 2 Tuesday, September 8, 2009 5:35:42 PM UTC
Comment on attachment 39129 [details] Patch v1 I'm not sure I understand why pixel tests are needed for some of these?
Eric Seidel (no email)
Comment 3 Tuesday, September 8, 2009 5:36:13 PM UTC
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?
Cameron McCormack (:heycam)
Comment 4 Wednesday, September 9, 2009 4:07:26 AM UTC
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>.
Eric Seidel (no email)
Comment 5 Wednesday, September 9, 2009 6:35:28 AM UTC
Comment on attachment 39129 [details] Patch v1 LGTM.
Eric Seidel (no email)
Comment 6 Wednesday, September 9, 2009 7:15:47 AM UTC
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.
Eric Seidel (no email)
Comment 7 Wednesday, September 9, 2009 5:35:40 PM UTC
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.
Cameron McCormack (:heycam)
Comment 8 Wednesday, September 9, 2009 11:57:24 PM UTC
(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.
WebKit Commit Bot
Comment 9 Thursday, September 10, 2009 2:06:47 AM UTC
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
Eric Seidel (no email)
Comment 10 Thursday, September 10, 2009 2:32:30 AM UTC
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
Eric Seidel (no email)
Comment 11 Thursday, September 10, 2009 2:47:59 AM UTC
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.
WebKit Commit Bot
Comment 12 Tuesday, September 22, 2009 6:27:08 PM UTC
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.
Eric Seidel (no email)
Comment 13 Tuesday, September 22, 2009 6:37:04 PM UTC
I'll take a look when I'm in the office.
Eric Seidel (no email)
Comment 14 Tuesday, September 22, 2009 10:08:27 PM UTC
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.)
Dirk Schulze
Comment 15 Sunday, October 11, 2009 11:02:55 AM UTC
(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.
Cameron McCormack (:heycam)
Comment 16 Sunday, October 11, 2009 11:07:32 AM UTC
(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?
Eric Seidel (no email)
Comment 17 Sunday, October 11, 2009 4:27:09 PM UTC
(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.
Dirk Schulze
Comment 18 Sunday, October 11, 2009 5:18:11 PM UTC
(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.
Dirk Schulze
Comment 19 Sunday, October 11, 2009 5:53:07 PM UTC
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.
Cameron McCormack (:heycam)
Comment 20 Monday, October 12, 2009 11:39:19 AM UTC
(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.
Dirk Schulze
Comment 21 Monday, October 12, 2009 5:15:47 PM UTC
landed in r49441. Thanks Cameron.
Note You need to log in before you can comment on or make changes to this bug.