The default bug view has changed. See this FAQ.

Add toJSON accessors on Performance and related interfaces

RESOLVED FIXED in mozilla25

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: khuey, Assigned: Mina Almasry)

Tracking

({dev-doc-needed})

unspecified
mozilla25
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=python][lang=c++])

Attachments

(2 attachments, 13 obsolete attachments)

999.74 KB, text/plain
Details
25.82 KB, patch
Details | Diff | Splinter Review
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.
http://lists.w3.org/Archives/Public/public-web-perf/2012Jun/0003.html
I'm not sure that that tone was necessary, but thanks for raising the issue.
Duplicate of this bug: 769173

Comment 4

5 years ago
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.
Summary: Add toJSON accessors on nsIDOMPerformance and related interfaces → Add toJSON accessors on Performance and related interfaces
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! :)
Duplicate of this bug: 878380
This bug needs an owner.  Kyle, Ms2ger, is either of you interested?  If not, any ideas who might be?
Blocks: 863402
Flags: needinfo?(khuey)
Flags: needinfo?(Ms2ger)
Whiteboard: [mentor=bz][lang=python][lang=c++]
This would have been a good intern project if we had an intern :-/
Flags: needinfo?(khuey)
Mina, are you interested in doing this?
Flags: needinfo?(almasry.mina)
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(almasry.mina)
/me sends email to respond to comment 14
(Assignee)

Comment 16

4 years ago
I think I'll work on this anyway. I'll do some digging and come back if/when I have questions. Thanks!
Assignee: nobody → almasry.mina

Comment 17

4 years ago
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?

Comment 19

4 years ago
(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.
(Assignee)

Comment 22

4 years ago
Created attachment 778893 [details] [diff] [review]
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?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 23

4 years ago
Created attachment 778894 [details]
TestCodeGenBinding.cpp with the new ToJSON method

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

Comment 24

4 years ago
Created 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

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.
Attachment #778893 - Attachment is obsolete: true
Attachment #779045 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

4 years ago
Created attachment 779047 [details]
TestCodeGenBinding.cpp with the new ToJSON method
Attachment #778894 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Created attachment 779048 [details]
TestExampleGenBinding.cpp
(Assignee)

Comment 27

4 years ago
Created attachment 779049 [details]
TestJSImplGenBinding.cpp
(Assignee)

Comment 28

4 years ago
Try server:

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

Updated

4 years ago
Flags: needinfo?(bzbarsky)

Updated

4 years ago
Keywords: dev-doc-needed
> 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.  ;)
Flags: needinfo?(Ms2ger)
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.... :(
Attachment #779045 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 31

4 years ago
Created attachment 779947 [details] [diff] [review]
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.
Attachment #779045 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #779947 - Flags: review?(bzbarsky)
(Assignee)

Comment 32

4 years ago
>   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
(Assignee)

Comment 33

4 years ago
Created attachment 779951 [details] [diff] [review]
ToJSON Patch - changes made.

Made one last change I missed.
Attachment #779947 - Attachment is obsolete: true
Attachment #779947 - Flags: review?(bzbarsky)
Attachment #779951 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Blocks: 897185
No longer blocks: 863402
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.
Attachment #779951 - Flags: review?(bzbarsky) → review-
> 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 }'.
(Assignee)

Comment 36

4 years ago
Created attachment 780176 [details] [diff] [review]
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.
Attachment #779951 - Attachment is obsolete: true
Attachment #780176 - Flags: review?(bzbarsky)
(Assignee)

Comment 37

4 years ago
Created attachment 780180 [details]
TestJSImplGenBinding.cpp No Duplicates
Attachment #779049 - Attachment is obsolete: true
> I had to check method.isJsonifier() in a couple of places

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

Comment 39

4 years ago
> 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.
(Assignee)

Comment 40

4 years ago
Created attachment 780444 [details] [diff] [review]
ToJSON Patch - changes made v2

Added another change I missed :/
Attachment #780176 - Attachment is obsolete: true
Attachment #780176 - Flags: review?(bzbarsky)
Attachment #780444 - Flags: review?(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.
(Assignee)

Comment 42

4 years ago
Created attachment 781175 [details] [diff] [review]
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|
Attachment #779047 - Attachment is obsolete: true
Attachment #779048 - Attachment is obsolete: true
Attachment #780180 - Attachment is obsolete: true
Attachment #780444 - Attachment is obsolete: true
Attachment #780444 - Flags: review?(bzbarsky)
Attachment #781175 - Flags: review?(bzbarsky)
(Assignee)

Comment 43

4 years ago
Created attachment 781177 [details]
TestJSImplGenBinding.cpp No Duplicates

Here it is after the changes to the patch.
(Assignee)

Comment 44

4 years ago
Created attachment 781180 [details] [diff] [review]
ToJSON patch - using __jsonifier identifier
Attachment #781175 - Attachment is obsolete: true
Attachment #781175 - Flags: review?(bzbarsky)
Attachment #781180 - Flags: review?(bzbarsky)
Attachment #781177 - Attachment mime type: text/x-c++src → text/plain
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!
Attachment #781180 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 46

4 years ago
Created attachment 781501 [details] [diff] [review]
Reviewed patch

My pleasure :-) Try push: https://tbpl.mozilla.org/?tree=Try&rev=fc5bd774eaff
Attachment #781180 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26410b337b5
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e26410b337b5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.