Closed
Bug 654685
Opened 13 years ago
Closed 13 years ago
Remove Boolean.prototype.toJSON
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: evilpie)
References
Details
(Whiteboard: [fixed-in-tracemonkey])
Attachments
(1 file)
345 bytes,
patch
|
paul.biggar
:
review+
|
Details | Diff | Splinter Review |
Boolean.prototype.toJSON is not in ES5. Bug 557371 removed {Number, String}.prototype.toJSON. Test case that triggered this was something like this: -- JSON.stringify(new Boolean(false), function(k, v) { assertEq(typeof v, "object"); }); --
Assignee | ||
Updated•13 years ago
|
Assignee: general → evilpies
Assignee | ||
Comment 1•13 years ago
|
||
Not sure what to test here?
Attachment #530015 -
Flags: review?(pbiggar)
Comment 3•13 years ago
|
||
Comment on attachment 530015 [details] [diff] [review] remove boolean toJSON Review of attachment 530015 [details] [diff] [review]: Have you checked that jstests doesn't fail? I'd be surprised if there wasn't something different didn't. I'd add the test from jandem's comment, and a test that toJSON isn't in Boolean.prototype. Looking at the standard, there's not much room to go wrong here (it's either "true" or "false"), but you might take a look at the code paths and make sure there's nothing else lurking like jandem's test.
Comment 4•13 years ago
|
||
(In reply to comment #3) > Have you checked that jstests doesn't fail? I'd be surprised if there wasn't > something different didn't. Hmmm, that's not english. I meant that we should check jstests don't fail (and fix them if they do) - I would have though they would.
Comment 5•13 years ago
|
||
I doubt one fails. JSON.stringify extracts the primitive boolean from Boolean objects without this method, and it's the exact same behavior you'd get with this method, so the only way I can think of that this would be visible is if you defined a custom toJSON method on Object.prototype that would be invoked but for Boolean.prototype.toJSON existing. And no one's going to do that because it'd mess up stringification of basically every object. Here's a test which I think we'd fail currently and which we'd pass without the Boolean.prototype.toJSON method. It's the only test I can think of which the presence of the method would affect, other than one directly testing for the presence of the method. Object.prototype.toJSON = function() { return 2; }; assertEq(JSON.stringify(new Boolean(true)), "2");
Assignee | ||
Comment 7•13 years ago
|
||
Jeff: don't worry, i didn't notice it either :P
Comment 8•13 years ago
|
||
Ha, right. That's what I get for going from memory and not looking at spec steps.
Comment 9•13 years ago
|
||
Comment on attachment 530015 [details] [diff] [review] remove boolean toJSON Review of attachment 530015 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you include the tests from comments 0 and 5.
Attachment #530015 -
Flags: review?(pbiggar) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Added the tests. https://hg.mozilla.org/tracemonkey/rev/22acbcf01a8f
Assignee | ||
Updated•13 years ago
|
Whiteboard: [fixed-in-tracemonkey]
Assignee | ||
Comment 11•13 years ago
|
||
Fixed warnings on tinderbox https://hg.mozilla.org/tracemonkey/rev/ec82b6f20c8f
Comment 12•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/22acbcf01a8f http://hg.mozilla.org/mozilla-central/rev/ec82b6f20c8f
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•