Closed
Bug 1076588
Opened 10 years ago
Closed 10 years ago
Object.freeze() should return its argument with no conversion when the argument is a primitive value
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: 446240525, Assigned: 446240525)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(1 file, 3 obsolete files)
4.30 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
js> Object.freeze("foo") TypeError: "foo" is not an object // should return "foo" in ES6
Attachment #8498597 -
Flags: review?(till)
Comment 3•10 years ago
|
||
Comment on attachment 8498597 [details] [diff] [review] bug-1076588.patch Review of attachment 8498597 [details] [diff] [review]: ----------------------------------------------------------------- Most excellent!
Attachment #8498597 -
Flags: review?(till) → review+
I will do another try run when the try tree reopens.
Attachment #8498597 -
Attachment is obsolete: true
Attachment #8499286 -
Flags: review+
Comment 5•10 years ago
|
||
Comment on attachment 8499286 [details] [diff] [review] fix a xpcshell test failure Review of attachment 8499286 [details] [diff] [review]: ----------------------------------------------------------------- I'm worried about the test change and what it might mean for the behavior that's being tested. Can you explain what's going on here? ::: toolkit/components/crashmonitor/test/unit/test_invalid_file.js @@ +7,5 @@ > * Test with sessionCheckpoints.json containing invalid data > */ > add_task(function test_invalid_file() { > // Write bogus data to checkpoint file > + let data = '"1234\x00"'; Wait, does this mean that the file is considered valid with the Object.freeze change, but without this one? If so, I don't think that's ok, and we should change things inside of OS.File.writeAtomic (I guess) so that its behavior doesn't change.
Attachment #8499286 -
Flags: review+
Till is right, the test should not be changed.
Attachment #8499286 -
Attachment is obsolete: true
Attachment #8499303 -
Flags: review?(till)
Comment 8•10 years ago
|
||
Comment on attachment 8499303 [details] [diff] [review] manually throw if the argument passed to Object.freeze here isn't an object Review of attachment 8499303 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, that's better.
Attachment #8499303 -
Flags: review?(till) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8499303 [details] [diff] [review] manually throw if the argument passed to Object.freeze here isn't an object Oh, but now we actually need a review from a peer of the CrashMonitor. Based on hg blame, Yoric is one.
Attachment #8499303 -
Flags: review?(dteller)
Comment on attachment 8499303 [details] [diff] [review] manually throw if the argument passed to Object.freeze here isn't an object Review of attachment 8499303 [details] [diff] [review]: ----------------------------------------------------------------- This is indeed an error case. However, I suspect that throwing an error here might prevent Firefox from starting, so I'd rather you called `Cu.reportError` and returned `null`, as when parsing fails.
Attachment #8499303 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8499303 -
Attachment is obsolete: true
Attachment #8499450 -
Flags: review?(dteller)
Comment on attachment 8499450 [details] [diff] [review] use Cu.reportError Review of attachment 8499450 [details] [diff] [review]: ----------------------------------------------------------------- Looks good (I only reviewed CrashMonitor and test). ::: toolkit/components/crashmonitor/test/unit/test_invalid_file.js @@ +13,5 @@ > {tmpPath: sessionCheckpointsPath + ".tmp"}); > > + // An invalid file will cause |init| to return null > + let status = yield CrashMonitor.init(); > + do_check_true(status === null ? true : false); Ah, good catch.
Attachment #8499450 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 13•10 years ago
|
||
All green try run: https://tbpl.mozilla.org/?tree=Try&rev=65715eb8aee1
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b532d05bccf
Flags: in-testsuite+
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1b532d05bccf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•