Andreas Gal :gal Assignee
	Description • 15 years ago

      No description provided.

	Jeff Walden [:Waldo]
	Comment 2 • 15 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.

	Andreas Gal :gal Assignee
	Comment 3 • 15 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.

	Brendan Eich [:brendan]
	Comment 4 • 15 years ago

(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

	Andreas Gal :gal Assignee
	Comment 5 • 15 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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 6 • 15 years ago

I wonder if it's worth just changing JS_GetPropertyDescriptorById. It's a new API as of not too long ago.

	Brendan Eich [:brendan]
	Comment 7 • 15 years 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

	Andreas Gal :gal Assignee
	Comment 9 • 15 years ago

review ping

	Andreas Gal :gal Assignee
	Comment 10 • 15 years ago

refreshed patch

	Jeff Walden [:Waldo]
	Comment 11 • 15 years ago

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.

	Andreas Gal :gal Assignee
	Comment 12 • 15 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.

	Andreas Gal :gal Assignee
	Comment 13 • 15 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.

	Andreas Gal :gal Assignee
	Comment 14 • 15 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

	Brendan Eich [:brendan]
	Comment 16 • 15 years ago

(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

	Andreas Gal :gal Assignee
	Comment 17 • 15 years ago

2:1 I lose. I will move it into jsapi. Waldo you can r+ based on that nit if you want.

	Brendan Eich [:brendan]
	Comment 18 • 15 years ago

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

	Jeff Walden [:Waldo]
	Comment 19 • 15 years ago

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.

	Andreas Gal :gal Assignee
	Comment 21 • 15 years ago

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.

	Jeff Walden [:Waldo]
	Comment 22 • 15 years ago

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?

	Andreas Gal :gal Assignee
	Comment 23 • 15 years ago

> Were you not looking at compiler warning output here?

What do you mean?

	Jeff Walden [:Waldo]
	Comment 24 • 15 years ago

I meant with respect to previous versions of the patch, where |*bp = !!rval| immediately followed a return statement.

	Andreas Gal :gal Assignee
	Comment 25 • 15 years ago

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

	Andreas Gal :gal Assignee
	Comment 26 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/cdb5a9fd8395

	Jeff Walden [:Waldo]
	Comment 27 • 15 years ago

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.

	Robert Sayre
	Comment 28 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/cdb5a9fd8395