1082663 - DOMMatrix does not implement stringifier from spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Potch [:potch]]

From the spec: http://dev.w3.org/fxtf/geometry/Overview.html#dom-dommatrixreadonly-stringifier

> In this example, a matrix is created and several 2D transformation methods are called:
> 
> var matrix = new DOMMatrix();
> matrix.scaleSelf(2);
> matrix.translateSelf(20,20);
> 
> Calling matrix.toString() returns the DOMString:
> 
> "matrix(2, 0, 0, 2, 20, 20)"

In Firefox this prints "[object DOMMatrix]".

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like this got added after we added the DOMMatrix implementation.

Comment 2

[Rik Cabanier]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Looks like this got added after we added the DOMMatrix implementation.

Can you tell me how I mark this up in the IDL? I'll implement it ASAP.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Stringifiers

Comment 4

[Boris Zbarsky [:bzbarsky]]

Just like it's in the spec:

  stringifier;

Comment 5

[Rik Cabanier]

Attachment: Fix + test for stringification of DOMMatrix

Comment 6

[Rik Cabanier]

Attachment: Fix + test for stringification of DOMMatrix

Comment 7

[Rik Cabanier]

Attachment: Fix + test for stringification of DOMMatrix

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8505260 [details] [diff] [review]
Fix + test for stringification of DOMMatrix

Where does the getStringRepresentation bit come from?  You just want:

  stringifier;

in the IDL and call the method Stringify in the C++.  What you're doing actually adds a getStringRepresentation method to the object, which is very much not desirable in this case.

Also, you don't need to define it on DOMMatrix; just putting it on DOMMatrixReadonly is enough.

r=me with those fixed

Comment 9

[Rik Cabanier]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8505260 [details] [diff] [review]
> Fix + test for stringification of DOMMatrix
> 
> Where does the getStringRepresentation bit come from?  You just want:
> 
>   stringifier;
> 
> in the IDL and call the method Stringify in the C++.  What you're doing
> actually adds a getStringRepresentation method to the object, which is very
> much not desirable in this case.
> 
> Also, you don't need to define it on DOMMatrix; just putting it on
> DOMMatrixReadonly is enough.
> 
> r=me with those fixed

I tried that first.
Adding 'stringifier' to the IDL didn't do anything so I switched to adding the method to DOMMatrixReadOnly. Then I noticed that that wouldn't stringify DOMMatrix, so I added it to the IDL if DOMMatrix. (I'll double check if just adding stringifier works today once my mac has finished reinstalling)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Adding 'stringifier' to the IDL didn't do anything

Should work.

> Then I noticed that that wouldn't stringify DOMMatrix

Whyever not?  It just puts a toString on the proto, and the proto of DOMMatrix chains up to the proto of DOMMatrixReadonly.

Comment 11

[Rik Cabanier]

(In reply to Boris Zbarsky [:bz] from comment #10)
> > Adding 'stringifier' to the IDL didn't do anything
> 
> Should work.

I tried and that did work.

> > Then I noticed that that wouldn't stringify DOMMatrix
> 
> Whyever not?  It just puts a toString on the proto, and the proto of
> DOMMatrix chains up to the proto of DOMMatrixReadonly.

No, putting it only on the DOMMatrixReadonly object didn't work.
See try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=53edc37b85c0

Try build where DOMMatrix also has stringifier:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86069ea6f0fd

Comment 12

[Rik Cabanier]

Attachment: Fix + test for stringification of DOMMatrix

Comment 13

[Boris Zbarsky [:bzbarsky]]

> No, putting it only on the DOMMatrixReadonly object didn't work.

Ah.  This looks like a bug in the bindings codegen to me.  We don't actually set up stringifiers for non-concrete interfaces, which is wrong.

Bug 1083591 filed.

Comment 14

[Rik Cabanier]

(In reply to Boris Zbarsky [:bz] from comment #13)
> > No, putting it only on the DOMMatrixReadonly object didn't work.
> 
> Ah.  This looks like a bug in the bindings codegen to me.  We don't actually
> set up stringifiers for non-concrete interfaces, which is wrong.
> 
> Bug 1083591 filed.

Do I need to wait until that bug is fixed?

Comment 15

[Boris Zbarsky [:bzbarsky]]

It might be a good idea, so we don't add stuff that we then have to immediately remove.

I expect that bug to be fixed within the next day or two (patch is up, just needs review), so I don't think waiting is unreasonable.

Comment 16

[Rik Cabanier]

Attachment: Fix + test for stringification of DOMMatrix

try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4e3f63602120

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5183c9014828

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5183c9014828