760851 - Add toJSON accessors on Performance and related interfaces

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

See the thread beginning at http://lists.w3.org/Archives/Public/public-script-coord/2012AprJun/0162.html.  We implement what IE does and then get it specced.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://lists.w3.org/Archives/Public/public-web-perf/2012Jun/0003.html

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not sure that that tone was necessary, but thanks for raising the issue.

Comment 4

[cobexer]

IE9 and at least Chrome 21 also implement this already, would be nice to have in Firefox too!

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Boris and I discussed this a bit on IRC.  Here's what we think the path forward is:

1. Add a 'stringifier'-like construct to WebIDL for JSON conversion ('jsonifier', perhaps?)  Unlike the stringifier, which can have any name, the JSON hook must be named toJSON.  You can look at dom/bindings/parser/WebIDL.py and dom/bindings/Codegen.py to see how stringifiers work.
2. Write some stuff in the codegen that automatically generates the toJSON function.  The toJSON hook should:
  a. Create a new JSObject
  b. For every attribute with a getter, the getter should be invoked, and the result of invoking that getter should be stored as a property on the JSObject
  c. That JSObject should be returned.

Spidermonkey should automatically handle the 'getter-returns-an-object' case by invoking the toJSON hook if relevant on that property as it iterates through.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

jhk, are you interested in implementing comment 5?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Apparently the spec added a section on serializers.  I need to read that to see if it's sane.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The spec is pretty reasonable.  It's at http://dev.w3.org/2006/webapi/WebIDL/#idl-serializers.

I think we should start with just what's in comment 5 and then we can implement the full serializer syntax if it's ever necessary.

Comment 9

[Jignesh Kakadiya [:jhk]]

> jhk, are you interested in implementing comment 5?

Sure Yeah! :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

This bug needs an owner.  Kyle, Ms2ger, is either of you interested?  If not, any ideas who might be?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This would have been a good intern project if we had an intern :-/

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mina, are you interested in doing this?

Comment 14

[Mina Almasry]

I'm more than happy to help, but...

> This would have been a good intern project if we had an intern :-/

I hope me saying this isn't inappropriate or sounds too self-interested: Part of the reason I'm so active around here these days is that I'd like to get an interview for a position/internship at mozilla's Toronto office soon. I'm trying to rack up bugs resolved, and this one sounds like a great deal of work for one "bug", so it's a bit out of my way.

Is anyone able/willing to help me get an interview? I'm a decent programmer, I swear.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

/me sends email to respond to comment 14

Comment 16

[Mina Almasry]

I think I'll work on this anyway. I'll do some digging and come back if/when I have questions. Thanks!

Comment 17

[David Bruant]

It just appeared to me that assuming WebIDL compliance, it might just be roughly (typed in the comment, so not tested):

  PerformanceTiming.prototype.toJSON = function(){
    if(!(this instanceof PerformanceTiming))
      throw new TypeError();

    var keys = Object.keys( PerformanceTiming.prototype ); // own and enumerable
    var copy = keys.reduce({}, (acc, k) => {
      var desc = Object.getOwnPropertyDescriptor(PerformanceTiming.prototype, p);
      var getter = desc.get; // accepting custom enumerable getters, I guess
      if(getter)
        acc[k] = getter.call(this); // correct this thanks to arrox function :-)
      return acc;
    })
 
    return JSON.stringify(copy);
  }

Of course, built-ins have to be the originals, and all usual mess JS puts us into. Maybe there is a need for other WebIDL-related checks.

Is it possible to self-host things in the DOM as it's possible in SpiderMonkey?

Note that the function looks generic enough to be almost applicable to other types. Should it been brought to the WebIDL spec?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Is it possible to self-host things in the DOM as it's possible in SpiderMonkey?

Not yet.  But also, something like this is way simpler to implement in C++ in some ways, as long as you hardcode the list of properties you care about.

