Finishing JS_AlreadyHasOwnProperty

RESOLVED FIXED in mozilla1.9beta3

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks: 1 bug)

Other Branch
mozilla1.9beta3
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
The new JS_AlreadyHasOwnProperty API (bug 400793) isn't finished: it isn't documented.  I'm working on that, and I have a few questions.

* Why not call it JS_HasOwnProperty?  (I think we should.  We can still rename it; it hasn't been in a release yet.)

* The implementation is quite different from the implementation of obj_hasOwnProperty (in jsobj.c).  Can some code be shared here?

* Object.prototype.hasOwnProperty() handles inner/outer objects specially.  When searching for a property on an object x, if the property is found on x's outer object, the JSAPI says false; the JS method says true.  Is that intentional?

* Object.prototype.hasOwnProperty() also handles JSPROP_SHARED, JSPROP_PERMANENT properties specially.   JS_AlreadyHasOwnProperty doesn't, so it exposes an implementation detail about Function.length/arity.  Is that intentional?
(Assignee)

Comment 1

10 years ago
* It doesn't CHECK_REQUEST(cx).  Should it?
(In reply to comment #0)
> * Why not call it JS_HasOwnProperty?  (I think we should.  We can still rename
> it; it hasn't been in a release yet.)

Because ECMA specifies hasOwnProperty on Object.prototype, and that does something different from what the underpinning of this API does. We may want to expose that standard method as JS_HasOwnProperty. This API does not resolve lazily-created properties -- hence the "Already".

> * The implementation is quite different from the implementation of
> obj_hasOwnProperty (in jsobj.c).  Can some code be shared here?

No, see above (and see the other bug, I thought we discussed all this there, but it might have been more on IRC; IRC is a problem :-/).

> * Object.prototype.hasOwnProperty() handles inner/outer objects specially. 
> When searching for a property on an object x, if the property is found on x's
> outer object, the JSAPI says false; the JS method says true.  Is that
> intentional?

I think so. Again, this is *not* ECMA hasOwnProperty, reflected as a JS API.

> * Object.prototype.hasOwnProperty() also handles JSPROP_SHARED,
> JSPROP_PERMANENT properties specially.   JS_AlreadyHasOwnProperty doesn't, so
> it exposes an implementation detail about Function.length/arity.  Is that
> intentional?

Yes, see above.

I'm pretty sure you are barking up the wrong tree here, except for the doc writing goodness -- for which, thanks! :-).

/be
Cc'ing bz, since he is sole user of this new API. Jason, you are right we can change this still, but since it can't do what Object.prototype.hasOwnProperty, we may end up doc'ing it as is. But don't let me stop you from proposing better API impl and/or name, taking into account the intentionality of the original change.

/be
(Assignee)

Comment 4

10 years ago
Created attachment 297063 [details] [diff] [review]
CHECK_REQUEST(cx) in JS_AlreadyHasOwn{,UC}Property

Thanks, Brendan.

I think this API should CHECK_REQUEST(cx), because it calls JS_LOCK_OBJ.  Patch attached.  I searched mxr for uses and only found <http://mxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLProtoImpl.cpp#258>, which is only called from within auto-requests.

Also please see http://developer.mozilla.org/en/docs/JS_AlreadyHasOwnProperty .
Assignee: general → jorendorff
Status: NEW → ASSIGNED
Attachment #297063 - Flags: review?(brendan)

Updated

10 years ago
Attachment #297063 - Flags: review?(brendan)
Attachment #297063 - Flags: review+
Attachment #297063 - Flags: approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.385; previous revision: 3.384
done

Not sure if this bug is fixed completely, so leaving open.
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Updated

10 years ago
Blocks: 412368
(Assignee)

Comment 6

10 years ago
Thanks, Reed.  It's fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.