The default bug view has changed. See this FAQ.

We need a public API for TypedArray / ArrayBuffer

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

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.
Created attachment 578555 [details] [diff] [review]
JSAPI: Adding JS_IsArrayBufferObject, JS_NewArrayBufferObject, JS_GetArrayBufferLength, JS_GetArrayBufferData
Attachment #578555 - Flags: review?(dmandelin)
Blocks: 707148
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.
Attachment #578555 - Flags: review?(dmandelin)
> ::: 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)
Blocks: 707674
Blocks: 707676
No longer blocks: 563742
No longer blocks: 707674
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.
Attachment #578555 - Attachment is obsolete: true
Attachment #580883 - Flags: review?(dmandelin)
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_. :-)
Attachment #580883 - Flags: review?(dmandelin) → review+
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.
(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.
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 .
Assignee: general → dteller
Attachment #580883 - Attachment is obsolete: true
Attachment #581556 - Flags: review?(dmandelin)
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.
Attachment #581556 - Flags: review?(dmandelin) → review-
Attachment #580883 - Attachment is obsolete: false
Created attachment 582008 [details] [diff] [review]
v4. As v2, but without the typo.

Ok, then let's just fix the typo.
Attachment #580883 - Attachment is obsolete: true
Attachment #582008 - Flags: review?(dmandelin)
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.
Attachment #582008 - Flags: review?(dmandelin) → review+
Created attachment 586375 [details] [diff] [review]
Final version

Ok, just moving things together in the .h
Attachment #581556 - Attachment is obsolete: true
Attachment #582008 - Attachment is obsolete: true
Keywords: checkin-needed
This patch doesn't apply on inbound tip...  Can you please post a merged version, then set checkin-needed again?
Keywords: checkin-needed
And also, the patch needs a useful User line and commit comment...
Created attachment 587258 [details] [diff] [review]
Final version, merged

Here it is
Attachment #586375 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/95ec5fc65854
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/95ec5fc65854
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.