>    return JSON.stringify(copy);

Just "return copy", actually.

> Should it been brought to the WebIDL spec?

Except that it's not correct because it doesn't see the original values of the properties, right?

Comment 19

[David Bruant]

(In reply to Boris Zbarsky (:bz) from comment #18)
> >    return JSON.stringify(copy);
> 
> Just "return copy", actually.
So that it gets the pretty printing of the original JSON.stringify call? Smart!


> > Should it been brought to the WebIDL spec?
> 
> Except that it's not correct because it doesn't see the original values of
> the properties, right?
What do you mean by "original values"? "getter.call(this)" should get the instance value for each property, no?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> So that it gets the pretty printing of the original JSON.stringify call? Smart!

Well, also because that's what WebIDL says to do.

> What do you mean by "original values"?

To quote you: "Of course, built-ins have to be the originals".

Or put another way, if the page munges PerformanceTiming.prototype that shouldn't affect the toJSON behavior.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Furthermore, the actual WebIDL definition of serializers doesn't match what you have described above.  For example, if the WebIDL has "readonly attribute Foo attr;" where Foo is an interface without a serializer, then your code will produce "copy: {}" in the serialization whereas the WebIDL algorithm for "serializer = { attribute };" as currently specified would leave the copy property out entirely because its value is not a serializable type.

Comment 22

[Mina Almasry]

Attachment: Patch adds jsonifier construct and toJSON to performance.timing

Okay so I've made a lot of progress, and I think I might be just maybe close to done. I'm posting the patch to confirm this is the behaviour you want and ask about tests. So far:

- jsonifier has been added, and it acts like a stringifier, except its return type is object and you can't choose a name for it. It's strange to me that toJSON returns an object. I'd think it'd return a DOMString... are you sure this is what you want?

- The codegen generates ToJSON. It creates a JSObject, calls all the getters on the underlying C++ object, wraps the returns as JS::Values, stores them on the JSObject created, and returns that object. If any of the getters fail, it returns early.

- I've added a jsonifier to performace.timing, and a mochitest which it passes.

What other tests do you want with this? What else is to do?

Comment 23

[Mina Almasry]

Attachment: TestCodeGenBinding.cpp with the new ToJSON method

In this file you can see the ToJSON method as in TestCodeGen.webidl

Comment 24

[Mina Almasry]

Attachment: Patch adds jsonifier construct and toJSON to performance.timing. With jsonifiers added to TestCodeGen.webidl, TestExampleGen, and TestJSImpGen, and mochitest for PerformanceTiming

Okay I've added a jsonifier to TestJSImplGen.webidl, fixed some foreseen nits, and updated comments. These are all the tests I can think of for this patch, for now, so I'm removing the info requested and asking for a review instead.

Comment 25

[Mina Almasry]

Attachment: TestCodeGenBinding.cpp with the new ToJSON method

Comment 26

[Mina Almasry]

Attachment: TestExampleGenBinding.cpp

Comment 27

[Mina Almasry]

Attachment: TestJSImplGenBinding.cpp

Comment 28

[Mina Almasry]

Try server:

https://tbpl.mozilla.org/?tree=Try&rev=b599c1268f62

Comment 29

[Boris Zbarsky [:bzbarsky]]

> are you sure this is what you want?

Yes.  The JS engine will convert it to a string itself, and that saves you the trouble of trying to do it.

> What other tests do you want with this?

None, given comment 24.  ;)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 779045 [details] [diff] [review]
Patch adds jsonifier construct and toJSON to performance.timing. With jsonifiers added to TestCodeGen.webidl, TestExampleGen, and TestJSImpGen, and mochitest for PerformanceTiming

