Bug 29001 – Drop in="" from <feFlood>

• Component: SVG
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

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

[Cameron McCormack (:heycam)]

Created attachment 39129 [details]
Patch v1

[Eric Seidel (no email)]

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

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

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 on attachment 39129 [details]
Patch v1

LGTM.

[Eric Seidel (no email)]

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

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

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

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

[Eric Seidel (no email)]

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]

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

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

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

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

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

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

landed in r49441. Thanks Cameron.