Bug 12569 – WebKit does not handle missing filter elements correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

WebKit does not handle missing filter elements correctly

This should be pretty easy to fix.

[Eric Seidel (no email)]

the top-right one is easy to fix.   it means adding something like this to all the renderers:

if (!filterId.isNull() && !filter)
    return;

[Rob Buis]

Created attachment 94791 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 94791 [details]
Patch

So do We pass the test now?  I see a lot of red, but I assume this is an improvement?  Do we need another big to fix the rest?  Could we file a bug with the w3c to fix their confusing test

[Eric Seidel (no email)]

Comment on attachment 94791 [details]
Patch

So do We pass the test now?  I see a lot of red, but I assume this is an improvement?  Do we need another big to fix the rest?  Could we file a bug with the w3c to fix their confusing test

[Eric Seidel (no email)]

Comment on attachment 94791 [details]
Patch

OK. Do we have more to fix here?  Do we need to file another bug?  I see a lot of red here. Is the test just confusing?  Do we need to file a big with the w3c to have them unconfuse it?

[Dirk Schulze]

Comment on attachment 94791 [details]
Patch

I'd like to see a test where we add the non-existant filter after the rendering, something like a repaint drt test. Also can you add the test from SVG1.1se test suite?

[Rob Buis]

Hi Eric,

(In reply to comment #5)
> (From update of attachment 94791 [details])
> OK. Do we have more to fix here?  Do we need to file another bug?  I see a lot of red here. Is the test just confusing?  Do we need to file a big with the w3c to have them unconfuse it?

The SVG 2nd edition test seems a lot better:

http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-felem-01-b.svg

I'll be sure to include this in the patch.
Cheers,

Rob.

[Rob Buis]

To be clear, we do pass both 1.1 and 1.1 SE tests with my patch. Only the top-right circle was wrong, and the patch fixes it.
Cheers,

Rob.

(In reply to comment #7)
> Hi Eric,
> 
> (In reply to comment #5)
> > (From update of attachment 94791 [details] [details])
> > OK. Do we have more to fix here?  Do we need to file another bug?  I see a lot of red here. Is the test just confusing?  Do we need to file a big with the w3c to have them unconfuse it?
> 
> The SVG 2nd edition test seems a lot better:
> 
> http://dev.w3.org/SVG/profiles/1.1F2/test/svg/filters-felem-01-b.svg
> 
> I'll be sure to include this in the patch.
> Cheers,
> 
> Rob.

[WebKit Review Bot]

Comment on attachment 94791 [details]
Patch

Attachment 94791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8734497

New failing tests:
inspector/profiler/cpu-profiler-profiling.html

[WebKit Review Bot]

Created attachment 94819 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Committed r87310: <http://trac.webkit.org/changeset/87310>

[Adam Klein]

r87310 seems to have caused regressions on the Chromium/WebKit bots:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1-SE%2Ffilters-felem-01-b.svg%2Csvg%2FW3C-SVG-1.1%2Ffilters-felem-01-b.svg&group=%40ToT%20-%20chromium.org

For the new test, it looks like we just need to add baselines.  But the old test regressed, and I can't tell for sure whether this is an improvement just by looking.  Please take a look.

[Adam Klein]

(In reply to comment #12)
> r87310 seems to have caused regressions on the Chromium/WebKit bots:
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1-SE%2Ffilters-felem-01-b.svg%2Csvg%2FW3C-SVG-1.1%2Ffilters-felem-01-b.svg&group=%40ToT%20-%20chromium.org
> 
> For the new test, it looks like we just need to add baselines.  But the old test regressed, and I can't tell for sure whether this is an improvement just by looking.  Please take a look.

Actually, looking more closely at the diffs, it looks like the Chromium results just need to be rebaselined all around.

[Rob Buis]

(In reply to comment #13)
> (In reply to comment #12)
> > r87310 seems to have caused regressions on the Chromium/WebKit bots:
> > 
> > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1-SE%2Ffilters-felem-01-b.svg%2Csvg%2FW3C-SVG-1.1%2Ffilters-felem-01-b.svg&group=%40ToT%20-%20chromium.org
> > 
> > For the new test, it looks like we just need to add baselines.  But the old test regressed, and I can't tell for sure whether this is an improvement just by looking.  Please take a look.
> 
> Actually, looking more closely at the diffs, it looks like the Chromium results just need to be rebaselined all around.

Hi Adam,

Agreed. It is an improvement btw and matches how the w3c want it to look.
Cheers,

Rob.

[Adam Klein]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > r87310 seems to have caused regressions on the Chromium/WebKit bots:
> > > 
> > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2FW3C-SVG-1.1-SE%2Ffilters-felem-01-b.svg%2Csvg%2FW3C-SVG-1.1%2Ffilters-felem-01-b.svg&group=%40ToT%20-%20chromium.org
> > > 
> > > For the new test, it looks like we just need to add baselines.  But the old test regressed, and I can't tell for sure whether this is an improvement just by looking.  Please take a look.
> > 
> > Actually, looking more closely at the diffs, it looks like the Chromium results just need to be rebaselined all around.
> 
> Hi Adam,
> 
> Agreed. It is an improvement btw and matches how the w3c want it to look.
> Cheers,
> 
> Rob.

Thanks.  Rebaselines submitted in http://trac.webkit.org/changeset/87334.