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.

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

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

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.

jhk, are you interested in implementing comment 5?

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

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.

> jhk, are you interested in implementing comment 5? Sure Yeah! :)

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

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

Mina, are you interested in doing this?

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.

/me sends email to respond to comment 14

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

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?

> 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?

(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?

> 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.

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.

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?

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

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.

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

> 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 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.... :(

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.

> 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

Made one last change I missed.

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.

> 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 }'.

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.

> I had to check method.isJsonifier() in a couple of places Why? Did just using "__toJSON" as the identifier not work?

> 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.

Added another change I missed :/

> 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.

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|

Here it is after the changes to the patch.

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!

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