Public API for OBJ_GET_PROPERTY or the JavaScript [] operator (and similar functions)

RESOLVED FIXED in mozilla1.9.1b2

Status

()

enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: aristid.breitkreuz, Assigned: aristid.breitkreuz)

Tracking

Trunk
mozilla1.9.1b2
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.0.3) Gecko/2008092510 Ubuntu/8.04 (hardy) Firefox/3.0.3
Build Identifier: JavaScript-C 1.7.0 2007-10-03

Spidermonkey has no public property APIs for jsid or jsval. Only char*, jschar* and jsint are supported. (Except for JS_GetMethodById.)

I would like to see the following methods:

* JS_GetPropertyById
* JS_SetPropertyById
* JS_HasPropertyById
* JS_DeletePropertyById, JS_DeletePropertyById2
* JS_AlreadyHasOwnPropertyById
* JS_DefinePropertyById
* JS_GetPropertyAttributesById
* JS_GetPropertyAttrsGetterAndSetterById
* JS_LookupPropertyById
* JS_LookupPropertyWithFlagsById
* JS_SetPropertyAttributesById

All being the same as their similarly named jschar* counterparts, just taking jsid as a parameter.

I will try to attach a patch implementing some of those functions soon.

Reproducible: Always
Comment on attachment 344299 [details] [diff] [review]
Patch adding JS_GetPropertyById and JS_SetPropertyById

I've never needed such APIs but they seem sensible. JS_GetPropertyById would be nice to have in combination with JS_Enumerate, at least.

The patch looks good.
Attachment #344299 - Flags: review?(brendan)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 344299 [details] [diff] [review]
Patch adding JS_GetPropertyById and JS_SetPropertyById

Looks great, keep 'em coming! Asking mrbkap for his views, maybe we should get the rest you propose in this bug.

/be
Attachment #344299 - Flags: review?(mrbkap)
Attachment #344299 - Flags: review?(brendan)
Attachment #344299 - Flags: review+
Comment on attachment 344299 [details] [diff] [review]
Patch adding JS_GetPropertyById and JS_SetPropertyById

Looks good.
Attachment #344299 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

11 years ago
Adds all proposed *ById functions, except:

* JS_GetPropertyAttributesById
* JS_GetPropertyAttrsGetterAndSetterById
* JS_SetPropertyAttributesById
Attachment #344299 - Attachment is obsolete: true
Comment on attachment 344330 [details] [diff] [review]
Alternative patch which actually renames JS_LookupPropertyByIdWithFlags to JS_LookupPropertyWithFlagsById

