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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: 446240525, Assigned: 446240525)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 2 obsolete files)

js> Object.getOwnPropertyDescriptor("foo", 0)
TypeError: "foo" is not an object     
// should return {configurable:false, enumerable:true, value:"f", writable:false}
Attached patch bug-1079188.patch (obsolete) — Splinter Review
Attachment #8500959 - Flags: review?(till)
Attached patch bug-1079188-v2.patch (obsolete) — Splinter Review
Attachment #8500959 - Attachment is obsolete: true
Attachment #8500959 - Flags: review?(till)
Attachment #8500965 - Flags: review?(till)
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 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 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+
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.