Closed
Bug 872377
Opened 12 years ago
Closed 11 years ago
Restore "toJSON" to list of reserved webidl keywords
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: jib, Assigned: almasry.mina)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
4.71 KB,
patch
|
Details | Diff | Splinter Review | |
1.10 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
Once 863402 is fixed, remove attachment 749583 [details] [diff] [review] workaround.
Updated•12 years ago
|
Blocks: ParisBindings
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → almasry.mina
Assignee | ||
Comment 1•11 years ago
|
||
Here is a patch that removes the toJSON implementation and relies on the jsonifier just added. Test included.
Attachment #781536 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Comment on attachment 781536 [details] [diff] [review]
resever-toJSON.patch
Lovely. r=me
Attachment #781536 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 4•11 years ago
|
||
Maybe I shouldn't mark this as checkin-needed until the jsonifier patch lands...
Keywords: checkin-needed
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 5•11 years ago
|
||
I removed the TODO comment that had to do with this bug.
Attachment #781536 -
Attachment is obsolete: true
Comment 6•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 7•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 8•11 years ago
|
||
How does |jsonifier| replace toJSON? jsonifier enumerates all non-static and serializable attributes in the interface. toJSON lets me do whatever I want (in my case, use my own representation for |object| attributes that I know are always arrays). Can I control the behavior of jsonifier somehow?
Reporter | ||
Comment 9•11 years ago
|
||
It doesn't I'm sure. But it replaces the workaround I had for not having serializer support (Bug 863402) in PeerConnection.js in comment 0.
Comment 10•11 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> It doesn't I'm sure. But it replaces the workaround I had for not having
> serializer support (Bug 863402) in PeerConnection.js in comment 0.
Okay. In that case, can I back this out? Contacts needs the same workaround.
Reporter | ||
Comment 11•11 years ago
|
||
Covered on IRC, reuben will "revert" only the change to WebIDL.py, since he still needs a custom toJSON() method to serialize his array stuff, which webidl doesn't handle yet.
Comment 12•11 years ago
|
||
Updated•11 years ago
|
Attachment #821093 -
Attachment description: aeiou → Allow toJSON in WebIDL interfaces again.
Attachment #821093 -
Attachment is patch: true
Attachment #821093 -
Flags: review?(continuation)
Comment 13•11 years ago
|
||
Comment on attachment 821093 [details] [diff] [review]
Allow toJSON in WebIDL interfaces again.
A DOM peer should review this, and I don't know anything about this issue in any event. Maybe peterv could look at it.
Attachment #821093 -
Flags: review?(continuation) → review?(peterv)
Comment 14•11 years ago
|
||
Comment on attachment 821093 [details] [diff] [review]
Allow toJSON in WebIDL interfaces again.
Please restore the comment about how per-spec toJSON needs to be in this list.
Comment 15•11 years ago
|
||
Attachment #821093 -
Attachment is obsolete: true
Attachment #821093 -
Flags: review?(peterv)
Attachment #821265 -
Flags: review?(peterv)
Comment 16•11 years ago
|
||
Comment on attachment 821265 [details] [diff] [review]
Allow toJSON identifier to be used again
First of all, open a new bug for this patch. This bug is not about allowing toJSON, and piling on is not the right thing to do. Second, you should probably describe in the comment why we need it ("We sometimes need custom serialization behaviour, so allow toJSON for now.").
Attachment #821265 -
Flags: review?(peterv) → review+
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•