Restore "toJSON" to list of reserved webidl keywords

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jib, Assigned: almasry.mina)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Once 863402 is fixed, remove attachment 749583 [details] [diff] [review] workaround.
(Assignee)

Updated

5 years ago
Assignee: nobody → almasry.mina
(Assignee)

Comment 1

5 years ago
Created attachment 781536 [details] [diff] [review]
resever-toJSON.patch

Here is a patch that removes the toJSON implementation and relies on the jsonifier just added. Test included.
Attachment #781536 - Flags: review?(bzbarsky)
Comment on attachment 781536 [details] [diff] [review]
resever-toJSON.patch

Lovely.  r=me
Attachment #781536 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 4

5 years ago
Maybe I shouldn't mark this as checkin-needed until the jsonifier patch lands...
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 5

5 years ago
Created attachment 782328 [details] [diff] [review]
Patch, removed TODO comment

I removed the TODO comment that had to do with this bug.
Attachment #781536 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ca0c8e910426
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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?
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.
(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.
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.
Created attachment 821093 [details] [diff] [review]
Allow toJSON in WebIDL interfaces again.
Attachment #821093 - Attachment description: aeiou → Allow toJSON in WebIDL interfaces again.
Attachment #821093 - Attachment is patch: true
Attachment #821093 - Flags: review?(continuation)
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 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.
Created attachment 821265 [details] [diff] [review]
Allow toJSON identifier to be used again
Attachment #821093 - Attachment is obsolete: true
Attachment #821093 - Flags: review?(peterv)
Attachment #821265 - Flags: review?(peterv)
Blocks: 929492
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+
You need to log in before you can comment on or make changes to this bug.