Last Comment Bug 654685 - Remove Boolean.prototype.toJSON
: Remove Boolean.prototype.toJSON
Status: RESOLVED FIXED
[fixed-in-tracemonkey]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: AWAY Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks: es5
  Show dependency treegraph
 
Reported: 2011-05-04 04:29 PDT by Jan de Mooij [:jandem]
Modified: 2011-05-10 15:14 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove boolean toJSON (345 bytes, patch)
2011-05-04 08:10 PDT, AWAY Tom Schuster [:evilpie]
paul.biggar: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-05-04 04:29:40 PDT
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"); 
});
--
Comment 1 AWAY Tom Schuster [:evilpie] 2011-05-04 08:10:21 PDT
Created attachment 530015 [details] [diff] [review]
remove boolean toJSON

Not sure what to test here?
Comment 2 Jason Orendorff [:jorendorff] 2011-05-04 09:00:51 PDT
Tom, there's a test case in comment 0.  :)
Comment 3 Paul Biggar 2011-05-04 11:07:37 PDT
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 Paul Biggar 2011-05-05 03:38:49 PDT
(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 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-05 14:25:49 PDT
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");
Comment 6 Jason Orendorff [:jorendorff] 2011-05-06 09:36:07 PDT
Jeff: There's also the test case in comment 0.
Comment 7 AWAY Tom Schuster [:evilpie] 2011-05-06 10:15:47 PDT
Jeff: don't worry, i didn't notice it either :P
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-05-06 10:40:39 PDT
Ha, right.  That's what I get for going from memory and not looking at spec steps.
Comment 9 Paul Biggar 2011-05-07 03:25:52 PDT
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.
Comment 10 AWAY Tom Schuster [:evilpie] 2011-05-07 13:40:37 PDT
Added the tests.
https://hg.mozilla.org/tracemonkey/rev/22acbcf01a8f
Comment 11 AWAY Tom Schuster [:evilpie] 2011-05-09 08:54:15 PDT
Fixed warnings on tinderbox
https://hg.mozilla.org/tracemonkey/rev/ec82b6f20c8f
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-05-10 15:14:10 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/22acbcf01a8f
http://hg.mozilla.org/mozilla-central/rev/ec82b6f20c8f

Note You need to log in before you can comment on or make changes to this bug.