897185 - toJSON should only deal with attributes whose types are serializable

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

The spec for this is at http://dev.w3.org/2006/webapi/WebIDL/#dfn-serializable-type

Basically, it's all primitives, DOMString, dates, enums, interface types that have "jsonifier", and various containers containing serializable types.

Comment 2

[Mina Almasry]

Is this as simple as checking the type of m is serializable here: 

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

And skipping it if it's not?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yep, exactly.

Comment 4

[Mina Almasry]

Attachment: WIP patch

Here's my shot at it so far. I went through all the types and some objects in WebIDL.py and added isSerializable() per spec. I'm a little confused because webidls here have no |serializer;| syntax, so I wasn't sure what to do about IDLInterface.isSerializable()

And for writing a test for this, I was wondering. How do I make an instance of TestCodeGen.webidl in a mochitest? Is TestCodeGen a property on window like Performance.webidl...? I would check myself but mach isn't working for me for some reason...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 782220 [details] [diff] [review]
WIP patch

> so I wasn't sure what to do about IDLInterface.isSerializable()

What you did: make it return true if and only if there's a jsonifier.

> How do I make an instance of TestCodeGen.webidl in a mochitest? 

You don't, unfortunately.  The codegen tests are just "does it compile?" tests.  Behavior tests need an actual DOM object that uses the IDL feature.  :(

However you can examine the codegen to see whether it's doing the right things.

As far as the patch goes, I think I would prefer us to not have an isSerializable on attributes or other non-type objects, just on types.  The spec just talks about "serializable types".

>+        self.hasJsonifier = False
>+        for m in self.members:
>+            if m.isMethod() and m.isJsonifier():
>+                self.hasJsonifier = True
>+                break

  self.hasJsonifier = any(m.isMethod() and m.isJsonifier() for m in self.members)

>@@ -1044,16 +1056,22 @@ class IDLDictionary(IDLObjectWithScope):
>+    def isSerializable(self):
>+        for m in members:
>+            if not m.isSerializable(self):
>+                return False
>+        return True

  def isSerializable(self):
    return all(m.type.isSerializable() for m in self.members)

I suspect the code you had here would throw a runtime exception... it's presumably untested?

>@@ -1188,16 +1206,19 @@ class IDLEnum(IDLObjectWithIdentifier):

I don't think this needs an isSerializable().

>@@ -1597,16 +1627,22 @@ class IDLUnionType(IDLType):

Again, use all() here.

>@@ -1927,16 +1963,19 @@ class IDLWrapperType(IDLType):

I would prefer this actually examined what our inner thing is instead of adding an isSerializable() on IDLEnum and IDLExternalInterface and IDLInterface and IDLDictionary.

Also, I would prefer we explicitly do this as isPrimitive() or isDOMString() or isDate().

r- because I'd like to see the updated thing, but looking good!

Comment 6

[Mina Almasry]

Attachment: Patch with changes

My bad. I thought I had a building patch but I must've forgot to qref or something.

This is a patch that builds. I've made all the changes. Also, I removed isSerializable() from IDLAttribute, to more closely match the spec, maybe? Now the test in the jsonifier method is |m.isAttr() and not m.isStatic() and m.type.isSerializable()|

I added 3 attributes the jsonifier should skip to all the TestGens, and TestCodeGenBinding.cpp skips it correctly. I'm attaching that too.

Comment 7

[Mina Almasry]

Attachment: TestCodeGenBinding.cpp

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 782291 [details] [diff] [review]
Patch with changes

>@@ -1927,16 +1939,29 @@ class IDLWrapperType(IDLType):
>+        if isinstance(self.inner, IDLInterface):

I'd prefer:

  if self.isInterface():
    if self.inner.isExternal():
      return False
    return any(m.isMethod() and m.isJsonifier() for m in self.inner.members)

>+        elif isinstance(self.inner, IDLEnum):

  elif self.isEnum():

>+        elif isinstance(self.inner, IDLDictionary):

  elif self.isDictionary()

>+  JS::Value SetJsonifierShouldSkipThis(JSContext*, JS::Rooted<JS::Value>&);
>+  TestParentInterface* SetJsonifierShouldSkipThis2(TestParentInterface&);
>+  TestCallbackInterface* SetJsonifierShouldSkipThis3(TestCallbackInterface&);

The return values here should be void, like for other setters.

Watch out for TestParentInterface not being includable in the example/jsimpl tests: make sure to do a try push!

r=me

Comment 9

[Mina Almasry]

Attachment: serialize-toJSON.patch

try push: https://tbpl.mozilla.org/?tree=Try&rev=926981133f81

Comment 10

[Mina Almasry]

Try looks okay, as far as I can tell. Marking checkin-needed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6039007537d6

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6039007537d6