1211607 - Comment jsapi.h entry points that call object internal methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: , part 1 - Document JSAPI functions that call object internal methods: JS_GetPrototype through JS_PreventExtensions

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: , part 2 - JS_GetOwnPropertyDescriptor and friends

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: , part 3 - JS_DefineProperty and friends

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: , part 4 - JS_HasProperty and friends

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: , part 5 - JS_GetProperty and friends

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: , part 6 - JS_SetProperty and friends

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: , part 7 - JS_DeleteProperty and friends

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: , part 8 - JS_Enumerate

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: , part 9 - JS::Call, JS::Construct, and friends

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8669870 [details] [diff] [review]
, part 1 - Document JSAPI functions that call object internal methods: JS_GetPrototype through JS_PreventExtensions

Review of attachment 8669870 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2754,5 @@
> +extern JS_PUBLIC_API(bool)
> +JS_GetPrototype(JSContext* cx, JS::HandleObject obj, JS::MutableHandleObject result);
> +
> +/**
> + * Change the prototype of obj.

Should we use // comments? I guess this is more consistent with the rest of the file.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8669872 [details] [diff] [review]
, part 3 - JS_DefineProperty and friends

Review of attachment 8669872 [details] [diff] [review]:
-----------------------------------------------------------------

The jsapi.cpp diff is a bit hard to read but I assume this is just moving code.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8669876 [details] [diff] [review]
, part 6 - JS_SetProperty and friends

Review of attachment 8669876 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +3051,5 @@
> +extern JS_PUBLIC_API(bool)
> +JS_SetPropertyById(JSContext* cx, JS::HandleObject obj, JS::HandleId id, JS::HandleValue v);
> +
> +extern JS_PUBLIC_API(bool)
> +JS_SetProperty(JSContext* cx, JS::HandleObject obj, const char* name, JS::HandleValue v);

Do you think it's worth removing a lot of those overloads by simply having one version that takes HandleId, and requiring all callers to use that? Having all these overloads is convenient maybe but also a bit much/confusing.

Not for this bug, but just wondering.

@@ +3064,5 @@
> +extern JS_PUBLIC_API(bool)
> +JS_SetElement(JSContext* cx, JS::HandleObject obj, uint32_t index, JS::HandleObject v);
> +
> +extern JS_PUBLIC_API(bool)
> +JS_SetElement(JSContext* cx, JS::HandleObject obj, uint32_t index, JS::HandleString v);

Same here, it seems unnecessary to have a separate function for each possible Value type.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8669879 [details] [diff] [review]
, part 8 - JS_Enumerate

Review of attachment 8669879 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2703,5 @@
>  
>  template <>
> +struct GCMethods<JSPropertyDescriptor> {
> +    static JSPropertyDescriptor initial() { return JSPropertyDescriptor(); }
> +};

Why does this patch need this?

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8669881 [details] [diff] [review]
, part 9 - JS::Call, JS::Construct, and friends

Review of attachment 8669881 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, nice to have these comments!

::: js/src/jsapi.h
@@ +3137,5 @@
>  
> +/*
> + * API for determining callability and constructability. [[Call]] and
> + * [[Construct]] are internal methods that aren't present on all objects, so it
> + * is useful to ask if they are there or not. The ES6 standard itself asks

Nit: maybe leave out the "ES6", so we don't need to update the comment in a year or two?

Also, ES2015? ;)

@@ +3249,5 @@
> + * Invoke a constructor. This is the C++ equivalent of
> + * `rval = new fun(...args)`.
> + *
> + * The value left in rval on success is always an object in practice,
> + * though at the moment this is not enforced by the C++ type system.

Follow-up good first bug?

Comment 15

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> > +extern JS_PUBLIC_API(bool)
> > +JS_SetPropertyById(JSContext* cx, JS::HandleObject obj, JS::HandleId id, JS::HandleValue v);
> > +extern JS_PUBLIC_API(bool)
> > +JS_SetProperty(JSContext* cx, JS::HandleObject obj, const char* name, JS::HandleValue v);
> 
> Do you think it's worth removing a lot of those overloads by simply having
> one version that takes HandleId, and requiring all callers to use that?
> Having all these overloads is convenient maybe but also a bit much/confusing.
> 
> Not for this bug, but just wondering.

I've wanted to do this for awhile, low-priority.

> > +extern JS_PUBLIC_API(bool)
> > +JS_SetElement(JSContext* cx, JS::HandleObject obj, uint32_t index, JS::HandleObject v);
> > +extern JS_PUBLIC_API(bool)
> > +JS_SetElement(JSContext* cx, JS::HandleObject obj, uint32_t index, JS::HandleString v);
> 
> Same here, it seems unnecessary to have a separate function for each
> possible Value type.

Those got added because people wanted efficiency.  :-\  Tho given we have library overhead for JSAPI calls, I'm a bit dubious.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> > Same here, it seems unnecessary to have a separate function for each
> > possible Value type.
> 
> Those got added because people wanted efficiency.  :-\  Tho given we have
> library overhead for JSAPI calls, I'm a bit dubious.

Hm, all the JS_SetElement overloads that take a specific type like int32_t have a RootedValue and then pass that to SetElement. I don't see how that'd be more efficient than having the RootedValue in the caller, other than *maybe* a small code size win.

Also, I bet in a lot of cases the caller now uses RootedObject/RootedString, then JS_SetElement has a RootedValue but we could have used a RootedValue directly :)

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Comment on attachment 8669876 [details] [diff] [review]
> , part 6 - JS_SetProperty and friends
> 
> Review of attachment 8669876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.h
> @@ +3051,5 @@
> > +extern JS_PUBLIC_API(bool)
> > +JS_SetPropertyById(JSContext* cx, JS::HandleObject obj, JS::HandleId id, JS::HandleValue v);
> > +
> > +extern JS_PUBLIC_API(bool)
> > +JS_SetProperty(JSContext* cx, JS::HandleObject obj, const char* name, JS::HandleValue v);
> 
> Do you think it's worth removing a lot of those overloads by simply having
> one version that takes HandleId, and requiring all callers to use that?
> Having all these overloads is convenient maybe but also a bit much/confusing.
> 
> Not for this bug, but just wondering.

I like the convenience (it would be about 4 more lines of code at hundreds of call sites otherwise), but there must be a way to get it using templates. Maybe like this:

template <class ConvertibleToId, class ConvertibleToValue>
bool
SetProperty(JSContext* cx, HandleObject obj, ConvertibleToId cid, ConvertibleToValue cv)
{
    RootedId id(cx);
    if (!ConvertToId(cx, cid, &id))
        return false;

    RootedValue v(cx);
    if (!ConvertToValue(cx, cv, &v))
        return false;

    return SetProperty(cx, obj, id, value);
}

I'll follow up.

> Same here, it seems unnecessary to have a separate function for each
> possible Value type.

Yeah.

Comment 18

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> > +struct GCMethods<JSPropertyDescriptor> {
> > +    static JSPropertyDescriptor initial() { return JSPropertyDescriptor(); }
> > +};
> 
> Why does this patch need this?

It doesn't. Leftover junk from rebasing. I deleted it.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> > + * API for determining callability and constructability. [[Call]] and
> > + * [[Construct]] are internal methods that aren't present on all objects, so it
> > + * is useful to ask if they are there or not. The ES6 standard itself asks
> 
> Nit: maybe leave out the "ES6", so we don't need to update the comment in a
> year or two?

Yeah, it reads better that way.

> Also, ES2015? ;)

No comment.

(re: Construct)
> > + * The value left in rval on success is always an object in practice,
> > + * though at the moment this is not enforced by the C++ type system.
> 
> Follow-up good first bug?

Filed bug 1212533.

Comment 20

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a708dd591ce53cfe49477218a8315464d352cd2
Bug 1211607, part 1 - Document JSAPI functions that call object internal methods: JS_GetPrototype through JS_PreventExtensions. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e46d83014d34b915feaeb5b4723207d002b3ba2
Bug 1211607, part 2 - JS_GetOwnPropertyDescriptor and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e64437e98647873cfd71ed0d047c4c217778425
Bug 1211607, part 3 - JS_DefineProperty and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc2e52c5c46924e280aa65b82fb18991819ca8a
Bug 1211607, part 4 - JS_HasProperty and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/956c8a74c79d7b69ae2de33317a0b15b0cd0f1ce
Bug 1211607, part 5 - JS_GetProperty and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cca514ee20fd45d2f694e1cec17e6a6c2bbf2cd6
Bug 1211607, part 6 - JS_SetProperty and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51f068fece31bf84bf4984e272e29998f7bbff7
Bug 1211607, part 7 - JS_DeleteProperty and friends. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eca86ad4632773635b3fca76037a4278eebf7cc
Bug 1211607, part 8 - JS_Enumerate. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c23963b599cb16d6f58420fad382c4322c7177c
Bug 1211607, part 9 - JS::Call, JS::Construct, and friends. r=jandem.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a708dd591ce
https://hg.mozilla.org/mozilla-central/rev/9e46d83014d3
https://hg.mozilla.org/mozilla-central/rev/7e64437e9864
https://hg.mozilla.org/mozilla-central/rev/ebc2e52c5c46
https://hg.mozilla.org/mozilla-central/rev/956c8a74c79d
https://hg.mozilla.org/mozilla-central/rev/cca514ee20fd
https://hg.mozilla.org/mozilla-central/rev/a51f068fece3
https://hg.mozilla.org/mozilla-central/rev/1eca86ad4632
https://hg.mozilla.org/mozilla-central/rev/1c23963b599c