Bug 11667 - SVG: method .getTransformToElement(elt) in SVGLocatable not implemented
Summary: SVG: method .getTransformToElement(elt) in SVGLocatable not implemented
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: Nobody
URL: http://www.carto.net/neumann/webkitsv...
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-21 08:31 PST by Andreas Neumann
Modified: 2006-12-03 16:48 PST (History)
1 user (show)

See Also:


Attachments
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. (3.11 KB, image/svg+xml)
2006-11-23 02:40 PST, Andreas Neumann
no flags Details
First attempt (14.76 KB, patch)
2006-11-25 14:53 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Improved patch (14.82 KB, patch)
2006-11-26 05:02 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Improved patch (14.93 KB, patch)
2006-11-26 07:02 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Improved testcase (15.21 KB, patch)
2006-11-27 03:27 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Now uses virtual base class (23.69 KB, patch)
2006-11-28 01:39 PST, Rob Buis
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Neumann 2006-11-21 08:31:40 PST
This method seems to be missing and returns a Null value.

Expected values are indicated in the example.
Comment 1 Andreas Neumann 2006-11-23 02:40:36 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.
Comment 2 Rob Buis 2006-11-25 14:53:20 PST
Created attachment 11630 [details]
First attempt

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

Rob.
Comment 3 Rob Buis 2006-11-26 05:02:35 PST
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.
Comment 4 Rob Buis 2006-11-26 07:02:51 PST
Created attachment 11636 [details]
Improved patch

This patch fixes a memleak involving getCTM.
Cheers,

Rob.
Comment 5 Eric Seidel (no email) 2006-11-26 13:09:28 PST
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.
Comment 6 Rob Buis 2006-11-27 03:17:22 PST
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.
Comment 7 Rob Buis 2006-11-27 03:27:49 PST
Created attachment 11640 [details]
Improved testcase

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

Rob.
Comment 8 Rob Buis 2006-11-28 01:39:45 PST
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 9 Oliver Hunt 2006-12-02 16:56:09 PST
Comment on attachment 11650 [details]
Now uses virtual base class

Looks reasonable
Comment 10 Mark Rowe (bdash) 2006-12-03 16:48:15 PST
Landed by Rob in r17991.