Closed
Bug 897185
Opened 11 years ago
Closed 11 years ago
toJSON should only deal with attributes whose types are serializable
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: almasry.mina, Assigned: almasry.mina)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•11 years ago
|
||
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.
Blocks: ParisBindings
Summary: toJSON should handle attributes whose types are serializable → toJSON should only deal with attributes whose types are serializable
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Yep, exactly.
Assignee | ||
Comment 4•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → almasry.mina
Comment 5•11 years ago
|
||
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-
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
Updated•11 years ago
|
Attachment #782292 -
Attachment mime type: text/x-c++src → text/plain
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
try push: https://tbpl.mozilla.org/?tree=Try&rev=926981133f81
Attachment #782291 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Try looks okay, as far as I can tell. Marking checkin-needed.
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6039007537d6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla25
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•