Summary: | SVG: method .getTransformToElement(elt) in SVGLocatable not implemented | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Neumann <a.neumann> | ||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | mrowe | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 420+ | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||
URL: | http://www.carto.net/neumann/webkitsvgbugs/SVGLocatableTest.svg | ||||||||||||||||
Attachments: |
|
Description
Andreas Neumann
2006-11-21 08:31:40 PST
Created attachment 11618 [details]
here is another testcase executed onload. The expected results are from Batik (which I believe, are right) and the other results are filled in by the SVG UA onload.
There was some confusion about this method. Firefox and Opera did it different in comparison to Batik and ASV6. ASV3 did not implement this method unfortunately. So, if you test against a different implementation I'll recommend Batik or a SVN/CVS version of Firefox/Opera.
Created attachment 11630 [details]
First attempt
I worked Andreas' testcase into one suitable for webkit tests.
Cheers,
Rob.
Created attachment 11635 [details]
Improved patch
Thi patch is an improvement, since it reports back any exceptions that get set while performing
this function, ie. SVG_MATRIX_NOT_INVERTABLE.
Cheers,
Rob.
Created attachment 11636 [details]
Improved patch
This patch fixes a memleak involving getCTM.
Cheers,
Rob.
Comment on attachment 11636 [details]
Improved patch
Ok, I have two concerns.
1. We're mallocing when not necessary:
+ SVGMatrix* ctm = SVGSVGElement::createSVGMatrix();
+ ctm->multiply(startctm);
In the error condition, no malloc is needed. Ideally we would just move off of SVGMatrix internally (this function could take/return AffineTransforms, and let something outside of it do the wrapping). I'm not sure it's worth going through contortions to avoid mallocs in error conditions however. Consider making this an AffineTransform in/out function though.
2. We need tests for the error conditions. I see this behavior:
+ if (ec)
+ return ctm;
Does that match the spec? Does that match firefox/opera? It would be good to test.
Otherwise it looks fine.
Hi Eric, (In reply to comment #5) > (From update of attachment 11636 [details] [edit]) > Ok, I have two concerns. > > 1. We're mallocing when not necessary: > + SVGMatrix* ctm = SVGSVGElement::createSVGMatrix(); > + ctm->multiply(startctm); > > In the error condition, no malloc is needed. Ideally we would just move off of > SVGMatrix internally (this function could take/return AffineTransforms, and let > something outside of it do the wrapping). I'm not sure it's worth going > through contortions to avoid mallocs in error conditions however. Consider > making this an AffineTransform in/out function though. I agree in theory, this is AFAIK a gray area since I don't think it is specified anywhere what to return in error conditions, ie. null or some "real" matrix. Also it could wait until we adopt AffineTransform more for internal use, where we use SVGMatrix now. > 2. We need tests for the error conditions. I see this behavior: > + if (ec) > + return ctm; > > Does that match the spec? Does that match firefox/opera? It would be good to > test. I added it in the test (now othermaciej pointed out how to do it). Will attach a new patch shortly. > Otherwise it looks fine. Great. Cheers, Rob. Created attachment 11640 [details]
Improved testcase
Now we test that getTransformToElement throws the right exception.
Cheers,
Rob.
Created attachment 11650 [details]
Now uses virtual base class
This patch introduces virtual base class SVGLocatable. This helps since we need only one implementation of getTransformToElement instead of one for SVGTextElement, one for SVGStyledTransformable etc. However it probably needs to be (performance) tested well.
Cheers,
Rob.
Comment on attachment 11650 [details]
Now uses virtual base class
Looks reasonable
Landed by Rob in r17991. |