>+++ b/dom/bindings/Codegen.py
>+                toJSONDec = { "name": "toJSON",

"toJSONDesc", please, here and in the two uses below.

Skipping a bunch of changes that I don't think are actually needed....

>+        # Jsonifiers have a different algo than most getters and setters

I think I would prefer it if we just had a CGJSONifier or something and created that instead of CGSpecializedMethod when we're creating our CGThings.  That would also make it easier to ensure that the CGJSONifier comes after all the attributes; see below for why.  CGJSONifier can inherit from CGSpecializedMethod and override definition_body if that would make it simpler.

>+def getJsonifierCall(descriptor, method):
>+    ret = "JSObject* result = JS_NewObject(cx, nullptr, nullptr, obj);\n"
>+    ret += "if(!result) {\n  return false;\n}\n";

This may be more readable as:

      ret = ("JSObject* result = JS_NewObject(cx, nullptr, nullptr, obj);\n"
             "if (!result) {\n"
             "  return false;\n"
             "}\n"

or so.

>+    for m in descriptor.interface.members:
>+        if m.isAttr():
>+            nativeMethodName = CGSpecializedGetter.makeNativeName(descriptor, m)
>+            getterReturnVar = m.identifier.name + "Result"
>+            ret += CGGetterCall(m.type, nativeMethodName, descriptor, m,

Hmm.  So this is duplicating the implementations of all those getters, no?  Why can we not do this instead:

    for m in descriptor.interface.members:
        if m.isAttr():
          ret += ("{ // scope for 'temp'\n"
                  "  JS::Rooted<JS::Value> temp;\n"
                  "  if (!get_%s(cx, obj, self, &temp)) {\n"
                  "    return false;\n"
                  "  }\n"
                  "  // Do the JS_DefineProperty(result) call here" % 
                  m.identifier.name)

or so?  We'd need to add a constructor to JSJitGetterCallArgs that takes a Rooted<Value>*, but that should be pretty simple.  And then we wouldn't have the code duplication and wouldn't need to add more arguments to the CGGetterCall constructor and whatnot.

>+            ret += "if (!(JS_SetProperty(cx, result, \"" + m.identifier.name + \

Like I said above, JS_DefineProperty.  JS_SetProperty has undesirable side-effects if people define properties on Object.prototype.  :(

>@@ -9016,16 +9080,22 @@ class CGBindingImplClass(CGClass):
>+            if name == "Jsonifier":
>+                if op.isIdentifierLess():
>+                    name = "ToJSON"
>+                else:
>+                    # We already added this method
>+                    return

Hmm.  Why is this change needed?  toJSON won't ever call into the C++ ToJSON method, right?  I don't think we need this at all.

>+++ b/dom/bindings/parser/WebIDL.py
>+            if memberType != "stringifiers" and memberType != "legacycallers" \
>+                and memberType != "jsonifiers":

I'd prefer this as:

              if (memberType != "stringifiers" and memberType != "legacycallers" and
                  memberType != "jsonifiers"):

> class IDLAttribute(IDLInterfaceMember):

I don't think we need to support jsonifier attributes; there is no use case.  Attributes shuld just always not be jsonifiers.

>     def p_AttributeWithQualifier(self, p):

So we don't need the changes to this method.

>@@ -3783,16 +3803,21 @@ class Parser(Tokenizer):
>+        jsonifier = IDLInterfaceMember.Special.Jsonifier in p[1]

Likewise, there is no need to support this on p_Operation.  There are no use cases for declaring an operation as a jsonifier, imo.  A bunch of the changes in p_Operation can go away, I think.

>+    def p_Jsonifier(self, p):

Right.  This is the only thing we really want to support.

>+        identifier = IDLUnresolvedIdentifier(BuiltinLocation("<auto-generated-identifier>"),
>+                                             "toJSON",
>+                                             allowDoubleUnderscore=True)

You actually want to pass allowForbidden=True, and not pass anything for allowDoubleUnderscode, since there are no double-underscores here.

>+    def p_QualifierJsonifier(self, p):

Please drop this, since we don't want to allow that, just allowing "jsonifier;".

>+++ b/dom/tests/mochitest/bugs/test_toJSON.html
>+    var perf = JSON.parse(JSON.stringify(window.performance.timing.toJSON()));

  var perf = JSON.parse(JSON.stringify(window.performance.timing));

please.  That's the part we really care about not regressing.

>+++ b/dom/webidl/PerformanceTiming.webidl
>+  jsonifier object ();

  jsonifier;

Please also add this to the Performance and PerformanceNavigation interfaces.

Please consider fixing bug 872377 as a followup.  That involves the following:

1)  Converting mozRTCSessionDescription to using this new setup and removing the manual toJSON implementation we added for that interface.

2)  Restoring toJSON to the forbidden list in the IDLUnresolvedIdentifier constructor (see bug 872377).

And please file a followup about only having toJSON handle attributes whose types are serializable; I would prefer that to be a separate bug because of the complexity it adds to the patch.

r- because of the various comments above, but this is looking great.  I'm just sorry you did all the work to add jsonifier methods/attributes.... :(

Comment 31

[Mina Almasry]

Attachment: ToJSON Patch - changes made.

Here's a patch with all the changes. I think the most objectionable bit are the flags I chose for the JS_DefineProperty call, and I hope I reversed all the changes I didn't have to make. Test has been updated, too.

Comment 32

[Mina Almasry]

>   var perf = JSON.parse(JSON.stringify(window.performance.timing));
> 
> please.  That's the part we really care about not regressing.

I'm still a little confused about which bits the js engine handle, and why for example this would be a test for toJSON at all (it's not called...), but change made. FWIW I tested the mochitest with and without toJSON and it passes either way.

> Please consider fixing bug 872377 as a followup.  That involves the
> following:
> 
> 1)  Converting mozRTCSessionDescription to using this new setup and removing
> the manual toJSON implementation we added for that interface.
> 
> 2)  Restoring toJSON to the forbidden list in the IDLUnresolvedIdentifier
> constructor (see bug 872377).
> 
> And please file a followup about only having toJSON handle attributes whose
> types are serializable; I would prefer that to be a separate bug because of
> the complexity it adds to the patch.

