Closed
Bug 1122552
Opened 9 years ago
Closed 9 years ago
Introduce [[GetOwnProperty]] object op
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
16.54 KB,
patch
|
jorendorff
:
review+
nmatsakis
:
feedback+
|
Details | Diff | Splinter Review |
5.51 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
With this op at hand we can at least easily replace the getGenericAttributes hook. The bigger goal however is for this to help getting rid of the lookup* ops.
Attachment #8550306 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•9 years ago
|
||
Trivial, because none of these implement the getGenericAttributes or lookup* ops, so we don't change any behavior by falling back to the native behavior.
Attachment #8550307 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8550306 [details] [diff] [review] introduce-getownpropertydescriptor Niko, can you look if the hook for typedobjects implements the right semantics?
Attachment #8550306 -
Flags: feedback?(nmatsakis)
Comment 4•9 years ago
|
||
Comment on attachment 8550307 [details] [diff] [review] introduce-getownpropertydescriptor-browser Mmm, looking forward to proxy ops and this stuff aligning more...
Attachment #8550307 -
Flags: review?(bzbarsky) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8550306 [details] [diff] [review] introduce-getownpropertydescriptor Review of attachment 8550306 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This is good stuff. ::: js/src/builtin/TypedObject.cpp @@ +1994,5 @@ > +TypedObject::obj_getOwnPropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id, > + MutableHandle<JSPropertyDescriptor> desc) > +{ > + MOZ_ASSERT(obj->is<TypedObject>()); > + Rooted<TypedObject *> typedObj(cx, &obj->as<TypedObject>()); Delete the assertion: JSObject::as<T> contains an equivalent assertion. @@ +2012,5 @@ > + { > + uint32_t index; > + if (js_IdIsIndex(id, &index)) { > + if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value())) > + return false; I don't see a bounds check here (or in the original obj_getGenericAttributes code). There should be one, right?
Attachment #8550306 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8550306 [details] [diff] [review] introduce-getownpropertydescriptor Review of attachment 8550306 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TypedObject.cpp @@ +2012,5 @@ > + { > + uint32_t index; > + if (js_IdIsIndex(id, &index)) { > + if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value())) > + return false; I hoped Niko could comment on this, but I guess the idea is that TypedObjects claim ownership of all indexes. I actually asked for a spec that clarifies this, but there seems to be none.
Comment 7•9 years ago
|
||
Comment on attachment 8550306 [details] [diff] [review] introduce-getownpropertydescriptor Review of attachment 8550306 [details] [diff] [review]: ----------------------------------------------------------------- Looks right to me ::: js/src/builtin/TypedObject.cpp @@ +2012,5 @@ > + { > + uint32_t index; > + if (js_IdIsIndex(id, &index)) { > + if (!obj_getArrayElement(cx, typedObj, descr, index, desc.value())) > + return false; I think it's best to consider these kinds of details a bit up in the air at the moment, but the intention was that typed objects (like typed arrays, iirc) claimed ownership of integer properties, yes.
Attachment #8550306 -
Flags: feedback?(nmatsakis) → feedback+
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3aaa3b3a42
https://hg.mozilla.org/mozilla-central/rev/ce3aaa3b3a42
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8550306 [details] [diff] [review] introduce-getownpropertydescriptor Review of attachment 8550306 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ScopeObject.cpp @@ +566,5 @@ > +with_GetOwnPropertyDescriptor(JSContext *cx, HandleObject obj, HandleId id, > + MutableHandle<JSPropertyDescriptor> desc) > +{ > + RootedObject actual(cx, &obj->as<DynamicWithObject>().object()); > + return GetOwnPropertyDescriptor(cx, obj, id, desc); Ups, this should be "actual" instead of "obj"
You need to log in
before you can comment on or make changes to this bug.
Description
•