TM: public JS API for new ES5 Object functions

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

9 years ago
Depends on: 546590
(Assignee)

Comment 1

9 years ago
Created attachment 431756 [details] [diff] [review]
patch
Assignee: general → gal

Comment 2

9 years ago
JS_ES5 prefix?  Yeesh.  js_GetOwnPropertyDescriptorById would be the better name, then have the JS_GetOwnPropertyDescriptor public version in jsapi.cpp just call that.
(Assignee)

Comment 3

9 years ago
Look into jsapi.h before complaining. Some of the names are already taken, and the file is extern "C", so we can't overload.
(In reply to comment #3)
> Look into jsapi.h before complaining. Some of the names are already taken, and
> the file is extern "C", so we can't overload.

Waldo's right, this is wrong. There's no existing conflict. This grep is from my mq pushed on top of tip tm:

$ grep Descriptor *.h|grep JS_
jsapi.h:JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN flags,
jscntxtinlines.h:            JS_CALL_VALUE_TRACER(trc, desc.value, "PropertyDescriptor::value");
jscntxtinlines.h:            JS_CALL_VALUE_TRACER(trc, desc.get, "PropertyDescriptor::get");
jscntxtinlines.h:            JS_CALL_VALUE_TRACER(trc, desc.set, "PropertyDescriptor::set");

Please do not add ES5_ middle-fix to API names.

/be
(Assignee)

Comment 5

9 years ago
You are both wrong. Brendan even quoted it.

extern JS_PUBLIC_API(JSBool)
JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN flags,
                             JSPropertyDescriptor *desc);

The equivalent JS function is Object.prototype.getPropertyDescriptor. We want it to return a jsval. The existing API function returns a JSPropertyDescriptor *. I can't overload it.

We can:

- not mention the ById, but that's inconsistent with existing naming and confusing.
- inject "Object", but that makes the naming out of sync with Object.* (GetPropertyDescriptorObjectById)
- make jsapi.h C++

I am fine with either. The ES5 was a placeholder until we have a better fix. But there is a a conflict.
I wonder if it's worth just changing JS_GetPropertyDescriptorById. It's a new API as of not too long ago.
(In reply to comment #5)
> You are both wrong. Brendan even quoted it.
> 
> extern JS_PUBLIC_API(JSBool)
> JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN
> flags,
>                              JSPropertyDescriptor *desc);
> 
> The equivalent JS function is Object.prototype.getPropertyDescriptor.

So? Either rename the old function as mrbkap suggested, or drop the "ById" noise and give your better new API the plain name.

There's no conflict if you don't insist on JS_ES5_InsanelyLongNamesById :-|.

/be
(Assignee)

Comment 8

9 years ago
Created attachment 432637 [details] [diff] [review]
patch
Attachment #431756 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #432637 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

9 years ago
Blocks: 546590
No longer depends on: 546590
(Assignee)

Comment 9

9 years ago
review ping
(Assignee)

Comment 10

9 years ago
Created attachment 433116 [details] [diff] [review]
patch

refreshed patch
Attachment #432637 - Attachment is obsolete: true
Attachment #433116 - Flags: review?(jwalden+bmo)
Attachment #432637 - Flags: review?(jwalden+bmo)

Updated

9 years ago
Attachment #433116 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 433116 [details] [diff] [review]
patch

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>+JS_PUBLIC_API(JSBool)
>+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsval name, jsval *vp)
>+{

As a diff-reducing measure it's acceptable to define this in jsobj.cpp, but it really should go in jsapi.cpp; likewise for JS_DefineOwnProperty.  (It's helpful to know that a JS_* function is always in jsapi.cpp; the current decimal-to-string exceptions that aren't -- at least for the moment -- are very confusing.)  Followup bug, or (even better) followup checkin immediately after this one?


>     AutoIdRooter nameidr(cx);
>-    if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr()))
>+    if (!JS_ValueToId(cx, name, nameidr.addr()))
>         return JS_FALSE;