Will do

Comment 33

[Mina Almasry]

Attachment: ToJSON Patch - changes made.

Made one last change I missed.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 779951 [details] [diff] [review]
ToJSON Patch - changes made.

So I was just looking at the codegen for TestJSImplGenBinding, and there are two other issues there:

1)  toJSON appears twice in the sMethods_specs array.
2)  There is a TestJSImplInterfaceJSImpl::ToJSON being generated, and a
    (totally unused) TestJSImplInterface::ToJSON that calls it.

Both of these are because the method is not identifierless.  What we should probably do is add a new predicate on IDLMethod that will return true if identifierless or toJSON and use it here as needed.  Or alternately use the identifier "__toJSON" for our jsonifier, which should make things Just Work, I think.  Except you'll want to use "toJSON" instead of jsonifier.identifier.name in toJSONDesc.

>+++ b/dom/bindings/Codegen.py
>+class CGJsonifierMethod(CGSpecializedMethod):
>+    def definition_body(self):
>+        ret = "JSObject* result = JS_NewObject(cx, nullptr, nullptr, obj);\n"

That should probably actually be:

          ret = "JS::Rooted<JSObject*> result(cx, JS_NewObject(cx, nullptr, nullptr, obj);\n"

>+        ret += "if(!result) {\n  return false;\n}\n";
>+        ret += "ErrorResult rv;\n"

See the "This may be more readable" review comment above?

>+        for m in self.descriptor.interface.members:
>+          if m.isAttr():
>+            if m.isStatic():

If static, I think we should just skip it; I don't see much of a use case for serializing static attributes.... especially since they live on the interface object, not the instance object's proto chain.

>+                    "  if(!JS_DefineProperty(cx, result, \"%s\", temp, nullptr, nullptr, JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT)) {\n"

