Last Comment Bug 707096 - We need a public API for TypedArray / ArrayBuffer
: We need a public API for TypedArray / ArrayBuffer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
https://bugzilla.mozilla.org/show_bug...
Depends on:
Blocks: 707148 707676
  Show dependency treegraph
 
Reported: 2011-12-02 00:55 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-01-11 17:59 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
JSAPI: Adding JS_IsArrayBufferObject, JS_NewArrayBufferObject, JS_GetArrayBufferLength, JS_GetArrayBufferData (2.95 KB, patch)
2011-12-02 05:23 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
v2 (1.99 KB, patch)
2011-12-12 05:15 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dmandelin: review+
Details | Diff | Review
v3. Moving this to jsfriendapi.h (5.00 KB, patch)
2011-12-14 00:28 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dmandelin: review-
Details | Diff | Review
v4. As v2, but without the typo. (2.00 KB, patch)
2011-12-15 09:28 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dmandelin: review+
Details | Diff | Review
Final version (2.01 KB, patch)
2012-01-06 02:46 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Final version, merged (2.14 KB, patch)
2012-01-10 00:52 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-02 00:55:20 PST
For the moment, all ArrayBuffer APIs are defined in jstypedarray.h and prefixed with js_*. From what I understand, this means that using them is not legal yet. We should export [some of] these APIs into jsapi.h with a JS_* name.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-02 05:23:54 PST
Created attachment 578555 [details] [diff] [review]
JSAPI: Adding JS_IsArrayBufferObject, JS_NewArrayBufferObject, JS_GetArrayBufferLength, JS_GetArrayBufferData
Comment 2 David Mandelin [:dmandelin] 2011-12-02 11:23:15 PST
Comment on attachment 578555 [details] [diff] [review]
JSAPI: Adding JS_IsArrayBufferObject, JS_NewArrayBufferObject, JS_GetArrayBufferLength, JS_GetArrayBufferData

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

For now, we're putting the typed array functions in jstypedarray.{h,cpp} marked as JS_FRIEND_API. The idea is that they're going to be public APIs someday, are OK to use in the browser, but are still stabilizing so they are not part of the standard public API.

::: js/src/jsapi.cpp
@@ +4222,5 @@
>      return js_SetReservedSlot(cx, obj, index, v);
>  }
>  
> +JS_PUBLIC_API(JSBool)
> +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj)

This doesn't need to (and therefore should not) take a cx. The usual rule is that if the function can fail, it takes a cx on which to report the exception. Otherwise it doesn't.

::: js/src/jsapi.h
@@ +3736,5 @@
> + * ArrayBuffers
> + */
> +
> +extern JS_PUBLIC_API(JSBool)
> +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj);

Please rename to JS_IsArrayBuffer.

@@ +3739,5 @@
> +extern JS_PUBLIC_API(JSBool)
> +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj);
> +
> +extern JS_PUBLIC_API(JSObject *)
> +JS_NewArrayBufferObject(JSContext *cx, jsint length);

Rename to JS_NewArrayBuffer.

@@ +3742,5 @@
> +extern JS_PUBLIC_API(JSObject *)
> +JS_NewArrayBufferObject(JSContext *cx, jsint length);
> +
> +extern JS_PUBLIC_API(JSBool)
> +JS_GetArrayBufferLength(JSContext *cx, JSObject *obj, jsuint *lengthp);

This one seems redundant with JS_GetArrayBufferByteLength.

@@ +3745,5 @@
> +extern JS_PUBLIC_API(JSBool)
> +JS_GetArrayBufferLength(JSContext *cx, JSObject *obj, jsuint *lengthp);
> +
> +extern JS_PUBLIC_API(void*)
> +JS_GetArrayBufferDataPtr(JSContext *cx, JSObject *obj);

This one seems redundant with JS_GetArrayBufferData.
Comment 3 Tom Schuster [:evilpie] 2011-12-04 07:41:42 PST
> ::: js/src/jsapi.h
> @@ +3736,5 @@
> > + * ArrayBuffers
> > + */
> > +
> > +extern JS_PUBLIC_API(JSBool)
> > +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj);
> 
> Please rename to JS_IsArrayBuffer.
> 
I prefer JS_ObjectIsArrayBuffer as we have ObjectIsDate etc. (only IsArrayObject is out of line)
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-12 05:15:33 PST
Created attachment 580883 [details] [diff] [review]
v2

Well, here is a v2. But it is so trivial that I cannot help but assume that I must have missed something.
Comment 5 David Mandelin [:dmandelin] 2011-12-12 11:52:05 PST
Comment on attachment 580883 [details] [diff] [review]
v2

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

r+ with typo fixed.

::: js/src/jstypedarray.cpp
@@ +2421,5 @@
>  
> +JS_FRIEND_API(JSObject *)
> +JS_NewArrayBuffer(JSContext *cx, jsuint nbytes)
> +{
> +  return JS_NewArrayBuffer(cx, nbytes);

This should be js_. :-)
Comment 6 :Ms2ger 2011-12-12 12:01:06 PST
Can we please at least move them to jsfriendapi.h? Requiring Gecko to include jstypedarray.h only makes it harder to make sure that we only use the blessed API.
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-13 03:42:19 PST
(In reply to Ms2ger from comment #6)
> Can we please at least move them to jsfriendapi.h? Requiring Gecko to
> include jstypedarray.h only makes it harder to make sure that we only use
> the blessed API.

I am willing to do that if necessary, for the new files. However, moving the JS_FRIEND_API(...) functions that already appear in jstypedarray.h is a little beyond the scope of my trivial contribution, and I would rather avoid the refactoring-in-a-refactoring syndrome.
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-14 00:28:59 PST
Created attachment 581556 [details] [diff] [review]
v3. Moving this to jsfriendapi.h

Here it is.

I have also experimented with publishing all JS_FRIEND_API(...) functions of jstypedarray.h in jsfriendapi.h. For some reason that I could not discover (perhaps a namespace issue? or an incompatible |extern "C"|?), this breaks linking, so I have rolledback to just adding the new friend functions in jsfriendapi.h .
Comment 9 David Mandelin [:dmandelin] 2011-12-14 18:17:24 PST
Comment on attachment 581556 [details] [diff] [review]
v3. Moving this to jsfriendapi.h

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

Gecko already includes jstypedarray.h. Let's just land the v2 patch rather than splitting up the API among different files or wasting time on linker issues.
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-15 09:28:33 PST
Created attachment 582008 [details] [diff] [review]
v4. As v2, but without the typo.

Ok, then let's just fix the typo.
Comment 11 David Mandelin [:dmandelin] 2011-12-15 13:03:06 PST
Comment on attachment 582008 [details] [diff] [review]
v4. As v2, but without the typo.

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

It'd be nice to move the declarations near the other JS_ files, but you don't need a new review for that, you can just move then and land.
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-06 02:46:41 PST
Created attachment 586375 [details] [diff] [review]
Final version

Ok, just moving things together in the .h
Comment 13 Boris Zbarsky [:bz] 2012-01-09 08:01:09 PST
This patch doesn't apply on inbound tip...  Can you please post a merged version, then set checkin-needed again?
Comment 14 Boris Zbarsky [:bz] 2012-01-09 08:03:17 PST
And also, the patch needs a useful User line and commit comment...
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-01-10 00:52:51 PST
Created attachment 587258 [details] [diff] [review]
Final version, merged

Here it is
Comment 17 Ed Morley [:emorley] 2012-01-11 17:59:46 PST
https://hg.mozilla.org/mozilla-central/rev/95ec5fc65854

Note You need to log in before you can comment on or make changes to this bug.