|name| should be jsid, not jsval, to save this effort in cases where a name has already been id-ified.



> static JSBool
>+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
>+        return false;
>+    }
>+
>+    JSObject *obj = JSVAL_TO_OBJECT(vp[2]);
>+
>+    jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID;
>+
>+    return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp);
>+}

The empty lines between the last three statements are a bit odd; remove them.


>+JS_PUBLIC_API(JSBool)
>+JS_DefineOwnProperty(JSContext *cx, JSObject *obj, jsval name, jsval descriptor)
>+{
>+    AutoIdRooter tvr(cx);
>+    AutoDescriptorArray descs(cx);
>+    PropertyDescriptor *desc = descs.append();
>+
>+    if (!JS_ValueToId(cx, name, tvr.addr()) ||
>+        !desc ||
>+        !desc->initialize(cx, tvr.id(), descriptor)) {
>+        return false;
>+    }

You should test desc before attempting to convert to id; this could double-report -- but changing to jsid would fix that anyway, because the ->id conversion would be unnecessary.

But a more basic question: is it acceptable for JS_DefineOwnProperty to return a success value but not actually define the property?  Your dummy variable right now means that a success value doesn't actually mean the property was defined.  Is that acceptable?  I'm thinking it isn't, and that we should add a JSBool outparam for whether a property was defined or not.  r- particularly to address this issue.
(Assignee)

Comment 12

9 years ago
> >+JS_PUBLIC_API(JSBool)
> >+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsval name, jsval *vp)
> >+{
> 
> As a diff-reducing measure it's acceptable to define this in jsobj.cpp, but it
> really should go in jsapi.cpp; likewise for JS_DefineOwnProperty.  (It's
> helpful to know that a JS_* function is always in jsapi.cpp; the current
> decimal-to-string exceptions that aren't -- at least for the moment -- are very
> confusing.)  Followup bug, or (even better) followup checkin immediately after
> this one?

IM(H) API entry points should be located where they can inline and access the definitions they need. The new API mirror jsobj operations and should be located there. No need to take inlining/linking overhead just so everything JS_ is in one file. That's what the lord in his infinite wisdom gaves us grep for :)

> 
> 
> >     AutoIdRooter nameidr(cx);
> >-    if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr()))
> >+    if (!JS_ValueToId(cx, name, nameidr.addr()))
> >         return JS_FALSE;
> 
> |name| should be jsid, not jsval, to save this effort in cases where a name has
> already been id-ified.
> 

Yeah, that's cleaner. I wasn't sure we want to export jsid hence my somewhat awkward signature. Fixing that.