Just JSPROP_ENUMERATE.

>+        ret += "args.rval().set(JS::ObjectValue(*result));\n" + \

args.rval().setObject(*result) might be better.
 
>+               "if (!MaybeWrapObjectValue(cx, args.rval())) {\n" + \

Not needed.  You created the object on cx, so it's already in the right compartment.

>+++ b/dom/bindings/parser/WebIDL.py
>@@ -3783,16 +3801,21 @@ class Parser(Tokenizer):
>+        jsonifier = IDLInterfaceMember.Special.Jsonifier in p[1]

Can't happen, since this is p_Operation.  Feel free to assert that it can't happen, if desired.

>@@ -3855,17 +3878,23 @@ class Parser(Tokenizer):
>+        if jsonifier:
>+            if len(arguments) != 0:
>+                raise WebIDLError("jsonifier has wrong number of arguments",
>+                                   [self.getLocation(p, 2)])
>+            if not returnType.isObject():
>+                raise WebIDLError("jsonifier must have object return type",
>+                                   [self.getLocation(p, 2)])

And this whole block should go away.

>@@ -3874,51 +3903,69 @@ class Parser(Tokenizer):
>-               not deleter and not legacycaller and not stringifier:
>+               not deleter and not legacycaller and not stringifier and \
>+               not jsonifier:

And this change is not needed.

>-            identifier = IDLUnresolvedIdentifier(location, "__%s%s%s%s%s%s%s" %
...
>+            if jsonifier:

And all this can go away too.

>         method = IDLMethod(self.getLocation(p, 2), identifier, returnType, arguments,
>                            static=static, getter=getter, setter=setter, creator=creator,
>                            deleter=deleter, specialType=specialType,
>-                           legacycaller=legacycaller, stringifier=stringifier)
>+                           legacycaller=legacycaller, stringifier=stringifier,
>+                           jsonifier=jsonifier)

And this change.

>+++ b/dom/tests/mochitest/bugs/test_toJSON.html
>+  var keysToSkip = ["TYPE_NAVIGATE", "TYPE_RELOAD", "TYPE_RESERVED",
>+                    "TYPE_BACK_FORWARD"];

That can go away once we stop jsonifying static attributes.

>+++ b/js/src/jsfriendapi.h
>+    explicit JSJitGetterCallArgs(const JS::MutableHandleValue& rval)

I would prefer this took a "JS::Rooted<JS::Value>*", not a MutableHandleValue, since the latter relies on implicit constructors and whatnot whereas the former is just directly taking the same constructor argument as MutableHandleValue does.