>diff -r 9dafdb183b2b js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp	Wed Oct 22 16:31:14 2008 +0200
>+++ b/js/src/jsapi.cpp	Wed Oct 22 20:47:03 2008 +0200
>@@ -3112,6 +3112,20 @@ JS_ConstructObjectWithArguments(JSContex
>     return js_ConstructObject(cx, clasp, proto, parent, argc, argv);
> }
> 
>+static JSBool DefinePropertyById(JSContext *cx, JSObject *obj, jsid id,

Function declarator at start of line, please (IOW, newline after storage class, type, and any pointer mode).

>-JS_LookupPropertyByIdWithFlags(JSContext *cx, JSObject *obj, jsid id,
>+JS_LookupPropertyWithFlagsById(JSContext *cx, JSObject *obj, jsid id,

Do not rename existing API -- we haven't shipped this, but we don't know who is already using it, and it ain't broke.

It ain't broke because putting ById after Property is better -- higher-precedence preposition goes after the phrase it modifies.

/be
Oops, I was confusing this API with one that returns flags via an out param.

in-param modifies to lookup algorithm should be reflected in the name with even higher precedence than the by-id vs. by-C-string or by-element-index name-part. So JS_LookupPropertyWithFlagsById is good.

But, is this an unshipped API we can rename? Is it unused in hg.mozilla.org repos (including comm-central)? Check!

/be
(Assignee)

Comment 9

11 years ago
Fixed the code style issue (static JSBool on its own line).

JS_LookupPropertyByIdWithFlags was used in a single non-JSAPI file. And only in the most recent non-released versions. So I patched that file too.
Attachment #344315 - Attachment is obsolete: true
Attachment #344330 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #344339 - Flags: review?(brendan)
Comment on attachment 344339 [details] [diff] [review]
Patch addressing brendan's comments

>+JS_PUBLIC_API(JSBool)
>+JS_DeletePropertyById(JSContext *cx, JSObject *obj, jsid id)
>+{
>+    jsval junk;
>+
>+    CHECK_REQUEST(cx);
>+    return JS_DeletePropertyById2(cx, obj, id, &junk);

No CHECK_REQUEST(cx) here, JS_DeletePropertyById2 does it for you.

r=me with that. Please post an updated patch for checkin and set the checkin-needed keyword. Thanks,

/be
Attachment #344339 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

11 years ago
Remove CHECK_REQUEST in JS_DeleteProperty and JS_DeletePropertyById, as brendan requested in comment #10.
Attachment #344339 - Attachment is obsolete: true
Attachment #344601 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Assignee: general → aristid.breitkreuz
Comment on attachment 344601 [details] [diff] [review]
Patch without unneeded CHECK_REQUEST
[Checkin: Comment 13]

r=brendan
Attachment #344601 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/27081984a11e
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: No public API for OBJ_GET_PROPERTY or the Javascript [] operator (and similar functions) → Public API for OBJ_GET_PROPERTY or the JavaScript [] operator (and similar functions)
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1b2
Version: unspecified → Trunk
Attachment #344601 - Attachment description: Patch without unneeded CHECK_REQUEST → Patch without unneeded CHECK_REQUEST [Checkin: Comment 13]

Updated

11 years ago
Flags: in-testsuite-
Flags: in-litmus-

Comment 14

10 years ago
Can we get a #define in jsversion.h that I could use to determine whether JS_*PropertyById() is supported in my spidermonkey version?  I guess

#ifdef JS_VERSION > 180

will work for now, but it's not nice.
We're not going to add indefinite ad-hoc or per-API feature-macros to jsversion.h -- you should either require an uprev engine or use JS_VERSION (you can derive a macro yourself if you need it).

/be

Comment 16

10 years ago
Hm, I was thinking more of along the lines of

#ifdef JS_API_5

or

#if JS_API >= 5

where the API "version" is bumped whenever any new public function is added to the released API.  The problem with using JS_VERSION is that it's not clear what the "next" JS_VERSION will be: I'm using 180 now, will the next be 181?  What if there's a patch release to 180 for a security fix which doesn't include the new API functions?  Would that be 181?  Separating the API version from the JS_VERSION would help those of us writing code anticipating the next release.
Why can't you use relational operators on JS_VERSION instead?

Adding another number line adds complexity by creating a parallel number line where we don't obviously need one. Creating the parallel number line allows for loss of parallelism, which would be bad. Is API a function of VERSION? Who knows. We try not to break API. Where we add, you'll have to test <= version-added-to.

/be
(In reply to comment #16)
> The problem with using JS_VERSION is that it's not clear
> what the "next" JS_VERSION will be: I'm using 180 now, will the next be 181?

That's a good point -- we're still defining JS_VERSION 10 in jsversion.h. We should pick a number. Thanks for pointing this out, I'll file a bug.
 
> What if there's a patch release to 180 for a security fix which doesn't include
> the new API functions?

We don't do releases like that.

> Would that be 181?  Separating the API version from the
> JS_VERSION would help those of us writing code anticipating the next release.

I mentioned trying not to break API, but we can and should break some bad old APIs soon. I'm not sure numbering the API "versions" in this light helps, but if it does, please file a new bug. Talking about it in a resolved bug does not help get the separate issue resolved.

/be
(In reply to comment #18)
> (In reply to comment #16)
> > The problem with using JS_VERSION is that it's not clear
> > what the "next" JS_VERSION will be: I'm using 180 now, will the next be 181?
> 
> That's a good point -- we're still defining JS_VERSION 10 in jsversion.h. We
> should pick a number. Thanks for pointing this out, I'll file a bug.

I used existing bug 479473 instead.

/be
(In reply to comment #18)
> (In reply to comment #16)
> > The problem with using JS_VERSION is that it's not clear
> > what the "next" JS_VERSION will be: I'm using 180 now, will the next be 181?
> 
> That's a good point -- we're still defining JS_VERSION 10

Obviously meant to type 180 there...

/be
You need to log in before you can comment on or make changes to this bug.