> 
> 
> > static JSBool
> >+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp)
> >+{
> >+    if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) {
> >+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
> >+        return false;
> >+    }
> >+
> >+    JSObject *obj = JSVAL_TO_OBJECT(vp[2]);
> >+
> >+    jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID;
> >+
> >+    return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp);
> >+}
> 
> The empty lines between the last three statements are a bit odd; remove them.
> 
> 
> >+JS_PUBLIC_API(JSBool)
> >+JS_DefineOwnProperty(JSContext *cx, JSObject *obj, jsval name, jsval descriptor)
> >+{
> >+    AutoIdRooter tvr(cx);
> >+    AutoDescriptorArray descs(cx);
> >+    PropertyDescriptor *desc = descs.append();
> >+
> >+    if (!JS_ValueToId(cx, name, tvr.addr()) ||
> >+        !desc ||
> >+        !desc->initialize(cx, tvr.id(), descriptor)) {
> >+        return false;
> >+    }
> 
> You should test desc before attempting to convert to id; this could
> double-report -- but changing to jsid would fix that anyway, because the ->id
> conversion would be unnecessary.
> 
> But a more basic question: is it acceptable for JS_DefineOwnProperty to return
> a success value but not actually define the property?  Your dummy variable
> right now means that a success value doesn't actually mean the property was
> defined.  Is that acceptable?  I'm thinking it isn't, and that we should add a
> JSBool outparam for whether a property was defined or not.  r- particularly to
> address this issue.

Fixing these. New patch in a sec.
(Assignee)

Comment 13

9 years ago
Note that I have an id now, but the name is not ById. I like this (we don't want to add const char* args in the future, so we could start dropping the ById). But its inconsistent. Make sure you are happy with the next patch. We will have to live with the api for a while. I am not married to the naming either way.
(Assignee)

Comment 14

9 years ago
just FYI:

host-3-164:src gal$ grep -l "JS_PUBLIC" *.cpp
jsapi.cpp
jsarena.cpp
jscntxt.cpp
jsdbgapi.cpp
jsdhash.cpp
jsgc.cpp
jshash.cpp
jslog2.cpp
jsobj.cpp
jsprf.cpp
jsutil.cpp
jsxdrapi.cpp
(Assignee)

Comment 15

9 years ago
Created attachment 433674 [details] [diff] [review]
patch
Attachment #433116 - Attachment is obsolete: true
Attachment #433674 - Flags: review?(jwalden+bmo)
(In reply to comment #14)
> just FYI:
> 
> host-3-164:src gal$ grep -l "JS_PUBLIC" *.cpp
> jsapi.cpp
> jsarena.cpp
> jscntxt.cpp
> jsdbgapi.cpp
> jsdhash.cpp
> jsgc.cpp
> jshash.cpp
> jslog2.cpp
> jsobj.cpp
> jsprf.cpp
> jsutil.cpp
> jsxdrapi.cpp

History: NSPR started as a separate library used by the JS engine, then around 1998 it was forked into js/src. So js{arena,hash,log2,prf,util}.cpp were from the original NSPR (more or less). Same would apply to jsdhash.cpp if I'd added it to NSPR 1. This leaves js{,dbg,xdr}api.cpp whose names all shout API, and jscntxt.cpp and jsgc.cpp, which have historically very recent JS_PUBLIC_API entry points defined in them.

So what, you ask? Good question. The objection to defining new API all over could probably be overcome with static inline locals that you want to inline-expand in the API entry point, but is that an issue here? If not, or if it can be avoided, use jsapi.cpp. There are competing values of inline-ability vs. one stop API shopping at work.

/be
(Assignee)

Comment 17

9 years ago
2:1 I lose. I will move it into jsapi. Waldo you can r+ based on that nit if you want.
The "one stop API shopping" or "APIs on the outside" is a value because it tends (no guarantees, but tendencies and norms help) to keep API disentangled from implementation. JSC and V8 have APIs segregated (IIRC, V8 patterned after JSC) and JSC at least, Ollie Hunt tells me, has strict ABI compatibility constraints on its exported API.

I've been talking up a big source reorg, perhaps when JaegerMonkey lands, so that could be a good time to further separate API from implementation. Inline-ability can be helped by jsfooinlines.h, or FooInlines.h in the StudlyCaps file naming convention.

ABI compatibility may not matter to us right now, but it could come to matter with ctypes, popular/decoupled platform consumers (add-ons), or our own desires to field-upgrade just the engine. It's not a goal now but it could be, and it is easier to do with segregated API sources.

/be
Comment on attachment 433674 [details] [diff] [review]
patch

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

> static JSBool
>+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
>+        return false;
>+    }
>+    JSObject *obj = JSVAL_TO_OBJECT(vp[2]);
>+    jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID;
>+    return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp);
>+}

You misunderstand the distinction between jsval and jsid.  There can be umpteen different jsvals which all correspond to a JSString consisting of the characters "foobar", because there can be multiple such strings.  There can only be one jsid which corresponds to the characters "foobar"; if you had more, that's going to start messing things up in unpredictable ways.  (Here, you might get multiple properties of the same name!)  jsval and jsid might be type-compatible in C++, but they aren't the same, and you can't safely pass in a jsval where a jsid is expected.  You need this bit that you cut out from the original code to get a proper jsid:

-    AutoIdRooter nameidr(cx);
-    if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr()))
-        return JS_FALSE;


> /* ES5 15.2.3.6: Object.defineProperty(O, P, Attributes) */
> static JSBool
> obj_defineProperty(JSContext* cx, uintN argc, jsval* vp)
> {
>     /* 15.2.3.6 steps 1 and 5. */
>     jsval v = (argc == 0) ? JSVAL_VOID : vp[2];
>     if (JSVAL_IS_PRIMITIVE(v)) {
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
>         return JS_FALSE;
>     }
>     *vp = vp[2];
>     JSObject* obj = JSVAL_TO_OBJECT(*vp);
> 
>     /* 15.2.3.6 step 2. */
>-    AutoIdRooter nameidr(cx);
>-    if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr()))
>-        return JS_FALSE;
>+    jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID;
> 
>     /* 15.2.3.6 step 3. */
>-    AutoDescriptorArray descs(cx);
>-    PropertyDescriptor *desc = descs.append();
>-    if (!desc || !desc->initialize(cx, nameidr.id(), argc >= 3 ? vp[4] : JSVAL_VOID))
>-        return JS_FALSE;
>+    jsval descval = argc >= 3 ? vp[4] : JSVAL_VOID;
> 
>     /* 15.2.3.6 step 4 */
>-    bool dummy;
>-    return DefineProperty(cx, obj, *desc, true, &dummy);
>+    JSBool junk;
>+    return JS_DefineOwnProperty(cx, obj, nameval, descval, &junk);

Same thing again here.
Attachment #433674 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 20

9 years ago
Created attachment 434137 [details] [diff] [review]
patch
Attachment #433674 - Attachment is obsolete: true
(Assignee)

Comment 21

9 years ago
Created attachment 434140 [details] [diff] [review]
patch

I just caught a bad bug the review didn't flag, so please look again carefully. Stuff shifted around a lot the last few review cycles.
Attachment #434137 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #434140 - Flags: review?(jwalden+bmo)
Comment on attachment 434140 [details] [diff] [review]
patch

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp

> JS_PUBLIC_API(JSBool)
>+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
>+{
>+    CHECK_REQUEST(cx);
>+
>+    return js_GetOwnPropertyDescriptor(cx, obj, id, vp);
>+}

Nix the newline here.


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>+JSBool
>+js_DefineOwnProperty(JSContext *cx, JSObject *obj, jsid id, jsval descriptor, JSBool *bp)
>+{
>+    AutoDescriptorArray descs(cx);
>+    PropertyDescriptor *desc = descs.append();
>+
>+    if (!desc ||
>+        !desc->initialize(cx, id, descriptor)) {
>+        return false;
>+    }

Nix the newline, make that a single-line if without braces.


>+    bool rval;
>+    if (!DefineProperty(cx, obj, *desc, true, &rval))
>+        return false;
>+    *bp = !!rval;
>+    return true;

Were you not looking at compiler warning output here?
Attachment #434140 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 23

9 years ago
> Were you not looking at compiler warning output here?

What do you mean?
I meant with respect to previous versions of the patch, where |*bp = !!rval| immediately followed a return statement.
(Assignee)

Comment 25

9 years ago
It was a mis-merge from rebasing. Did you not look at the patch you reviewed? =)
(Assignee)

Comment 26

9 years ago
http://hg.mozilla.org/tracemonkey/rev/cdb5a9fd8395
Whiteboard: fixed-in-tracemonkey
I looked, and I r-'d it.  :-)  Regarding the specific spot, I assume patch writers take care to eliminate compiler warnings before posting patches, because that's usually the case and the trouble testing it usually doesn't pay off.  But the point still stands: check patches for compiler warnings.

Comment 28

9 years ago
http://hg.mozilla.org/mozilla-central/rev/cdb5a9fd8395
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 566818

Updated

9 years ago
No longer blocks: 566818
You need to log in before you can comment on or make changes to this bug.