This is getting much closer, but r- because I'd like to see the patch with the above updates.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> I'm still a little confused about which bits the js engine handle, and why for example
> this would be a test for toJSON at all (it's not called...)

See ECMA-262 edition 5 section 15.12.3, the part after "The abstract operation Str(key, holder)..." step 2.b.i.

Basically, per spec if I have something like:

  JSON.stringify({ toJSON: function() { return { "a": 5 } } })

then you get out the string '{ "a": 5 }'.

Comment 36

[Mina Almasry]

Attachment: ToJSON Patch - changes made.

Changes made. Now there are no duplicates in TestJSImpGenBinding.cpp, and I believe I took out all the changes that weren't needed. Posting new TesJSImplGenBinding.cpp...

To stop the duplicates, I had to check method.isJsonifier() in a couple of places and stop the CG* from being generated.

Comment 37

[Mina Almasry]

Attachment: TestJSImplGenBinding.cpp No Duplicates

Comment 38

[Boris Zbarsky [:bzbarsky]]

> I had to check method.isJsonifier() in a couple of places

Why?  Did just using "__toJSON" as the identifier not work?

Comment 39

[Mina Almasry]

> Why?  Did just using "__toJSON" as the identifier not work?

With __toJSON, some of the names that I think we want as toJSON came out __toJSON, even when I changed JSONDesc to use "toJSON" instead of identifier.name in JSONDesc. And some others came out toJSON, so I was getting compiler errors. After wrestling with it for a while, I tried your first suggestion, and that turned out to be a tad easier. I could try again, if you prefer that route.

I should also explain why keysToSkip is still in the test: it's because the static attrs still show up when you enumerate the original object. I chose to enumerate the original and not the JSON copy because this way the test will notice if the JSON copy is missing some keys that you didn't explicitly ask it to skip.

Comment 40

[Mina Almasry]

Attachment: ToJSON Patch - changes made v2

Added another change I missed :/

Comment 41

[Boris Zbarsky [:bzbarsky]]

> With __toJSON, some of the names that I think we want as toJSON came out __toJSON

Which ones?  The only name that should matter is the one we use in JSONDesc.  Mind attaching the codegen for the __toJSON approach?

> I tried your first suggestion

My first suggestion was to add a new method on IDLMethod and use that, instead of adding explicit checks for jsonifier everywhere, for what it's worth.  But I still think the __toJSON approach is likely better.

> it's because the static attrs still show up when you enumerate the original object.

Oh, right, constants go on the proto too.  OK, that works.

Comment 42

[Mina Almasry]

Attachment: ToJSON patch - using __jsonifier identifier

Alright there are no duplicates is TestJSImplGenBinding.cpp, and no extra methods created.

To do that I had to:

- use __jsonifier identifier (I picked that to have some symmetry with stringifier)

- add |and not isIdentifierless()| in CGCallbackFunction

I also removed some more changes I don't think were needed, so the patch is a little cleaner now, I think.

By the way, trying to add a stringifier to TestJSImplGen.webidl just makes Codegen.py crash on |assert self.body is not None|

Comment 43

[Mina Almasry]

Attachment: TestJSImplGenBinding.cpp No Duplicates

Here it is after the changes to the patch.

Comment 44

[Mina Almasry]

Attachment: ToJSON patch - using __jsonifier identifier

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 781180 [details] [diff] [review]
ToJSON patch - using __jsonifier identifier

>+++ b/dom/bindings/Codegen.py
>@@ -1480,17 +1480,27 @@ class MethodDefiner(PropertyDefiner):
>+class CGJsonifierMethod(CGSpecializedMethod):
>+        ret = ("JS::Rooted<JSObject*> result(cx, JS_NewObject(cx, nullptr, nullptr, obj));\n"

Actually, just realized: the fourth arg to JS_NewObject here should be nullptr as well.

>+               "if(!result) {\n"

Space after "if"?

>+               "ErrorResult rv;\n")

This is unused, afaict.  Take it out, please.

>+              getterCall = ("get_%s(cx, obj, self, JSJitGetterCallArgs(&temp))" %
>+                            m.identifier.name)

Is there a reason this is a separate variable?  Seems like you could just write this in the string below, with the %s for the name substituted into that main string?

>+                      "  if(!JS_DefineProperty(cx, result, \"%s\", temp, nullptr, nullptr, 

Space after "if", please.

Also, if you use single quotes to delimit the Python string, you won't need to escape the double quotes inside it, which is a bit more readable.

>+++ b/dom/bindings/parser/WebIDL.py
>+        identifier = IDLUnresolvedIdentifier(BuiltinLocation("<auto-generated-identifier>"),
>+                                             "__jsonifier", allowForbidden=True,

You no longer need the allowForbidden=True.

>+++ b/dom/tests/mochitest/bugs/test_toJSON.html
>+  // We need to skip all the static attributes.

Those are interface constants, not static attributes.

r=me with the above nits fixed.  Thank you!

Comment 46

[Mina Almasry]

Attachment: Reviewed patch

My pleasure :-) Try push: https://tbpl.mozilla.org/?tree=Try&rev=fc5bd774eaff

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e26410b337b5

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e26410b337b5