Closed Bug 707096 Opened 13 years ago Closed 12 years ago

We need a public API for TypedArray / ArrayBuffer

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: Yoric, Assigned: Yoric)

References

()

Details

Attachments

(1 file, 5 obsolete files)

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.
Attachment #578555 - Flags: review?(dmandelin)
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)
No longer blocks: 707674
Attached patch v2 (obsolete) — Splinter Review
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.
Attached patch v3. Moving this to jsfriendapi.h (obsolete) — Splinter Review
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
Attached patch v4. As v2, but without the typo. (obsolete) — Splinter Review
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+
Attached patch Final version (obsolete) — Splinter Review
Ok, just moving things together in the .h
Attachment #581556 - Attachment is obsolete: true
Attachment #582008 - Attachment is obsolete: true
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...
Attachment #586375 - Attachment is obsolete: true
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
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: