Closed
Bug 1060873
Opened 10 years ago
Closed 10 years ago
Object.isExtensible() should return false when given primitive values as input
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)
2.40 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
No description provided.
js> Object.isExtensible("foo")
TypeError: "foo" is not an object
// should return false in ES6
(In reply to ziyunfei from comment #0)
>
I pressed the enter key by mistake.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Attachment #8481879 -
Flags: review?(till)
Keywords: dev-doc-needed → dev-doc-complete
Comment 5•10 years ago
|
||
Comment on attachment 8481879 [details] [diff] [review]
bug1060873.patch
Review of attachment 8481879 [details] [diff] [review]:
-----------------------------------------------------------------
Great, the change looks good. It requires a few more test changes, though, as there are existing tests that now fail.
Going forward, it'd be great if you could run the jstest and jit-test suites before asking for a review. See [1] for more info on that.
thanks again, these changes are much appreciated!
[1]: https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey#Testing_your_changes
Attachment #8481879 -
Flags: review?(till)
(In reply to Till Schneidereit [:till] from comment #5)
> Great, the change looks good. It requires a few more test changes, though,
> as there are existing tests that now fail.
Hi till, could you tell me which test(s) failed for you? Cause I did run the test suites and only saw:
REGRESSIONS
js1_5/extensions/toLocaleFormat-01.js
js1_5/extensions/toLocaleFormat-02.js
FAIL
Comment 7•10 years ago
|
||
(In reply to ziyunfei from comment #6)
> Hi till, could you tell me which test(s) failed for you? Cause I did run the
> test suites and only saw:
>
> REGRESSIONS
> js1_5/extensions/toLocaleFormat-01.js
> js1_5/extensions/toLocaleFormat-02.js
> FAIL
Interestingly, one of the test failures I saw is entirely unrelated to your patch, but happens on my machine only. I'm investigating this.
The other was in fact your newly-added test. It uses a function `assertFalse`, which doesn't seem to exist. Can you change the tests to use `assertEq(Object.isExtensible(), false)`?
Also, symbols are disabled for Firefox 33 and might not make Firefox 34, either. Can you put the Symbol-related test into an if block along the lines of `if (typeof Symbol !== "undefined") { [test here] }`?
Attachment #8481879 -
Attachment is obsolete: true
Attachment #8482663 -
Flags: review?(till)
Attachment #8482663 -
Attachment is obsolete: true
Attachment #8482663 -
Flags: review?(till)
Attachment #8482665 -
Flags: review?(till)
Comment 10•10 years ago
|
||
Comment on attachment 8482665 [details] [diff] [review]
bug1060873 v3.patch
Review of attachment 8482665 [details] [diff] [review]:
-----------------------------------------------------------------
Now the test isn't running properly at all anymore, because you introduced various syntax errors. Please make sure the final version you attach successfully runs.
Attachment #8482665 -
Flags: review?(till)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8482665 -
Attachment is obsolete: true
Attachment #8482713 -
Flags: review?(till)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #10)
> Comment on attachment 8482665 [details] [diff] [review]
> bug1060873 v3.patch
>
> Review of attachment 8482665 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Now the test isn't running properly at all anymore, because you introduced
> various syntax errors. Please make sure the final version you attach
> successfully runs.
Sorry, it's my bad. I promise I will be more careful next time.
Thanks for your patience!
Comment 13•10 years ago
|
||
Comment on attachment 8482713 [details] [diff] [review]
bug1060873 v4.patch
Review of attachment 8482713 [details] [diff] [review]:
-----------------------------------------------------------------
Nice, thanks for the patch and bearing with me on the change requests.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e75a5c333c8
Attachment #8482713 -
Flags: review?(till) → review+
Assignee: nobody → 446240525
Status: UNCONFIRMED → 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
•