Bug 12569 - WebKit does not handle missing filter elements correctly
Summary: WebKit does not handle missing filter elements correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Rob Buis
URL: http://www.w3.org/Graphics/SVG/Test/2...
Keywords:
Depends on: 5527
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2007-02-04 03:24 PST by Eric Seidel
Modified: 2014-05-12 05:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.33 KB, patch)
2011-05-25 09:19 PDT, Rob Buis
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.25 MB, application/zip)
2011-05-25 11:43 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2007-02-04 03:24:43 PST
WebKit does not handle missing filter elements correctly

This should be pretty easy to fix.
Comment 1 Eric Seidel 2007-02-06 07:51:25 PST
the top-right one is easy to fix.   it means adding something like this to all the renderers:

if (!filterId.isNull() && !filter)
    return;
Comment 2 Rob Buis 2011-05-25 09:19:13 PDT
Created attachment 94791 [details]
Patch
Comment 3 Eric Seidel 2011-05-25 09:25:09 PDT
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
Comment 4 Eric Seidel 2011-05-25 09:26:03 PDT
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
Comment 5 Eric Seidel 2011-05-25 09:49:14 PDT
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?
Comment 6 Dirk Schulze 2011-05-25 10:10:24 PDT
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?
Comment 7 Rob Buis 2011-05-25 10:40:44 PDT
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.
Comment 8 Rob Buis 2011-05-25 11:08:41 PDT
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.
Comment 9 WebKit Review Bot 2011-05-25 11:43:13 PDT
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
Comment 10 WebKit Review Bot 2011-05-25 11:43:18 PDT
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
Comment 11 Rob Buis 2011-05-25 12:02:06 PDT
Committed r87310: <http://trac.webkit.org/changeset/87310>
Comment 12 Adam Klein 2011-05-25 15:22:27 PDT
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.
Comment 13 Adam Klein 2011-05-25 15:28:01 PDT
(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.
Comment 14 Rob Buis 2011-05-25 15:29:58 PDT
(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.
Comment 15 Adam Klein 2011-05-25 16:41:18 PDT
(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.