Closed
Bug 1211607
Opened 9 years ago
Closed 9 years ago
Comment jsapi.h entry points that call object internal methods
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(9 files)
8.25 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.26 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
41.63 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
13.77 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.98 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
12.37 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
9.25 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
4.85 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
19.97 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8669870 -
Flags: review?(jdemooij)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8669871 -
Flags: review?(jdemooij)
Assignee | ||
Updated•9 years ago
|
Attachment #8669870 -
Attachment is obsolete: true
Attachment #8669870 -
Flags: review?(jdemooij)
Assignee | ||
Updated•9 years ago
|
Attachment #8669870 -
Attachment is obsolete: false
Attachment #8669870 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8669872 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8669874 -
Flags: review?(jdemooij)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8669875 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8669876 -
Flags: review?(jdemooij)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8669877 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8669879 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8669881 -
Flags: review?(jdemooij)
Comment 10•9 years ago
|
||
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.
Attachment #8669870 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8669871 -
Flags: review?(jdemooij) → review+
Comment 11•9 years ago
|
||
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.
Attachment #8669872 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8669874 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8669875 -
Flags: review?(jdemooij) → review+
Comment 12•9 years ago
|
||
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.
Attachment #8669876 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8669877 -
Flags: review?(jdemooij) → review+
Comment 13•9 years ago
|
||
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?
Attachment #8669879 -
Flags: review?(jdemooij) → review+
Comment 14•9 years ago
|
||
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?
Attachment #8669881 -
Flags: review?(jdemooij) → review+
Comment 15•9 years ago
|
||
(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•9 years ago
|
||
(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 :)
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Assignee | ||
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•