Closed
Bug 1079188
Opened 10 years ago
Closed 10 years ago
Coerce the argument passed to Object.getOwnPropertyDescriptor using ToObject
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, 2 obsolete files)
2.59 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
js> Object.getOwnPropertyDescriptor("foo", 0) TypeError: "foo" is not an object // should return {configurable:false, enumerable:true, value:"f", writable:false}
Attachment #8500959 -
Flags: review?(till)
Attachment #8500959 -
Attachment is obsolete: true
Attachment #8500959 -
Flags: review?(till)
Attachment #8500965 -
Flags: review?(till)
Comment 3•10 years ago
|
||
Comment on attachment 8500965 [details] [diff] [review] bug-1079188-v2.patch Review of attachment 8500965 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, mostly looks good, but cancelling review because of the `enumerable: false` thing mentioned below. ::: js/src/tests/ecma_6/Object/getOwnPropertyDescriptor.js @@ +18,5 @@ > +} > + > +assertDeepEq(Object.getOwnPropertyDescriptor("foo", "length"), { > + configurable: false, > + enumerable: false, Hmm, in comment 0 you said that this should be true - and that makes sense to me. It's also true for `new String('foo')` as the object. Not sure what's going on here, but that has to be fixed before we can land this.
Attachment #8500965 -
Flags: review?(till)
(In reply to Till Schneidereit [:till] from comment #3) > Comment on attachment 8500965 [details] [diff] [review] > bug-1079188-v2.patch > > Review of attachment 8500965 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, mostly looks good, but cancelling review because of the `enumerable: > false` thing mentioned below. > > ::: js/src/tests/ecma_6/Object/getOwnPropertyDescriptor.js > @@ +18,5 @@ > > +} > > + > > +assertDeepEq(Object.getOwnPropertyDescriptor("foo", "length"), { > > + configurable: false, > > + enumerable: false, > > Hmm, in comment 0 you said that this should be true - and that makes sense > to me. It's also true for `new String('foo')` as the object. Not sure what's > going on here, but that has to be fixed before we can land this. Till, in comment 0, I tested the "0" property, in this place, I tested the "length" property. I'm sorry for confusing you.
Comment 5•10 years ago
|
||
Comment on attachment 8500965 [details] [diff] [review] bug-1079188-v2.patch Review of attachment 8500965 [details] [diff] [review]: ----------------------------------------------------------------- Oh! No reason to apologize at all. In fact, I apologize for my sloppy reading. In this case, r=me with a test for `0` added :)
Attachment #8500965 -
Flags: review+
Attachment #8500965 -
Attachment is obsolete: true
Attachment #8501167 -
Flags: review?(till)
Comment 7•10 years ago
|
||
Comment on attachment 8501167 [details] [diff] [review] add test for `0` property Review of attachment 8501167 [details] [diff] [review]: ----------------------------------------------------------------- Perfect, thanks!
Attachment #8501167 -
Flags: review?(till) → review+
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d25bc2a722d9
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d25bc2a722d9
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
•