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.
Created attachment 25741 [details] Un-custom-bind SVGMatrix v1
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.
The original SVGMatrix docs, btw: http://www.w3.org/TR/SVG/coords.html#InterfaceSVGMatrix
Comment on attachment 25741 [details] Un-custom-bind SVGMatrix v1 Dimitri has a better patch in the works. Thanks for your review Tim.
I apologize for confusion, I should've mentioned that I discussed this with Eric and have more work to do on the patch :).
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.)
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.
(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 :-)
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!
Committed in http://trac.webkit.org/changeset/40265, http://trac.webkit.org/changeset/40268