Bug 11667 – SVG: method .getTransformToElement(elt) in SVGLocatable not implemented

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

[Andreas Neumann]

This method seems to be missing and returns a Null value.

Expected values are indicated in the example.

[Andreas Neumann]

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.

[Rob Buis]

Created attachment 11630 [details]
First attempt

I worked Andreas' testcase into one suitable for webkit tests.
Cheers,

Rob.

[Rob Buis]

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.

[Rob Buis]

Created attachment 11636 [details]
Improved patch

This patch fixes a memleak involving getCTM.
Cheers,

Rob.

[Eric Seidel (no email)]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 11640 [details]
Improved testcase

Now we test that getTransformToElement throws the right exception.
Cheers,

Rob.

[Rob Buis]

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.

[Oliver Hunt]

Comment on attachment 11650 [details]
Now uses virtual base class

Looks reasonable

[Mark Rowe (bdash)]

Landed by Rob in r17991.