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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[aristid.breitkreuz]

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 1

[aristid.breitkreuz]

Attachment: Patch adding JS_GetPropertyById and JS_SetPropertyById

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Brendan Eich [:brendan]]

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

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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

Looks good.

Comment 5

[aristid.breitkreuz]

Attachment: Patch adding most proposed functions

Adds all proposed *ById functions, except:

* JS_GetPropertyAttributesById
* JS_GetPropertyAttrsGetterAndSetterById
* JS_SetPropertyAttributesById

Comment 6

[aristid.breitkreuz]

Attachment: Alternative patch which actually renames JS_LookupPropertyByIdWithFlags to JS_LookupPropertyWithFlagsById

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[aristid.breitkreuz]

Attachment: Patch addressing brendan's comments

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.

Comment 10

[Brendan Eich [: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

Comment 11

[aristid.breitkreuz]

Attachment: Patch without unneeded CHECK_REQUEST [Checkin: Comment 13]

Remove CHECK_REQUEST in JS_DeleteProperty and JS_DeletePropertyById, as brendan requested in comment #10.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 344601 [details] [diff] [review]
Patch without unneeded CHECK_REQUEST
[Checkin: Comment 13]

r=brendan

Comment 13

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/27081984a11e

Comment 14

[C. Scott Ananian]

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.

Comment 15

[Brendan Eich [:brendan]]

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

[C. Scott Ananian]

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.

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Brendan Eich [:brendan]]

(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

Comment 19

[Brendan Eich [:brendan]]

(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

Comment 20

[Brendan Eich [:brendan]]

(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