Bug 29001

Summary: Drop in="" from <feFlood>
Product: WebKit Reporter: Cameron McCormack <cam>
Component: SVGAssignee: Cameron McCormack <cam>
Status: RESOLVED FIXED    
Severity: Minor CC: eric, krit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 29059, 29100    
Bug Blocks: 68469, 26389    
Attachments:
Description Flags
Patch v1 eric: review+, commit-queue: commit-queue-

Description Cameron McCormack 2009-09-06 17:39:02 PDT
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
Comment 1 Cameron McCormack 2009-09-06 19:00:22 PDT
Created attachment 39129 [details]
Patch v1
Comment 2 Eric Seidel (no email) 2009-09-08 09:35:42 PDT
Comment on attachment 39129 [details]
Patch v1

I'm not sure I understand why pixel tests are needed for some of these?
Comment 3 Eric Seidel (no email) 2009-09-08 09:36:13 PDT
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?
Comment 4 Cameron McCormack 2009-09-08 20:07:26 PDT
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 5 Eric Seidel (no email) 2009-09-08 22:35:28 PDT
Comment on attachment 39129 [details]
Patch v1

LGTM.
Comment 6 Eric Seidel (no email) 2009-09-08 23:15:47 PDT
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.
Comment 7 Eric Seidel (no email) 2009-09-09 09:35:40 PDT
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.
Comment 8 Cameron McCormack 2009-09-09 15:57:24 PDT
(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 9 WebKit Commit Bot 2009-09-09 18:06:47 PDT
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 10 Eric Seidel (no email) 2009-09-09 18:32:30 PDT
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 11 Eric Seidel (no email) 2009-09-09 18:47:59 PDT
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 12 WebKit Commit Bot 2009-09-22 10:27:08 PDT
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.
Comment 13 Eric Seidel (no email) 2009-09-22 10:37:04 PDT
I'll take a look when I'm in the office.
Comment 14 Eric Seidel (no email) 2009-09-22 14:08:27 PDT
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.)
Comment 15 Dirk Schulze 2009-10-11 03:02:55 PDT
(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.
Comment 16 Cameron McCormack 2009-10-11 03:07:32 PDT
(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?
Comment 17 Eric Seidel (no email) 2009-10-11 08:27:09 PDT
(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.
Comment 18 Dirk Schulze 2009-10-11 09:18:11 PDT
(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.
Comment 19 Dirk Schulze 2009-10-11 09:53:07 PDT
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.
Comment 20 Cameron McCormack 2009-10-12 03:39:19 PDT
(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.
Comment 21 Dirk Schulze 2009-10-12 09:15:47 PDT
landed in r49441. Thanks Cameron.