Closed Bug 897185 Opened 7 years ago Closed 7 years ago
JSON should only deal with attributes whose types are serializable
No description provided.
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?
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)
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-
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 #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+
Attachment #782291 - Attachment is obsolete: true
Try looks okay, as far as I can tell. Marking checkin-needed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.