Closed Bug 897185 Opened 6 years ago Closed 6 years ago

toJSON should only deal with attributes whose types are serializable

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: almasry.mina, Assigned: almasry.mina)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Depends on: 760851
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.
Summary: toJSON should handle attributes whose types are serializable → toJSON should only deal with attributes whose types are serializable
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?
Yep, exactly.
Attached patch WIP patch (obsolete) — Splinter Review
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...
Attachment #782220 - Flags: review?(bzbarsky)
Assignee: nobody → almasry.mina
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!
Attachment #782220 - Flags: review?(bzbarsky) → review-
Attached patch Patch with changes (obsolete) — Splinter Review
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.
Attachment #782220 - Attachment is obsolete: true
Attachment #782291 - Flags: review?(bzbarsky)
Attached file TestCodeGenBinding.cpp
Attachment #782292 - Attachment mime type: text/x-c++src → text/plain
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
Attachment #782291 - Flags: review?(bzbarsky) → review+
try push: https://tbpl.mozilla.org/?tree=Try&rev=926981133f81
Attachment #782291 - Attachment is obsolete: true
Try looks okay, as far as I can tell. Marking checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6039007537d6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6039007537d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla25
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.