Bug 22650 – Make most of SVGMatrix bindings generated again

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

[Dimitri Glazkov (Google)]

This is essentially a kinder, gentler revision of bug 14155, where we just add one line to code generator, rather than switching to custom bindings.

[Dimitri Glazkov (Google)]

Created attachment 25741 [details]
Un-custom-bind SVGMatrix v1

[Eric Seidel (no email)]

I don't like this being a one-line "hack" without explanation.  It seems from looking at the bugs, that SVGMatrix is effectively read-only/copy-on-write, yet our AffineTransform is not.

Lots of ways we could have solved this, besides making custom bindings methods (which I think was the wrong solution).  One of them would have been to have made an SVGMatrix stack object wrapper class around AffineTransform (different from how we used to have SVGMatrix heap objects) and to have used that as the PODType.  multiply() etc. on that intermediate binding layer would have contained this "copy instead of mutate" logic which Niko put in the bindings with the old patch.

I'm not sure I understand how your solution works?

Why does just not "writing back" to wherever the matrix came from solve things like:

element.matrix = z;
var a = element.matrix;
var b = a.multiply(10);
assert(element.matrix == z);
assert(a == element.matrix);
assert(a != b);

Seems your solution would solve the first assert, but not the second two?  I must be missing something.   I should look at the generated code.

[Eric Seidel (no email)]

The original SVGMatrix docs, btw:
http://www.w3.org/TR/SVG/coords.html#InterfaceSVGMatrix

[Eric Seidel (no email)]

Comment on attachment 25741 [details]
Un-custom-bind SVGMatrix v1

Dimitri has a better patch in the works.  Thanks for your review Tim.

[Dimitri Glazkov (Google)]

I apologize for confusion, I should've mentioned that I discussed this with Eric and have more work to do on the patch :).

[Dimitri Glazkov (Google)]

Created attachment 26067 [details]
SVGMatrix v2 (immutable attribute)

Per our discussion, I attempted implementing SVGMatrix, a wrapper around AffineTransform, but it kind got complicated with ObjC bindings and various places where there's a need for implicit AffineTransform<->SVGMatrix conversion, so instead I went with plan B.

This is a modification of the previous patch, with the exception that instead of hard-coding SVGMatrix-specific behavior into code generator, I added "Immutable" attribute (which is already used on IDL properties) to methods, with the following behavior:

If a method is marked as "Immutable", it will return a new instance as a result rather than the reference to the existing instance (self.)

[Eric Seidel (no email)]

Comment on attachment 26067 [details]
SVGMatrix v2 (immutable attribute)

I think this makes the code more complicated :(  I'm not sure what to tell you.  Immutable doesn't convey the intended meaning to me when reading the .idl file.  You and I talked about adding an SVGMatrix class shim, but I remember you encountered some troubles with that.  The shim still seems like the cleanest solution to me.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 26067 [details] [review])
> I think this makes the code more complicated :(  I'm not sure what to tell you.
>  Immutable doesn't convey the intended meaning to me when reading the .idl
> file.  You and I talked about adding an SVGMatrix class shim, but I remember
> you encountered some troubles with that.  The shim still seems like the
> cleanest solution to me.
> 

I've checked the patch, and I think it's a good idea. Please have a look at http://bugs.webkit.org/show_bug.cgi?id=17736. It explains the idea behind 'Immutable' for properties, and this is a logical extension for methods.

I'd vote for accepting the patch, if it doesn't break any regressions tests (most noticeable: SVG pixel tests :-)

[Eric Seidel (no email)]

Comment on attachment 26067 [details]
SVGMatrix v2 (immutable attribute)

Sigh.  I'm still in love with the shim idea. :)  But this is better than nothing for sure!

[Dimitri Glazkov (Google)]

Committed in http://trac.webkit.org/changeset/40265, http://trac.webkit.org/changeset/40268