Bug 5527

so it looks like this bug is also contributing to filters-tile-01-b.svg not working correctly. Two things need to be fixed as far as I can see: 1) We need to properly read in the subregion bounds. If nothing is specified for the filter subregion, it should default to {0%,0% 100%,100%} of the filter region according to the spec. Right now everything is defaulting to {0,0 0,0} if it's not specified. 2) We need to properly clip the subregion. FE_QUARTZ_CROP_TO_RECT calls could work well enough, or we could pull out the subregion specification into the cross platform section to share more code, but I'm not sure how to do that yet. Eric, do you know what would need to be changed in order to fix the first bullet? I haven't been able to figure out what would need to be done in order to fix it.

(In reply to comment #1) > so it looks like this bug is also contributing to filters-tile-01-b.svg not working correctly. Two things > need to be fixed as far as I can see: > > 1) We need to properly read in the subregion bounds. If nothing is specified for the filter subregion, it > should default to {0%,0% 100%,100%} of the filter region according to the spec. Right now everything is > defaulting to {0,0 0,0} if it's not specified. Well, for now we'd probably have to interpret 0,0,0,0 (or !myQRect.isValid()) as being "the default" and just make the code know about the 100% default... Why we'd have to do this, is because the KCanvas doesnt' currently have any concept of "relative lengths" like the khtml render tree does. Eventually this will change, and relative length computations will be made in the ksvg2 render tree instead of the dom. Another solution would be to make the DOM know about this default (this would fit better with the current model) and pass absolute coordinates into KCanvas (as it expects). You could model this off of other dom classes which handle relative lengths. I believe there are existing methods on KCanvasRenderingStyle and KSVG::Helper to help you with this conversion. > 2) We need to properly clip the subregion. FE_QUARTZ_CROP_TO_RECT calls could work well enough, or > we could pull out the subregion specification into the cross platform section to share more code, but > I'm not sure how to do that yet. Well, I don't think clipping can be done in the cross platform section of the code... however, storing of the clipping data should be done there. It should be added to the FEStandardAttributes class if it isn't there already.

Moving to Erick who may already have a fix.

http://bugzilla.opendarwin.org/show_bug.cgi?id=6774 has a good test case for this.

Reassigning to "webkit-unassigned".

*** Bug 6774 has been marked as a duplicate of this bug. ***

A nice test case is attached to: http://bugzilla.opendarwin.org/show_bug.cgi?id=6774

Created attachment 16060 [details] Initial patch This brings back SVG filters in a useable state. Fixes numerous other bugs, too. See ChangeLog.

Created attachment 16061 [details] LayoutTest results

Comment on attachment 16060 [details] Initial patch r=me, provided you deal with th e following issues: Can we use RetainPtr for

Comment on attachment 16060 [details] Initial patch r=me, provided you deal with th e following issues: Can we use RetainPtr for

That should have said can we use RetainPtr for SVGResourceFilter::m_imagesByName I'm unhappy with DEBUG_OUTPUT_IMAGE/dumpOutputImage. Our usual approach would be to have dumpOutputImage available in debug builds, and to then set a break pooint wherever, and call it from the debugger, rather than having yet another flag that effects runtime behaviour. There are also a couple of places you change my copyright notices, while your at it you can update the email address to oliver <at> nerget <dot> com :D

Comment on attachment 16061 [details] LayoutTest results rubber stamped

(In reply to comment #12) > That should have said can we use RetainPtr for > SVGResourceFilter::m_imagesByName To be discussed on IRC. > > I'm unhappy with DEBUG_OUTPUT_IMAGE/dumpOutputImage. Our usual approach would > be to have dumpOutputImage available in debug builds, and to then set a break > pooint wherever, and call it from the debugger, rather than having yet another > flag that effects runtime behaviour. And passing a filename where to store the output? I'm rather fine with this solution as I'm using it in quite a lot tricky places (ie. SVGRootInlineBox & DEBUG_CHUNK_BUILDING). It's just a per-file specific define, don't see a big problem with it. > There are also a couple of places you change my copyright notices, while your > at it you can update the email address to oliver <at> nerget <dot> com :D Okay. Greetings, Niko

Landed in r25175.

This seems broken on TOT. At least the test case for bug 6774.

Comment on attachment 16061 [details] LayoutTest results Clearing flag, since this patch was already landed before the bug was reopened.

Comment on attachment 16060 [details] Initial patch Clearing flag, since this patch was already landed before the bug was reopened.

My mistake. This appears to work fine on TOT. I must have been testing with Safari 3 by accident.