Could you provide a description of the bug?

Any point in keeping js::IsArrayBuffer around?

Comment on attachment 588655 [details] [diff] [review] content/base > nsContentUtils::CreateArrayBuffer(JSContext *aCx, const nsACString& aData, > JSObject** aResult) >+ JS_ASSERT(JS_IsArrayBufferObject(*aResult)); MOZ_ASSERT, please.

Comment on attachment 588660 [details] [diff] [review] dom Review of attachment 588660 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 588653 [details] [diff] [review] xpconnect r+, assuming these methods are just renames of the old ones.

>Any point in keeping js::IsArrayBuffer around? I don't think, it does any harm, and corresponds to how I did JS_IsTypedArray. >> nsContentUtils::CreateArrayBuffer(JSContext *aCx, const nsACString& aData, >> JSObject** aResult) >>+ JS_ASSERT(JS_IsArrayBufferObject(*aResult)); >MOZ_ASSERT, please. Actually this assert is not even needed, because JS_GetArrayBufferData in the next line, asserts the same. I am going to remove it. One overall note, a lot of the in content/ often did something like: >if (js_IsTypedArray(obj)) { > inner = js::TypedArray::getTypedArray(obj); > // do stuff with inner >} From my understanding if obj already has a typed array class, we never actually searched on the proto chain, so this was unnecessary. (Correct me if, I am wrong)

> so this was unnecessary. That code makes no sense. Maybe it meant to have a '!' in front of the js_IsTypedArray call? Please check with whoever has blame for that code?

A question. This looks like it adds another level of function call overhead to the various APIs involved, right? Is there any way to avoid that?

No this example is correct, but I might have an explanation. The API used to be different, so you had to extract the TypedArray struct from the JSObject, so this call was needed. For example see this diff, https://hg.mozilla.org/mozilla-central/diff/025d0712bfce/content/canvas/src/CustomQS_Canvas2D.h. While, I doubt that this one call is going to matter much, I could do try some benchmarks, I am sure Waldo has some suggestions, too.

I can't do this reviewing without some documentation. I barely knew the old TypedArrays API we're replacing here, and don't know the new one at all; without knowing how/whether these functions are different or just synonyms, I can't review. I'd be happy to review given the needed documentation of these functions. OTOH, it seems that none of that is very canvas/WebGL specific, so someone knowing the old and new Typed Arrays APIs could review this.

Maybe I should have said that, but the functions are the same (they still call the same internal function). I assumed everybody would assume that. For the only intended change see comment 12 - 15. If you don't want to do the review, you are free to forward it to everybody you would like.

Comment on attachment 588652 [details] [diff] [review] js/src patch Review of attachment 588652 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsfriendapi.h @@ +478,5 @@ > + TYPED_ARRAY_TYPE_FLOAT64, > + > + /* > + * Special type that's a uint8, but assignments are clamped to 0 .. 255. > + * Treat the raw data type as a uint8. uint8_t, both places. @@ +490,5 @@ > + * Create a new typed array of type atype (one of the TypedArray > + * enumerant values above), with nelements elements. > + */ > +JS_FRIEND_API(JSObject *) > +JS_NewTypedArray(JSContext *cx, uint32_t type, int32_t nelements); I think it would be better if we had explicitly typed (ha ha) methods for all these, not enum-switched. So this should be JS_NewInt8Array, JS_NewUint8Array, ..., JS_NewUint8ClampedArray. Yeah, it's verbose. And it gets worse once you do the same for JS_NewUint8ArrayFromArray and JS_NewUint8ArrayWithBuffer (so assume this comment applies to those APIs as well). :-\ But it seems much preferable to me than to have an enum-switched thing. @@ +495,5 @@ > + > +/* > + * Create a new typed array of type atype (one of the TypedArray > + * enumerant values above), and copy in values from the given JSObject, > + * which must either be a typed array or an array-like object. The "which" phrase is kind of awkward. What we really mean is that we take the "length" property of the object and copy that many elements. But if there's no "length" property, we treat it as 0. So, really, you can pass any object at all here. The only constraint (other than "length", "0", "1", etc. not throwing on get) is that if you pass an object where its "length" is too big, we'll throw. So how about this: Create a new typed array and copy in values from the given object. The object is used as if it were an array; that is, the new array (if successfully created) will have length given by obj.length, and its elements will be those specified by obj[0], obj[1], and so on, after conversion to the typed array element type. My only slight concern about this description is that obj[0], obj[1] could be interpreted as if they were C expressions, but I think |obj.length| at least should clarify that in context "obj" is a JS expression, not a C expression. @@ +502,5 @@ > +JS_NewTypedArrayFromArray(JSContext *cx, uint32_t type, JSObject *arrayArg); > + > +/* > + * Create a new typed array of type atype (one of the TypedArray > + * enumerant values above), using a given ArrayBuffer for storage. "using the given" @@ +512,5 @@ > +JS_NewTypedArrayWithBuffer(JSContext *cx, uint32_t type, JSObject *bufArg, > + int32_t byteoffset, int32_t length); > + > +JS_FRIEND_API(JSBool) > +JS_IsArrayBufferObject(JSObject *obj); I would ditch "Object" here, but I guess this is for consistency with JS_IsArrayObject and JS_ObjectIsRegExp (name botch on the latter, but meh)? @@ +515,5 @@ > +JS_FRIEND_API(JSBool) > +JS_IsArrayBufferObject(JSObject *obj); > + > +JS_FRIEND_API(JSObject *) > +JS_NewArrayBuffer(JSContext *cx, jsuint nbytes); uint32_t, not jsuint. @@ +518,5 @@ > +JS_FRIEND_API(JSObject *) > +JS_NewArrayBuffer(JSContext *cx, jsuint nbytes); > + > +JS_FRIEND_API(uint32_t) > +JS_GetArrayBufferByteLength(JSObject *obj); What happens if |obj| is not an ArrayBuffer? The reader shouldn't have to look up the behavior here by examining the implementation, but that's exactly what everyone who uses this method will have to do. More generally, these new APIs should all be commented to explain what they do. Sometimes that's really short -- "Returns true if obj is an ArrayBuffer object." Sometimes it's longer, if the method makes assumptions/assertions (like here, I believe, that the specified object is an ArrayBuffer). But in any case, I think we should move away from just adding declarations without descriptions. And many of these methods definitely need descriptions (and not just in the sense of needing them just for style). ::: js/src/jstypedarray.cpp @@ +2510,4 @@ > JS_FRIEND_API(JSObject *) > +JS_NewArrayBuffer(JSContext *cx, jsuint nbytes) > +{ > + return js::CreateArrayBuffer(cx, nbytes); Inside using namespace js, you can/should not prefix uses of namespace-js methods and values. @@ +2515,5 @@ > + > +JS_FRIEND_API(JSObject *) > +JS_NewTypedArray(JSContext *cx, uint32_t atype, int32_t nelements) > +{ > + return js::CreateTypedArray(cx, atype, nelements); Same. @@ +2529,5 @@ > } > > JS_FRIEND_API(JSObject *) > +JS_NewTypedArrayWithArrayBuffer(JSContext *cx, uint32_t atype, JSObject *bufArg, > + int32_t byteoffset, int32_t length) alignment @@ +2534,3 @@ > { > JS_ASSERT(atype >= 0 && atype < TypedArray::TYPE_MAX); > + JS_ASSERT(bufArg && IsArrayBuffer(bufArg)); Split this one in two, please, for better clarity in what went wrong if something does go wrong? ::: js/src/methodjit/PolyIC.cpp @@ +2609,5 @@ > * IC are not compatible with carrying entries in floating point registers. > * Since we can use type information to generate inline paths for typed > * arrays, just don't generate these ICs with inference enabled. > */ > + if (!f.cx->typeInferenceEnabled() && js::IsTypedArray(obj)) This is probably inside a using namespace js, so kill the prefix. @@ +2907,5 @@ > return attachHoleStub(f, obj, key); > > #if defined JS_METHODJIT_TYPED_ARRAY > /* Not attaching typed array stubs with linear scan allocator, see GetElementIC. */ > + if (!f.cx->typeInferenceEnabled() && js::IsTypedArray(obj)) Ditto.

Tom, I'm in the same boat as Benoit (comment 16). I'm not sure I can give a useful review for the audio portion.

Okay, need to figure something out for the other reviews, I guess somebody from the JS team could do them (or maybe Ms2ger to rescue?). Waldo: I agree with your review, as most people really seem not to understand what this is for. So I will try to beef up the comments there a bit. I hugely follow your demand after type-safety, but I also see the ugliness coming from these methods. My thinking was an api that is supposed to be semi-internal could expose some stuff? Everything is better than including the jstypedarray.h after all :)

Comment on attachment 588654 [details] [diff] [review] content/canvas but mostly WebGL Review of attachment 588654 [details] [diff] [review]: ----------------------------------------------------------------- a somewhat hesitant r+, but if this breaks stuff, this is going to give lots of WebGL test failures, I don't think this could break stuff without tests catching it.

I added two new methods to CustomQS_WebGL.h in bug 721230 which use the old typed arrays API. Once that lands on m-c, I'll provide a patch here to use the new jsfriend typed arrays API with the two new methods.

Don't worry, did read Comment 18? We are probably going to change the API, again. I am gone next week anyway, so when I need to fix the other stuff, I could as well fix yours, too.

Comment on attachment 588655 [details] [diff] [review] content/base r=me, I guess, but this really needs review from someone on the JS team who understands the relevant typed array API changes (e.g. why looking for the array buffer is no longer needed). Please get someone like that to look at this?

There were two more files mentioned in bug 707148 that I don't see in any patch here: content/canvas/src/WebGLContextValidate.cpp dom/telephony/Radio.cpp Except the 2nd of those has been removed and I don't see any relevant references in dom/telephony/*, so I think it can be ignored. Note that I have some patches that will (syntactically) conflict with these (bug 720949, bug 575688). I'd prefer for this bug to land first. I would finish this up myself, only I ran into the same question about the proto chain search when doing bug 575688.

>Note that I have some patches that will (syntactically) conflict with these (bug 720949, bug >575688). I'd prefer for this bug to land first. I would finish this up myself, only I ran into >the same question about the proto chain search when doing bug 575688. I am still around to fix this. As I said before we never actually looked at the proto because js_IsTypedArray doesn't consider the prototype. And when the object is already a typed array we wouldn't walk up the proto chain. Currently I am more concerned about the possible speed problems by not inling anymore. I could just fix up the review comments.

(In reply to Tom Schuster (evilpie) from comment #27) > I could just fix up the review comments. That would be terribly nice of you :)

Minor updates to "js/src patch"

I hope you like that :)

Looks like I forgot to remove js::, I am going to change that before pushing.

Some things seem to have changed here (quickstubs).

Comment on attachment 603311 [details] [diff] [review] content/canvas but mostly WebGL Review of attachment 603311 [details] [diff] [review]: ----------------------------------------------------------------- In bug 722154, Ms2ger is removing a lot of our existing custom quickstubs. Could you rebase your patch on top of his, so that your patch would become significantly smaller?

Comment on attachment 603307 [details] [diff] [review] audio Review of attachment 603307 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these comments. ::: content/html/content/src/nsHTMLAudioElement.cpp @@ +183,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsHTMLAudioElement::MozWriteAudio(const JS::Value &aData, JSContext *aCx, PRUint32 *aRetVal) Put the & and * on the left here, please, and elsewhere in this function. @@ +194,5 @@ > return NS_ERROR_DOM_TYPE_MISMATCH_ERR; > } > > + JSObject *darray = &aData.toObject(); > + JS::AutoObjectRooter tvr(aCx); I don't understand this GC stuff at all, so please make sure you got it right? :) @@ +206,2 @@ > } else if (JS_IsArrayObject(aCx, darray)) { > + JSObject *nobj = JS_NewFloat32ArrayFromArray(aCx, darray); No need for the |nobj| variable, I think.

Comment on attachment 603305 [details] [diff] [review] js/src Review of attachment 603305 [details] [diff] [review]: ----------------------------------------------------------------- The typed-array-type-tagging stuff just results in weirdness of class-to-tag-to-class hoops, or vice versa, so we should get rid of it. I had a couple comments from the first five minutes of looking at this, might as well make 'em even if they're not important to the patch concept. ::: js/src/Makefile.in @@ -200,5 @@ > jsprf.h \ > jsproto.tbl \ > jsprvtd.h \ > jspubtd.h \ > - jstypedarray.h \ \o/ ::: js/src/jsclone.cpp @@ +687,5 @@ > > bool > JSStructuredCloneReader::readArrayBuffer(uint32_t nbytes, Value *vp) > { > + JSObject *obj = js::CreateArrayBuffer(context(), nbytes); s/js:://

How's this bug going? It's currently blocking bug 575688.

(In reply to David Mandelin from comment #40) > How's this bug going? It's currently blocking bug 575688. Ignore this question--Steve is apparently taking this over.

Note that this applies on top of part 3 of the patches for bug 737245, but this change (reshuffling the JSAPI for typed arrays) is independent of that. There are also a series of followup patches that I'll be posting in other bugs: - removal of proto searching (will throw a TypeError instead) - implementation of DataViews - conversion of ArrayBuffer -> ArrayBufferObject (a subclass of ArrayBuffer) and possibly some other, as-yet-unwritten ones: - conversion of TypedArray -> TypedArrayObject, also dealing with the specific typed arrays - proper proxy handling But I think this patch should stand on its own. It will require the other patches in this bug to be updated to this latest exposed API.

CheckedUnwrap is now in mozilla-central, so it would probably be good to switch to that at some point if you get the chance. Not sure whether it makes more sense to do in this patch or the one underneath it. Thanks for working on this! :-)

Comment on attachment 609379 [details] [diff] [review] "Fix uses of typedarray outside the engine" [ Oops, comment & upload fail.

The API has been further updated (and documented), and now because of UnwrapObjectChecked we need to pass a JSContext* around, which required IDL changes. So passing it back to bjacob for review.

Sorry, needed to further update the API. In particular, UnwrapObjectChecked will require passing around a cx arg.

Comment on attachment 611113 [details] [diff] [review] Update typedarray API in content/canvas Sorry, screwed this up when hacking on bzexport. Newest API requires IDL changes.

Comment on attachment 611111 [details] [diff] [review] Update typedarray API usage in content/base Further modified the API. JS_Is* are now fallible due to UnwrapObjectChecked, and so require a JSContext* to be passed around.

Comment on attachment 611110 [details] [diff] [review] Update typedarray API usage in xpconnect JS_Is*Array is now fallible due to UnwrapObjectChecked, which requires a JSContext* to be passed around.

Comment on attachment 611108 [details] [diff] [review] Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the engine [ New API. JS_Is*Array is now fallible due to UnwrapObjectChecked. JS_*Unchecked variants of many accessors when already guarded by JS_Is*Array. Removed some of the enumerated type variants, but left what was still needed for structured cloning. Fleshed out the API comments. Note that this API is preparing for the introduction of UnwrapObjectChecked, but still uses plain UnwrapObject currently. I start using the checked variant in a later patch.

Further API updates, mainly switching to fallible JS_Is*Array (due to UnwrapObjectChecked) and minimizing use of enumerated view type constructors

There's a good chance this needs updating to the patch in bug 740069

Comment on attachment 611108 [details] [diff] [review] Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the engine [ Review of attachment 611108 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jstypedarray.cpp @@ +2614,5 @@ > +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj, JSBool *isArray) > +{ > + obj = UnwrapObject(obj); > + *isArray = obj->isArrayBuffer(); > + return true; This shouldn't return a boolean if it isn't fallible. Did you mean to use UnwrapObjectChecked somewhere?

Comment on attachment 611115 [details] [diff] [review] Update typedarray API usage in dom/ Review of attachment 611115 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDOMBlobBuilder.cpp @@ +441,1 @@ > return mBlobSet.AppendArrayBuffer(obj); Add {}s and reindent the body while you're here ::: dom/system/gonk/SystemWorkerManager.cpp @@ -41,1 @@ > Didn't review this file. ::: dom/workers/XMLHttpRequestPrivate.cpp @@ -40,1 @@ > I think this file has gone by now.

Comment on attachment 611120 [details] [diff] [review] Update typedarray API usage in audio code Review of attachment 611120 [details] [diff] [review]: ----------------------------------------------------------------- r- for the throwing comment, looks good otherwise. ::: content/events/src/nsDOMNotifyAudioAvailableEvent.cpp @@ +117,3 @@ > if (!mCachedArray) { > NS_DROP_JS_OBJECTS(this, nsDOMNotifyAudioAvailableEvent); > + NS_ERROR("Failed to create array for audio signal!"); Make this an NS_WARNING or remove it, please. ::: content/html/content/src/nsHTMLAudioElement.cpp @@ +193,4 @@ > return NS_ERROR_DOM_TYPE_MISMATCH_ERR; > } > > + JSObject *darray = &aData.toObject(); * on the left @@ +198,5 @@ > JSObject *tsrc = NULL; > > // Allow either Float32Array or plain JS Array > + JSBool isTypedArray; > + if (JS_IsFloat32Array(aCx, darray, &isTypedArray) && isTypedArray) { If JS_IsFloat32Array returns false, shouldn't we throw? @@ +199,5 @@ > > // Allow either Float32Array or plain JS Array > + JSBool isTypedArray; > + if (JS_IsFloat32Array(aCx, darray, &isTypedArray) && isTypedArray) { > + tsrc = darray; I wonder if this needs rooting too. @@ +204,2 @@ > } else if (JS_IsArrayObject(aCx, darray)) { > + JSObject *nobj = JS_NewFloat32ArrayFromArray(aCx, darray); Fix the * here too

(In reply to Ms2ger from comment #57) > Comment on attachment 611108 [details] [diff] [review] > Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the > engine [ > > Review of attachment 611108 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jstypedarray.cpp > @@ +2614,5 @@ > > +JS_IsArrayBufferObject(JSContext *cx, JSObject *obj, JSBool *isArray) > > +{ > > + obj = UnwrapObject(obj); > > + *isArray = obj->isArrayBuffer(); > > + return true; > > This shouldn't return a boolean if it isn't fallible. Did you mean to use > UnwrapObjectChecked somewhere? Sorry, I have a convoluted patch stack. This patch only changes the API to be fallible. Bug 741041 switches everything over to UnwrapObjectChecked. (Bug 741041 depends on bug 741040 depends on bug 575688 depends on bug 741039 depends on this. Most of those dependencies aren't real, but I've spent too much time reshuffling this patch stack already, and I had an r+ in the middle that I really didn't want to invalidate.)

(In reply to Ms2ger from comment #59) > Comment on attachment 611120 [details] [diff] [review] > Update typedarray API usage in audio code > > Review of attachment 611120 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for the throwing comment, looks good otherwise. > > > JSObject *tsrc = NULL; > > > > // Allow either Float32Array or plain JS Array > > + JSBool isTypedArray; > > + if (JS_IsFloat32Array(aCx, darray, &isTypedArray) && isTypedArray) { > > If JS_IsFloat32Array returns false, shouldn't we throw? Oops, excessive cleverness. I somehow assumed the second test was JS_IsArrayBufferObject, not JS_IsArrayObject, and figured one failure implied the next. You are correct; thanks. > > @@ +199,5 @@ > > > > // Allow either Float32Array or plain JS Array > > + JSBool isTypedArray; > > + if (JS_IsFloat32Array(aCx, darray, &isTypedArray) && isTypedArray) { > > + tsrc = darray; > > I wonder if this needs rooting too. I doubt it. darray comes from aData, and if that isn't rooted, we'd already be on shaky ground. But I changed it to root anyway, since it's not worth thinking through the logic. > > @@ +204,2 @@ > > } else if (JS_IsArrayObject(aCx, darray)) { > > + JSObject *nobj = JS_NewFloat32ArrayFromArray(aCx, darray); > > Fix the * here too I need to get me an emacs hacker who can give me an inline syntax checker that takes the source code path into account when deciding what coding conventions to follow.

Comment on attachment 611516 [details] [diff] [review] "Fix uses of typedarray outside the engine" [ Review of attachment 611516 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: content/html/content/src/nsHTMLAudioElement.cpp @@ +200,5 @@ > // Allow either Float32Array or plain JS Array > + JSBool isTypedArray; > + if (!JS_IsFloat32Array(aCx, darray, &isTypedArray)) { > + return NS_ERROR_FAILURE; > + } else if (isTypedArray) { Just 'if' (and an empty line between the blocks) please

Comment on attachment 611110 [details] [diff] [review] Update typedarray API usage in xpconnect >-CheckTargetAndPopulate(const nsXPTType& type, >+CheckTargetAndPopulate(JSContext *cx, >+ const nsXPTType& type, > PRUint8 requiredType, > size_t typeSize, > uint32_t count, > JSObject* tArr, > void** output, > nsresult* pErr) Per in-person discussion, let's just assert at the top here that IsTypedArrayObject succeeds. Might even be worth exposing some sort of assertTypedArray from spidermonkey to avoid the hassle of declaring the outparam etc. >- memcpy(*output, JS_GetTypedArrayData(tArr), byteSize); >+ void *data; >+ if (!JS_GetTypedArrayData(cx, tArr, &data)) { With the above, we can use the infallible version here. >@@ -1669,90 +1674,99 @@ XPCConvert::JSTypedArray2Native(XPCCallC > void** d, > JSObject* jsArray, > uint32_t count, > const nsXPTType& type, > nsresult* pErr) > { > NS_ABORT_IF_FALSE(jsArray, "bad param"); > NS_ABORT_IF_FALSE(d, "bad param"); >- NS_ABORT_IF_FALSE(js_IsTypedArray(jsArray), "not a typed array"); >+ JSContext* cx = ccx.GetJSContext(); >+ JSBool isArray; >+ if (!JS_IsTypedArrayObject(cx, jsArray, &isArray)) { >+ if (pErr) >+ *pErr = NS_ERROR_XPC_BAD_CONVERT_JS; >+ return false; >+ } >+ NS_ABORT_IF_FALSE(isArray, "not a typed array"); Why is this necessary? It seems like we can just assertTypedArray, since the caller already checks this. > > // Check the actual length of the input array against the > // given size_is. >- uint32_t len = JS_GetTypedArrayLength(jsArray); >+ uint32_t len; >+ JSBool ok = JS_GetTypedArrayLength(cx, jsArray, &len); >+ MOZ_ASSERT(ok); This shouldn't be necessary either.

Updated to reflect verbal review comments: - removed JS_NewTypedArray taking an enum type - buried the enum type in a C++ namespace (ArrayBufferView) - replaced JS_GetTypedArrayData with type-specific versions - replace JS_Accessor, JS_AccessorUnchecked pairs with a single JS_Accessor that takes a trailing JSContext parameter that will be used for DEBUG-only asserts I did this kinda fast, and I'll need to update all of the in-tree users again, but it should be about right.

(In reply to Steve Fink [:sfink] from comment #64) > I did this kinda fast, and I'll need to update all of the in-tree users > again, but it should be about right. Please get review on this patch before doing that...

Comment on attachment 611667 [details] [diff] [review] Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the engine Review of attachment 611667 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testTypedArrays.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- Nice testification. @@ +57,5 @@ > +typedef JSObject *(CreationFunc)(JSContext *cx, int32_t nelements); > + > +typedef void *(*GetDataFunc)(JSObject *, JSContext*); > + > +template<CreationFunc Create, typename Element> Why not just include the element-getter function in the template as well (a template parameter to the template), rather than have this VoidGetData nonsense? Needs more templates, of course, but it seems a bit better than casting function pointers. @@ +59,5 @@ > +typedef void *(*GetDataFunc)(JSObject *, JSContext*); > + > +template<CreationFunc Create, typename Element> > +bool > +TestPlainTypedArray(JSContext *cx, VoidGetData getter) Put this above the BEGIN_TEST thing, so that it gets seen before the actual test code that uses it. I'm vaguely surprised this even compiles. ::: js/src/jsclone.cpp @@ +677,5 @@ > bool > JSStructuredCloneReader::readTypedArray(uint32_t tag, uint32_t nelems, Value *vp) > { > uint32_t atype = TagToArrayType(tag); > + JSObject *obj = CreateTypedArray(context(), atype, nelems); See comments on CreateTypedArray in jstypedarray.cpp. @@ +708,5 @@ > > bool > JSStructuredCloneReader::readArrayBuffer(uint32_t nbytes, Value *vp) > { > + JSObject *obj = js::ArrayBuffer::create(context(), nbytes); We should be in a using namespace js, so the prefix isn't necessary. (If it is, add the using.) ::: js/src/jsfriendapi.h @@ +841,5 @@ > + TYPE_FLOAT32, > + TYPE_FLOAT64, > + > + /* > + * Special type that's a uint8, but assignments are clamped to 0 .. 255. [0, 256) reads nicer to me. @@ +856,5 @@ > +/* > + * Create a new typed array with nelements elements. > + */ > + > +JS_FRIEND_API(JSObject *) Should these be |extern| as well? I'd guess so, but maybe I'm wrong about that. See what other friend stuff does. @@ +857,5 @@ > + * Create a new typed array with nelements elements. > + */ > + > +JS_FRIEND_API(JSObject *) > +JS_NewInt8Array(JSContext *cx, int32_t nelements); These should be |uint32_t nelements|, since that's the right conceptual type here. Since these are all variations on a theme, I wouldn't bother with blank lines between them -- keep things a little clearer that they're all the "same", not conceptually distinct. Same for the other blocks of similar functions. @@ +927,5 @@ > + */ > + > +JS_FRIEND_API(JSObject *) > +JS_NewInt8ArrayWithBuffer(JSContext *cx, JSObject *arrayBuffer, > + int32_t byteoffset, int32_t length); Make it |uint32_t length|, please. @@ +962,5 @@ > +JS_NewFloat64ArrayWithBuffer(JSContext *cx, JSObject *arrayBuffer, > + int32_t byteoffset, int32_t length); > + > +/* > + * Create a new ArrayBuffer with the given byte length Period at the end of a complete sentence. @@ +965,5 @@ > +/* > + * Create a new ArrayBuffer with the given byte length > + */ > + > +JS_FRIEND_API(JSObject *) No blank line before this, because this comment applies to exactly this single method. This is consistent with the comment style on methods declared in jsapi.h. @@ +969,5 @@ > +JS_FRIEND_API(JSObject *) > +JS_NewArrayBuffer(JSContext *cx, uint32_t nbytes); > + > +/* > + * Check whether obj supports JS_GetTypedArray* APIs. Note that this may fail Note that the method returns an ArrayBufferView::* value. Or, better yet, change its return type to that. @@ +976,5 @@ > + * succeeds, then the "Unchecked" versions of APIs may be called on that object > + * (eg JS_GetTypedArrayTypeUnchecked). > + */ > + > +JS_FRIEND_API(JSBool) No blank line here either. (I think I'm not going to bother pointing out later instances of this, except to say that I could go either way on removing blank lines when it's before a block of similar methods, like all the JS_Is*Array methods.) @@ +1031,5 @@ > +/* > + * Return the available byte length of an array buffer. |obj| must have passed > + * a JS_IsArrayBufferObject test, or somehow be known that it would pass such a > + * test: it is an ArrayBuffer or a wrapper of an ArrayBuffer, and the > + * unwrapping will succeed. If cx is NULL, then DEBUG builds won't assert. If the object isn't a proxy, and it's not an ArrayBuffer, we could still assert even with |cx == NULL|. Right? (Here and in the other methods with this comment.) Requiring |cx != NULL| seems best, if possible. I don't know if it's possible to do that or not. If it is, we should. If not, um. I think we want to assert if we know there's a mismatch even when we have |cx == NULL|, even if we won't know all the time. I get that we want users to know that they'll get mismatch-detection on misuse, so we want a comment that includes something like it does not. I don't know how to say something, allowing for assertions sometimes when |cx == NULL|, without getting into too much overshare. Maybe you can think of something. @@ +1083,5 @@ > +/* > + * Return a pointer to the start of the data referenced by a typed array. |obj| > + * must have passed a JS_Is*Array test, or somehow be known that it would pass > + * such a test: it is a typed array or a wrapper of a typed array, and the > + * unwrapping will succeed. If cx is NULL, then DEBUG builds won't assert. Hmm. If we're handing out raw pointers to mutable data here, probably we should somehow say when it's safe to modify that data. How about saying you have to be inside a request to call these methods, and to modify the data in the pointers returned by them? ::: js/src/jstypedarray.cpp @@ +141,1 @@ > * first. Could you file a followup bug, or deal with it somewhere, to fix this last sentence? Given the semantics of the method it just doesn't make any sense -- you can hand in anything and get back something vaguely reasonable here (even if just NULL to indicate none found, or something). @@ +1081,5 @@ > class TypedArrayTemplate > : public TypedArray > { > + template<typename ElementType> > + friend JSObject *NewTypedArrayFromArray(JSContext *cx, JSObject *other); templatized friend declarations? You are more optimistic than I am -- make sure to try this before landing. :-) @@ +1703,2 @@ > { > + uint32_t boffset = (byteOffsetInt < 0) ? 0 : uint32_t(byteOffsetInt); Make the docs say "< 0", not "== -1", then. @@ +1708,5 @@ > + return NULL; // invalid byteOffset > + } > + > + uint32_t len; > + if (lengthInt < 0) { Er, hrm. It wasn't clear to me what "optional" meant in the context of this argument in the friend functions, then. XXXXXXXXXXXXXXXXrevise that old comment @@ +1712,5 @@ > + if (lengthInt < 0) { > + len = (buffer->arrayBufferByteLength() - boffset) / sizeof(NativeType); > + if (len * sizeof(NativeType) != (buffer->arrayBufferByteLength() - boffset)) { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS); > + return NULL; // given byte array doesn't map exactly to sizeof(NativeType)*N Spaces around * @@ +1717,3 @@ > } > + } else { > + len = (uint32_t) lengthInt; C++-style cast. @@ +1720,4 @@ > } > > + // Go slowly and check for overflow. > + uint32_t arrayByteLength = len*sizeof(NativeType); Spaces. @@ +1721,5 @@ > > + // Go slowly and check for overflow. > + uint32_t arrayByteLength = len*sizeof(NativeType); > + if (uint32_t(len) >= INT32_MAX / sizeof(NativeType) || > + uint32_t(boffset) >= INT32_MAX - arrayByteLength) No need for the casts here. @@ +1724,5 @@ > + if (uint32_t(len) >= INT32_MAX / sizeof(NativeType) || > + uint32_t(boffset) >= INT32_MAX - arrayByteLength) > + { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS); > + return NULL; // overflow occurred along the way when calculating boffset+len*sizeof(NativeType) Spaces amongst the math, and I'd omit "along the way". @@ +1729,5 @@ > + } > + > + if (arrayByteLength + boffset > buffer->arrayBufferByteLength()) { > + JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_TYPED_ARRAY_BAD_ARGS); > + return NULL; // boffset+len is too big for the arraybuffer Spaces around +. @@ +1759,5 @@ > + static JSObject * > + createTypedArrayWithOffsetLength(JSContext *cx, JSObject *other, > + int32_t byteOffsetInt, int32_t lengthInt) > + { > + JS_ASSERT(!other->isTypedArray()); Erm...how do you reconcile this with the comment that says "the offset and length arguments are ignored if a typedarray is passed in"? @@ +1770,5 @@ > + * Otherwise create a new typed array and copy len properties from > + * the object. > + */ > + return createTypedArrayFromArray(cx, other); > + } There's some else-after-return here that needs removing. @@ +2306,5 @@ > } > > +template<typename ElementType> > +static inline JSObject * > +NewArray(JSContext *cx, int32_t nelements) Should be uint32_t. But why not just don't you have TypedArrayTemplate::internalCreate, say, for this? Then you could have Uint8Array::internalCreate and such, which seems more pleasant than knowing element type stuff. That seems better to me than having this out-of-line thing that has to be qualified with the element type, and requires internal specialization of stuff too. This method should also be called in the obvious place in TypedArrayTemplate::create, since it's duplicating those bits. @@ +2316,5 @@ > +} > + > +template<typename ElementType> > +static inline JSObject * > +NewArrayWithBuffer(JSContext *cx, JSObject *arrayBuffer, int32_t byteoffset, int32_t intLength) Same for this too. @@ +2345,5 @@ > + JS_FRIEND_API(JSBool) JS_Is ## Name ## Array(JSContext *cx, JSObject *obj, JSBool *isArray)\ > + { \ > + obj = UnwrapObject(obj); \ > + Class *clasp = obj->getClass(); \ > + *isArray = (clasp == &TypedArray::fastClasses[TypedArrayTemplate<NativeType>::ArrayTypeID()]); \ We should add |static const Class * const clasp = ...| to the Uint8Array and similar classes at some point, for much-superior readability. Not necessary now, just something to do sometime. @@ +2600,5 @@ > + Uint8ClampedArray::create > + }; > + > +JSObject * > +js::CreateTypedArray(JSContext *cx, uint32_t atype, int32_t nelements) Can't you insert a switch-on-type at the one place in jsclone.cpp where we switch on typed array kind, and then call Uint8Array::internalCreate directly? (This would avoid rather-awkward use of a method that's meant to be used in a native method exposed to script, too.) It seems unnecessary to have the array just for this one single use. @@ +2631,4 @@ > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > return obj->arrayBufferByteLength(); The assertion here is unnecessary assuming the accessor has it too. Remove the one here after ensuring the accessor has the assert. @@ +2639,4 @@ > { > obj = UnwrapObject(obj); > JS_ASSERT(obj->isArrayBuffer()); > return obj->arrayBufferDataOffset(); Same. @@ +2656,1 @@ > } This should be removed. @@ +2665,1 @@ > } This should be removed. @@ +2665,5 @@ > } > > JS_FRIEND_API(JSObject *) > +JS_NewTypedArrayWithBuffer(JSContext *cx, uint32_t atype, JSObject *bufArg, > + int32_t byteoffset, int32_t length) This should be removed. ::: js/src/jstypedarray.h @@ +291,3 @@ > > +JSObject * > +CreateTypedArray(JSContext *cx, uint32_t atype, int32_t nelements); This can be removed, after previous changes. ::: js/src/jstypedarrayinlines.h @@ +69,5 @@ > } > > inline uint32_t > TypedArray::getLength(JSObject *obj) { > + JS_ASSERT(js::IsFastOrSlowTypedArray(obj)); Why are these prefixes necessary? You're inside namespace js, no? Please undo them. ::: js/src/shell/js.cpp @@ +939,5 @@ > size_t len = ftell(file); > if (fseek(file, 0, SEEK_SET) != 0) { > JS_ReportError(cx, "can't seek start of %s", pathname); > } else { > + obj = JS_NewUint8Array(cx, len); So much purtier.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #66) > Comment on attachment 611667 [details] [diff] [review] > Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the > engine > > Review of attachment 611667 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsapi-tests/testTypedArrays.cpp > @@ +57,5 @@ > > +typedef JSObject *(CreationFunc)(JSContext *cx, int32_t nelements); > > + > > +typedef void *(*GetDataFunc)(JSObject *, JSContext*); > > + > > +template<CreationFunc Create, typename Element> > > Why not just include the element-getter function in the template as well (a > template parameter to the template), rather than have this VoidGetData > nonsense? Needs more templates, of course, but it seems a bit better than > casting function pointers. I tried to figure out how to do that and failed. The return type of the getters varies. It's actualy the same as Element, I think, but I can't make one template parameter's type depend on another template parameter. Oh! Yes I can. Ok, you now have this on your conscience: template<JSObject *CreateWithBuffer(JSContext *, JSObject *, int32_t, int32_t), JSObject *CreateFromArray(JSContext *, JSObject *), typename Element, Element *GetData(JSObject *, JSContext *)> bool TestArrayFromBuffer(JSContext *cx) { ... > @@ +59,5 @@ > > +typedef void *(*GetDataFunc)(JSObject *, JSContext*); > > + > > +template<CreationFunc Create, typename Element> > > +bool > > +TestPlainTypedArray(JSContext *cx, VoidGetData getter) > > Put this above the BEGIN_TEST thing, so that it gets seen before the actual > test code that uses it. I'm vaguely surprised this even compiles. Can't. The bodies of these templatized functions use CHECK, which seems to be a class method on the test class implicitly created by BEGIN_TEST. I guess it works to have it after because it's a (templatized) method declaration, and the compiler knows that it has to scan the full class definition before compiling the bodies. > ::: js/src/jsclone.cpp > @@ +677,5 @@ > > bool > > JSStructuredCloneReader::readTypedArray(uint32_t tag, uint32_t nelems, Value *vp) > > { > > uint32_t atype = TagToArrayType(tag); > > + JSObject *obj = CreateTypedArray(context(), atype, nelems); > > See comments on CreateTypedArray in jstypedarray.cpp. Rewritten. > @@ +708,5 @@ > > > > bool > > JSStructuredCloneReader::readArrayBuffer(uint32_t nbytes, Value *vp) > > { > > + JSObject *obj = js::ArrayBuffer::create(context(), nbytes); > > We should be in a using namespace js, so the prefix isn't necessary. (If it > is, add the using.) Fixed. Also removed a js:: from JSStructuredCloneWriter::startWrite(const js::Value &v), just for you. > ::: js/src/jsfriendapi.h > @@ +841,5 @@ > > + TYPE_FLOAT32, > > + TYPE_FLOAT64, > > + > > + /* > > + * Special type that's a uint8, but assignments are clamped to 0 .. 255. > > [0, 256) reads nicer to me. But... but.. unbalanced parentheses! Ok, changed. Also changed uint8 to uint8_t in the comment. > > @@ +856,5 @@ > > +/* > > + * Create a new typed array with nelements elements. > > + */ > > + > > +JS_FRIEND_API(JSObject *) > > Should these be |extern| as well? I'd guess so, but maybe I'm wrong about > that. See what other friend stuff does. Right you are. Fixed. > @@ +857,5 @@ > > + * Create a new typed array with nelements elements. > > + */ > > + > > +JS_FRIEND_API(JSObject *) > > +JS_NewInt8Array(JSContext *cx, int32_t nelements); > > These should be |uint32_t nelements|, since that's the right conceptual type > here. Ok > Since these are all variations on a theme, I wouldn't bother with blank > lines between them -- keep things a little clearer that they're all the > "same", not conceptually distinct. Same for the other blocks of similar > functions. Ok > @@ +927,5 @@ > > + */ > > + > > +JS_FRIEND_API(JSObject *) > > +JS_NewInt8ArrayWithBuffer(JSContext *cx, JSObject *arrayBuffer, > > + int32_t byteoffset, int32_t length); > > Make it |uint32_t length|, please. See the comment -- length == -1 means to use as much of the buffer as possible. Unless that's seems too gross to you. It should really either be a default value or overloaded, but C is such fun. byteoffset seems like it should be uint32_t, though, since you can't have a negative offset from the beginning of an ArrayBuffer. Previously, it accepted -1 to use the default offset of zero, but that seems entirely pointless. > @@ +969,5 @@ > > +JS_FRIEND_API(JSObject *) > > +JS_NewArrayBuffer(JSContext *cx, uint32_t nbytes); > > + > > +/* > > + * Check whether obj supports JS_GetTypedArray* APIs. Note that this may fail > > Note that the method returns an ArrayBufferView::* value. Or, better yet, > change its return type to that. I guess this comment must be for JS_GetTypedArrayType? Hm. That would be C++ only. I made a JSArrayBufferViewType that works for either. Hit me if it's too ugly. > @@ +976,5 @@ > > + * succeeds, then the "Unchecked" versions of APIs may be called on that object > > + * (eg JS_GetTypedArrayTypeUnchecked). > > + */ > > + > > +JS_FRIEND_API(JSBool) > > No blank line here either. (I think I'm not going to bother pointing out > later instances of this, except to say that I could go either way on > removing blank lines when it's before a block of similar methods, like all > the JS_Is*Array methods.) > > @@ +1031,5 @@ > > +/* > > + * Return the available byte length of an array buffer. |obj| must have passed > > + * a JS_IsArrayBufferObject test, or somehow be known that it would pass such a > > + * test: it is an ArrayBuffer or a wrapper of an ArrayBuffer, and the > > + * unwrapping will succeed. If cx is NULL, then DEBUG builds won't assert. > > If the object isn't a proxy, and it's not an ArrayBuffer, we could still > assert even with |cx == NULL|. Right? (Here and in the other methods with > this comment.) Yes, and in the eventual implementation it does. I updated the comment. > Requiring |cx != NULL| seems best, if possible. I don't know if it's > possible to do that or not. If it is, we should. If not, um. I think we I saw very, very few cases where cx was not available. But more than zero. > want to assert if we know there's a mismatch even when we have |cx == NULL|, > even if we won't know all the time. I get that we want users to know that > they'll get mismatch-detection on misuse, so we want a comment that includes > something like it does not. I don't know how to say something, allowing for > assertions sometimes when |cx == NULL|, without getting into too much > overshare. Maybe you can think of something. I've updated it to "If cx is NULL, then DEBUG builds may be unable to check whether unwrapping should fail." Let me know what you think. > > @@ +1083,5 @@ > > +/* > > + * Return a pointer to the start of the data referenced by a typed array. |obj| > > + * must have passed a JS_Is*Array test, or somehow be known that it would pass > > + * such a test: it is a typed array or a wrapper of a typed array, and the > > + * unwrapping will succeed. If cx is NULL, then DEBUG builds won't assert. > > Hmm. If we're handing out raw pointers to mutable data here, probably we > should somehow say when it's safe to modify that data. How about saying you > have to be inside a request to call these methods, and to modify the data in > the pointers returned by them? Requests are so Q1 2012. Runtimes are now single-threaded, so you can mutate the data whenever you want. Well, from that thread. I'll put some scary language in. > > ::: js/src/jstypedarray.cpp > @@ +141,1 @@ > > * first. > > Could you file a followup bug, or deal with it somewhere, to fix this last > sentence? Given the semantics of the method it just doesn't make any sense > -- you can hand in anything and get back something vaguely reasonable here > (even if just NULL to indicate none found, or something). To tell you the truth, I've never understood that comment. I'll fix it in bug 741039 (which is already in your queue, but I'll have to update it for this patch and this comment.) > @@ +1081,5 @@ > > class TypedArrayTemplate > > : public TypedArray > > { > > + template<typename ElementType> > > + friend JSObject *NewTypedArrayFromArray(JSContext *cx, JSObject *other); > > templatized friend declarations? You are more optimistic than I am -- make > sure to try this before landing. :-) I really hope it doesn't break, because I'm going to rip it all right back out in bug 741041 anyway. > > @@ +1703,2 @@ > > { > > + uint32_t boffset = (byteOffsetInt < 0) ? 0 : uint32_t(byteOffsetInt); > > Make the docs say "< 0", not "== -1", then. Updated everything to == -1. > @@ +1708,5 @@ > > + return NULL; // invalid byteOffset > > + } > > + > > + uint32_t len; > > + if (lengthInt < 0) { > > Er, hrm. It wasn't clear to me what "optional" meant in the context of this > argument in the friend functions, then. XXXXXXXXXXXXXXXXrevise that old > comment It's commented in the friend functions. Whether it's a good idea is another question. > @@ +1721,5 @@ > > > > + // Go slowly and check for overflow. > > + uint32_t arrayByteLength = len*sizeof(NativeType); > > + if (uint32_t(len) >= INT32_MAX / sizeof(NativeType) || > > + uint32_t(boffset) >= INT32_MAX - arrayByteLength) > > No need for the casts here. Sure 'nuff. > @@ +1759,5 @@ > > + static JSObject * > > + createTypedArrayWithOffsetLength(JSContext *cx, JSObject *other, > > + int32_t byteOffsetInt, int32_t lengthInt) > > + { > > + JS_ASSERT(!other->isTypedArray()); > > Erm...how do you reconcile this with the comment that says "the offset and > length arguments are ignored if a typedarray is passed in"? s/typedarray/array/ > @@ +2306,5 @@ > > } > > > > +template<typename ElementType> > > +static inline JSObject * > > +NewArray(JSContext *cx, int32_t nelements) > > Should be uint32_t. > > But why not just don't you have TypedArrayTemplate::internalCreate, say, for > this? Then you could have Uint8Array::internalCreate and such, which seems > more pleasant than knowing element type stuff. That seems better to me than > having this out-of-line thing that has to be qualified with the element > type, and requires internal specialization of stuff too. > > This method should also be called in the obvious place in > TypedArrayTemplate::create, since it's duplicating those bits. I rip out half of these functions in bug 741041. I think I end up doing exactly what you're suggesting here. I called it makeInstance instead of internalCreate, but that's a review for another day. Are you ok with this being a followup bug? > @@ +2316,5 @@ > > +} > > + > > +template<typename ElementType> > > +static inline JSObject * > > +NewArrayWithBuffer(JSContext *cx, JSObject *arrayBuffer, int32_t byteoffset, int32_t intLength) > > Same for this too. Yes, this'll die too. > @@ +2345,5 @@ > > + JS_FRIEND_API(JSBool) JS_Is ## Name ## Array(JSContext *cx, JSObject *obj, JSBool *isArray)\ > > + { \ > > + obj = UnwrapObject(obj); \ > > + Class *clasp = obj->getClass(); \ > > + *isArray = (clasp == &TypedArray::fastClasses[TypedArrayTemplate<NativeType>::ArrayTypeID()]); \ > > We should add |static const Class * const clasp = ...| to the Uint8Array and > similar classes at some point, for much-superior readability. Not necessary > now, just something to do sometime. Hm... that's a good idea. > @@ +2600,5 @@ > > + Uint8ClampedArray::create > > + }; > > + > > +JSObject * > > +js::CreateTypedArray(JSContext *cx, uint32_t atype, int32_t nelements) > > Can't you insert a switch-on-type at the one place in jsclone.cpp where we > switch on typed array kind, and then call Uint8Array::internalCreate > directly? (This would avoid rather-awkward use of a method that's meant to > be used in a native method exposed to script, too.) It seems unnecessary to > have the array just for this one single use. Hm. Ok, fair enough. Though I used the friend APIs instead. > @@ +2631,4 @@ > > { > > obj = UnwrapObject(obj); > > JS_ASSERT(obj->isArrayBuffer()); > > return obj->arrayBufferByteLength(); > > The assertion here is unnecessary assuming the accessor has it too. Remove > the one here after ensuring the accessor has the assert. Can I postpone that until the ArrayBufferObject patch? It fixes this. > @@ +2656,1 @@ > > } > > This should be removed. Gone > @@ +2665,1 @@ > > } > > This should be removed. Gone > @@ +2665,5 @@ > > } > > > > JS_FRIEND_API(JSObject *) > > +JS_NewTypedArrayWithBuffer(JSContext *cx, uint32_t atype, JSObject *bufArg, > > + int32_t byteoffset, int32_t length) > > This should be removed. Gone > ::: js/src/jstypedarray.h > @@ +291,3 @@ > > > > +JSObject * > > +CreateTypedArray(JSContext *cx, uint32_t atype, int32_t nelements); > > This can be removed, after previous changes. Gone > ::: js/src/jstypedarrayinlines.h > @@ +69,5 @@ > > } > > > > inline uint32_t > > TypedArray::getLength(JSObject *obj) { > > + JS_ASSERT(js::IsFastOrSlowTypedArray(obj)); > > Why are these prefixes necessary? You're inside namespace js, no? Please > undo them. Sorry, I've moved them back and forth between files a couple of times. Fixed. > ::: js/src/shell/js.cpp > @@ +939,5 @@ > > size_t len = ftell(file); > > if (fseek(file, 0, SEEK_SET) != 0) { > > JS_ReportError(cx, "can't seek start of %s", pathname); > > } else { > > + obj = JS_NewUint8Array(cx, len); > > So much purtier.

Comment on attachment 612013 [details] [diff] [review] Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the engine Review of attachment 612013 [details] [diff] [review]: ----------------------------------------------------------------- > Oh! Yes I can. Ok, you now have this on your conscience: > > template<JSObject *CreateWithBuffer(JSContext *, JSObject *, int32_t, > int32_t), > JSObject *CreateFromArray(JSContext *, JSObject *), > typename Element, > Element *GetData(JSObject *, JSContext *)> > bool > TestArrayFromBuffer(JSContext *cx) > { ... Heh. > byteoffset seems like it should be uint32_t, though, since you can't have a > negative offset from the beginning of an ArrayBuffer. Previously, it accepted > -1 to use the default offset of zero, but that seems entirely pointless. Hm, true. I guess neither of us is thinking things through completely enough, at least not at all times. But BY OUR POWERS COMBINED... > I've updated it to "If cx is NULL, then DEBUG builds may be unable to check > whether unwrapping should fail." Let me know what you think. Well worded. Well, mostly well worded. Why "check whether unwrapping should fail" rather than "assert that the object is a typed array or a wrapper of a typed array", say? I'd expect users to want to know that it's specifically assertion coverage they'll be losing by passing NULL there. > > Er, hrm. It wasn't clear to me what "optional" meant in the context of this > > argument in the friend functions, then. XXXXXXXXXXXXXXXXrevise that old > > comment > > It's commented in the friend functions. Whether it's a good idea is another > question. Hum, I meant to revise this before posting it, didn't I. What I meant was that "optional" meaning "-1 was supplied" wasn't clear to me, and that the interface should say "pass -1 for this special behavior". XXXXXXXXXXXXI'll make sure to check on that when I get to reviewing the patch. > I rip out half of these functions in bug 741041. I think I end up doing exactly > what you're suggesting here. I called it makeInstance instead of > internalCreate, but that's a review for another day. Are you ok with this being > a followup bug? Sounds good. But I insist it be painted green! > Can I postpone that until the ArrayBufferObject patch? It fixes this. Sure. ::: js/src/jsfriendapi.h @@ +851,5 @@ > +}; > + > +} > + > +typedef ArrayBufferView::ViewType JSArrayBufferViewType; Not super-pretty, but a fair solution, if we have to provide C-compatibility. @@ +884,5 @@ > +/* > + * Create a new typed array and copy in values from the given object. The > + * object is used as if it were an array; that is, the new array (if > + * successfully created) will have length given by obj.length, and its elements > + * will be those specified by obj[0], obj[1], and so on, after conversion to s/obj/array/ since you've changed the argument name. @@ +911,5 @@ > +/* > + * Create a new typed array using the given ArrayBuffer for storage. byteOffset > + * must not exceed (signed) INT32_MAX. The length value is optional; if -1 is > + * passed, enough elements to use up the remainder of the byte array is used as > + * the default value. Looks good. ::: js/src/jstypedarray.cpp @@ +141,1 @@ > * first. I have a vague recollection of noting at some point that I don't understand this "Callers should..." line, and you either explaining it or saying it was going to go away. I don't remember which it was. Meh. @@ +1710,5 @@ > + > + uint32_t len; > + if (lengthInt == -1) { > + len = (buffer->arrayBufferByteLength() - boffset) / sizeof(NativeType); > + if (len * sizeof(NativeType) != (buffer->arrayBufferByteLength() - boffset)) { Don't need excess parentheses here. @@ +1717,3 @@ > } > + } else { > + len = static_cast<uint32_t>(lengthInt); Hum, I guess I said C++-style and meant uint32_t(lengthInt), constructor-style and all. But for a cast as innocuous as this, either seems to work. ::: js/src/jstypedarray.h @@ +291,5 @@ > > } // namespace js > > +bool > +IsFastTypedArrayClass(const js::Class *clasp); Hum, shouldn't this be inside namespace js? (And lose the js:: at the same time.)

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #68) > Comment on attachment 612013 [details] [diff] [review] > Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the > engine > > > I've updated it to "If cx is NULL, then DEBUG builds may be unable to check > > whether unwrapping should fail." Let me know what you think. > > Well worded. Well, mostly well worded. Why "check whether unwrapping > should fail" rather than "assert that the object is a typed array or a > wrapper of a typed array", say? I'd expect users to want to know that it's > specifically assertion coverage they'll be losing by passing NULL there. Ok, I agree that "check" does not really tell people that it's an assertion. But your replacement text is inaccurate -- in a DEBUG build, it will *always* "assert that the object is a typed array or a wrapper of a typed array". When cx is NULL, it may not assert if the object is a wrapper of a typed array *but the unwrapping would fail*. Sorry, this is all just an artifact of me mixing things up so that the API is coming in without an implementation. (Given the recent updates, I haven't even written that implementation yet.) It's going to look something like: if (cx) obj = UnwrapObjectChecked(cx, obj); else obj = UnwrapObject(obj); MOZ_ASSERT(obj->isTypedArray()); Well, other than actually checking the result of UnwrapObjectChecked instead of doing a null pointer dereference. :) I'll reword the comment a little to at least refer to assertions. > @@ +141,1 @@ > > * first. > > I have a vague recollection of noting at some point that I don't understand > this "Callers should..." line, and you either explaining it or saying it was > going to go away. I don't remember which it was. Meh. Well, neither. I said I don't entirely understand it either, but I was going to be using it for a slightly different purpose (or at least in different situations), and I would update that patch with an accurate comment. > ::: js/src/jstypedarray.h > @@ +291,5 @@ > > > > } // namespace js > > > > +bool > > +IsFastTypedArrayClass(const js::Class *clasp); > > Hum, shouldn't this be inside namespace js? (And lose the js:: at the same > time.) Er, yeah.

Comment on attachment 611111 [details] [diff] [review] Update typedarray API usage in content/base I'm having a lot of trouble reviewing this without having any idea of what the new APIs actually are or how they work. I can't seem to find them in the patches attached to this bug; am I just being blind?

Comment on attachment 611111 [details] [diff] [review] Update typedarray API usage in content/base >+++ b/content/base/src/nsContentUtils.cpp >+ memcpy(JS_GetArrayBufferDataUnchecked(*aResult), aData.BeginReading(), dataLen); I'm not seeing that API in the api patch itself. Is this now just JS_GetArrayBufferData? >+++ b/content/base/src/nsDOMBlobBuilder.cpp >+ JSBool isBuffer; >+ if (JS_IsArrayBufferObject(aCx, obj, &isBuffer) && isBuffer) >+ return mBlobSet.AppendArrayBuffer(obj); > } Why is it OK to silently press on with an exception on aCx if JS_IsArrayBufferObject returns false? This "checking api may throw" business seems really hard to use properly. :( What are the use cases for it, as opposed to just returning false on failure to unwrap? >+++ b/content/base/src/nsWebSocket.cpp > nsWebSocket::Send(nsIVariant *aData) >+ // Get the JSContext That doesn't look right. I'll give you some JSContext generally unrelated to the one script is running on right now, which seems pretty sketchy. What you should do instead is annotate send() in the IDL with [implicit_jscontext] and then you'll just get a JSContext* as the second argument to Send. >+++ b/content/base/src/nsXMLHttpRequest.cpp >@@ -2673,19 +2674,22 @@ GetRequestBody(nsIVariant* aBody, nsIInp >+ NS_ENSURE_TRUE(JS_IsArrayBufferObject(aCx, obj, &isBuffer)); I'm a little surprised this compiled. It needs a second arg, no? >@@ -2883,21 +2887,25 @@ nsXMLHttpRequest::Send(nsIVariant* aVari >+ nsIScriptContext* sc = GetContextForEventHandlers(&rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ JSContext *cx = sc->GetNativeContext(); This likewise looks wrong. You need to do the implicit_jscontext thing here.

(In reply to Boris Zbarsky (:bz) from comment #71) > Comment on attachment 611111 [details] [diff] [review] > Update typedarray API usage in content/base > > >+++ b/content/base/src/nsContentUtils.cpp > >+ memcpy(JS_GetArrayBufferDataUnchecked(*aResult), aData.BeginReading(), dataLen); > > I'm not seeing that API in the api patch itself. Is this now just > JS_GetArrayBufferData? Yes > >+++ b/content/base/src/nsDOMBlobBuilder.cpp > >+ JSBool isBuffer; > >+ if (JS_IsArrayBufferObject(aCx, obj, &isBuffer) && isBuffer) > >+ return mBlobSet.AppendArrayBuffer(obj); > > } > > Why is it OK to silently press on with an exception on aCx if > JS_IsArrayBufferObject returns false? Yeah, I messed that one up. Fixed. > This "checking api may throw" business seems really hard to use properly. > :( What are the use cases for it, as opposed to just returning false on > failure to unwrap? Have you & bholley resolved this question? I'm no longer sure of the answer. We decided to make all of the other accessor APIs non-throwing but asserting, but the JS_Is* APIs can still throw. > >+++ b/content/base/src/nsWebSocket.cpp > > nsWebSocket::Send(nsIVariant *aData) > >+ // Get the JSContext > > That doesn't look right. I'll give you some JSContext generally unrelated > to the one script is running on right now, which seems pretty sketchy. > > What you should do instead is annotate send() in the IDL with > [implicit_jscontext] and then you'll just get a JSContext* as the second > argument to Send. Ok, I had that before. I reverted to it. > >+++ b/content/base/src/nsXMLHttpRequest.cpp > >@@ -2673,19 +2674,22 @@ GetRequestBody(nsIVariant* aBody, nsIInp > >+ NS_ENSURE_TRUE(JS_IsArrayBufferObject(aCx, obj, &isBuffer)); > > I'm a little surprised this compiled. It needs a second arg, no? Yes. Now I'm not sure what version I posted here. > >@@ -2883,21 +2887,25 @@ nsXMLHttpRequest::Send(nsIVariant* aVari > >+ nsIScriptContext* sc = GetContextForEventHandlers(&rv); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ JSContext *cx = sc->GetNativeContext(); > > This likewise looks wrong. You need to do the implicit_jscontext thing here. Ok. That gets a little messy, though. I end up passing in NULL from some tests.

I won't claim it's pretty, but this gets the dom bindings compiling with the new API.

Waldo - so it turns out that WebGL really does use some typed arrays of varying type in code paths that don't otherwise care about the type. (Right now, they'll be one of Int8Array, Uint8Array, Uint8ClampedArray, or Float32Array.) So here's an additional patch to add in a few calls that work for any ArrayBufferView.

I confess I don't know how to build the stuff in gonk/, so I've never tried compiling this.

Comment on attachment 613748 [details] [diff] [review] Update typedarray JSAPI usage in gonk Argh. API will change again. Cancelling r?.

Comment on attachment 613742 [details] [diff] [review] Add some ArrayBufferView APIs Review of attachment 613742 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jstypedarray.cpp @@ +2251,1 @@ > return NewArray<NativeType>(cx, uint32_t(nelements)); \ Might as well get rid of the unnecessary cast now. @@ +2658,5 @@ > return static_cast<double *>(TypedArray::getDataOffset(obj)); > } > + > +JS_FRIEND_API(void *) > +JS_GetArrayBufferViewData(JSObject *obj, JSContext *cx) I remain ambivalent about this API. It seems a footgun likely to be used by people who don't realize a better API is available. If the gonk code is the only potential user for this, I'd really rather we just changed that code to be typesafe, somehow. But I guess I don't know what that way is at the moment, and this typed array stuff has been a pretty long trail of tears already, so let's punt for now and just go with this.

Comment on attachment 613748 [details] [diff] [review] Update typedarray JSAPI usage in gonk I can verify whether this patch works. Do I need to apply any other patches besides this one?

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #77) > Comment on attachment 613742 [details] [diff] [review] > Add some ArrayBufferView APIs > > Review of attachment 613742 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jstypedarray.cpp > @@ +2251,1 @@ > > return NewArray<NativeType>(cx, uint32_t(nelements)); \ > > Might as well get rid of the unnecessary cast now. Ok > @@ +2658,5 @@ > > return static_cast<double *>(TypedArray::getDataOffset(obj)); > > } > > + > > +JS_FRIEND_API(void *) > > +JS_GetArrayBufferViewData(JSObject *obj, JSContext *cx) > > I remain ambivalent about this API. It seems a footgun likely to be used by > people who don't realize a better API is available. If the gonk code is the > only potential user for this, I'd really rather we just changed that code to > be typesafe, somehow. But I guess I don't know what that way is at the > moment, and this typed array stuff has been a pretty long trail of tears > already, so let's punt for now and just go with this. No, it's way more than gonk. The original user I found was WebGL, which is passing enum-discriminated buffers to GL (that's unavoidable with OpenGL). But there turned out to be 3 or 4 places where I was doing something awkward because it allowed any of several different typed arrays. Some of those might be doable with something like JS_GetSingleByteTypedArrayData, but... no. (A couple of places are ok with any of Int8, Uint8, Uint8Clamped.) I put a little bit of a scare comment in jsfriendapi.h, but I was hoping the void* return value would be enough to make most users think twice.

You know, thinking about it, I think part of the problem is that people are passing around typed arrays and using them as arraybuffers. That's legitimate if you want a substring of the arraybuffer (though soon a DataView would probably make more sense.) But I think some of those users never actually access the data through the typed array; it's just used as a vehicle for passing it around. Then again, I'm never sure whether I should be considering ArrayBuffers to be annoying implementation details or not.

Why the void*? When web specs use ArrayBufferView arguments (and a _bunch_ of them are doing that, not just WebGL), they just want the actual byte array underlying the view, no matter what the view happens to be. So it would make sense to return a uint8_t*, to me.

This patch applies on top of the JSAPI update. After discussion with bz and bholley, it seems like there's no need for an API that sets an exception when an unwrapping is denied, and it substantially complicates the code to handle it. This patch makes JS_Is* infallible, though it still needs a JSContext* argument to check whether unwrapping is allowed.

(In reply to Philipp von Weitershausen [:philikon] from comment #78) > Comment on attachment 613748 [details] [diff] [review] > Update typedarray JSAPI usage in gonk > > I can verify whether this patch works. Do I need to apply any other patches > besides this one? Yes, there are 3 patches leading up to this. I guess I should go through and number them all again. They are currently first in the list: "Update JSAPI for typed arrays, remove uses of jstypedarray.h outside the engine", "Add some ArrayBufferView APIs", and "Make JS_IsTypedArrayObject, JS_IsArrayBufferObject, etc. infallible". Or just look for everything reviewed by jwalden+bmo. But I canceled the review for now because I made another JSAPI update, and wanted to wait until the JSAPI was finalized (again) before burning more of anyone else's time. Thanks!

Comment on attachment 614108 [details] [diff] [review] Make JS_IsTypedArrayObject, JS_IsArrayBufferObject, etc. infallible Review of attachment 614108 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testTypedArrays.cpp @@ +28,3 @@ > > JSObject *proto = JS_GetPrototype(buffer); > + CHECK(JS_IsArrayBufferObject(proto, cx)); ! surely? @@ +31,2 @@ > JSObject *dummy = JS_GetParent(proto); > + CHECK(JS_IsArrayBufferObject(dummy, cx)); id. @@ +59,2 @@ > JSObject *proto = JS_GetPrototype(array); > + CHECK(JS_IsTypedArrayObject(proto, cx)); id. @@ +61,2 @@ > JSObject *dummy = JS_GetParent(proto); > + CHECK(JS_IsTypedArrayObject(dummy, cx)); id. ::: js/src/jsfriendapi.h @@ +958,5 @@ > + * Check whether obj supports JS_GetTypedArray* APIs. Note that this may return > + * false if a security wrapper is encountered that denies the unwrapping. If > + * this test or one of the JS_Is*Array tests succeeds, then it is safe to call > + * the various accessor JSAPI calls defined below. cx MUST be non-NULL and > + * valid. I wouldn't bother with the last sentence -- anyone using this should be assuming that, only being surprised if the opposite were true. @@ +1001,5 @@ > /* > + * Check whether obj supports the JS_GetArrayBuffer* APIs. Note that this may > + * return false if a security wrapper is encountered that denies the > + * unwrapping. If this test succeeds, then it is safe to call the various > + * accessor JSAPI calls defined below. cx MUST be non-NULL and valid. Same for the last sentence here.

sfink, I would appreciate a rollup of the API patches before you ask to review the consumers.

Awesome! Thanks for the quick review. (In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #91) > Comment on attachment 614108 [details] [diff] [review] > Make JS_IsTypedArrayObject, JS_IsArrayBufferObject, etc. infallible > > Review of attachment 614108 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsapi-tests/testTypedArrays.cpp > @@ +28,3 @@ > > > > JSObject *proto = JS_GetPrototype(buffer); > > + CHECK(JS_IsArrayBufferObject(proto, cx)); > > ! surely? No. Where by "no", I mean "yes". Y'know, after writing all those tests, you'd think I'd bother to run them. > ::: js/src/jsfriendapi.h > @@ +958,5 @@ > > + * Check whether obj supports JS_GetTypedArray* APIs. Note that this may return > > + * false if a security wrapper is encountered that denies the unwrapping. If > > + * this test or one of the JS_Is*Array tests succeeds, then it is safe to call > > + * the various accessor JSAPI calls defined below. cx MUST be non-NULL and > > + * valid. > > I wouldn't bother with the last sentence -- anyone using this should be > assuming that, only being surprised if the opposite were true. Ok

Rollup patch of typed array JSAPI changes. Carrying over Waldo's r+ from the 3 constituent patches.

(In reply to Ms2ger from comment #92) > sfink, I would appreciate a rollup of the API patches before you ask to > review the consumers. Done. Time for r? spam. Though the latest API is a much more straightforward change.

Comment on attachment 614111 [details] [diff] [review] Update typedarray JSAPI usage in XPConnect [ Updated to latest API. I believe I have also addressed all of your prior review comments.

Comment on attachment 614116 [details] [diff] [review] Update typedarray JSAPI use in content/base [ It might have been better to fold together this patch and the dom and dom bindings patches. The IDL updates here propagate to there.

Comment on attachment 614127 [details] [diff] [review] Update typedarray JSAPI usage in gonk For the JSAPI changes, you now only need the JSAPI patch https://bugzilla.mozilla.org/attachment.cgi?id=614432 But the whole tree won't compile without applying all of the other patches here. I don't know what parts you compile when you compile gonk.

Comment on attachment 614121 [details] [diff] [review] Update typedarray JSAPI usage in content/canvas [ Sorry for the re-review request, but the API has mutated a fair amount. It is fully documented now, at least.

Comment on attachment 614125 [details] [diff] [review] Update typedarray API usage in dom/. Not sure of the proper reviewer here. The only piece of substance is the AutoSafeJSContext move, though.

Comment on attachment 614143 [details] [diff] [review] Update typedarray JSAPI usage in audio [ Review of attachment 614143 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLAudioElement.cpp @@ +182,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsHTMLAudioElement::MozWriteAudio(const JS::Value &aData, JSContext *aCx, PRUint32 *aRetVal) Can you move the & and * to the left here too? @@ +221,5 @@ > > // Don't write more than can be written without blocking. > PRUint32 writeLen = NS_MIN(mAudioStream->Available(), dataLength / mChannels); > > + nsresult rv = mAudioStream->Write(JS_GetArrayBufferViewData(tsrc, aCx), writeLen); Why not use JS_GetFloat32ArrayData here?

(In reply to Ms2ger from comment #101) > Comment on attachment 614143 [details] [diff] [review] > Update typedarray JSAPI usage in audio [ > > Review of attachment 614143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/html/content/src/nsHTMLAudioElement.cpp > @@ +182,5 @@ > > return NS_OK; > > } > > > > NS_IMETHODIMP > > +nsHTMLAudioElement::MozWriteAudio(const JS::Value &aData, JSContext *aCx, PRUint32 *aRetVal) > > Can you move the & and * to the left here too? Ok. There's still a mixture of styles in the file, but I'll do my bit to not make it worse. > @@ +221,5 @@ > > > > // Don't write more than can be written without blocking. > > PRUint32 writeLen = NS_MIN(mAudioStream->Available(), dataLength / mChannels); > > > > + nsresult rv = mAudioStream->Write(JS_GetArrayBufferViewData(tsrc, aCx), writeLen); > > Why not use JS_GetFloat32ArrayData here? Because I'm an idiot. Any further questions?

Comment on attachment 614111 [details] [diff] [review] Update typedarray JSAPI usage in XPConnect [ >- memcpy(*output, JS_GetTypedArrayData(tArr), byteSize); >+ memcpy(*output, JS_GetArrayBufferViewData(tArr, cx), typeSize); Why did byteSize become typeSize here?

(In reply to Bobby Holley (:bholley) from comment #103) > Comment on attachment 614111 [details] [diff] [review] > Update typedarray JSAPI usage in XPConnect [ > > > >- memcpy(*output, JS_GetTypedArrayData(tArr), byteSize); > >+ memcpy(*output, JS_GetArrayBufferViewData(tArr, cx), typeSize); > > Why did byteSize become typeSize here? Because I'm an idiot, and I had a larger change from which I copied and pasted it, except in retrospect it was probably wrong there too. Fixed in my copy.

Comment on attachment 614116 [details] [diff] [review] Update typedarray JSAPI use in content/base [ In nsWebSocket::GetSendParams, please use static_cast or reinterpret_cast, whichever one compiles, instead of the C-style cast. These changes should land in a single changeset with the dom/ changes, at the very least, since neither one compiles without the other... r=me with the above nit.

Comment on attachment 614125 [details] [diff] [review] Update typedarray API usage in dom/. This should really get review from bent.

Comment on attachment 614136 [details] [diff] [review] Update typedarray JSAPI usage in DOM bindings >--- a/dom/bindings/Codegen.py >+++ b/dom/bindings/Codegen.py >@@ -1184,19 +1184,19 @@ def getArgumentConversionTemplate(type, >- " }") >+ " }\n") That's wrong; please undo it. >+++ b/dom/webidl/XMLHttpRequest.webidl All the changes to this file are, afaict, unneccessary and should be reverted. r=me with the above issues addressed.

Comment on attachment 614111 [details] [diff] [review] Update typedarray JSAPI usage in XPConnect [ (In reply to Steve Fink [:sfink] from comment #104) > (In reply to Bobby Holley (:bholley) from comment #103) > > Why did byteSize become typeSize here? > > Because I'm an idiot, and I had a larger change from which I copied and > pasted it, except in retrospect it was probably wrong there too. > > Fixed in my copy. Ok. r=bholley with that fixed.

Then again, here's a slightly improved version of the core patch. It adds the feature that it no longer seg faults a debug build whenever you try to create a typed array of any kind. (All JS tests pass with this version)

(In reply to Boris Zbarsky (:bz) from comment #107) > Comment on attachment 614136 [details] [diff] [review] > Update typedarray JSAPI usage in DOM bindings > > >--- a/dom/bindings/Codegen.py > >+++ b/dom/bindings/Codegen.py > >@@ -1184,19 +1184,19 @@ def getArgumentConversionTemplate(type, > >- " }") > >+ " }\n") > > That's wrong; please undo it. Ok > >+++ b/dom/webidl/XMLHttpRequest.webidl > > All the changes to this file are, afaict, unneccessary and should be > reverted. Ok. It looked to me like that was the intended direction of the webidl stuff, so I left it there as documentation after I discovered it didn't actually do anything. But I'm happy to throw it out.

(In reply to Boris Zbarsky (:bz) from comment #105) > Comment on attachment 614116 [details] [diff] [review] > Update typedarray JSAPI use in content/base [ > > These changes should land in a single changeset with the dom/ changes, at > the very least, since neither one compiles without the other... All of these patches need to be merged into a single changeset for landing, since the JSAPI changes break compilation everywhere.

Is this about ready to land? Once it does, I think c-p-g is a go. But I'm guessing this will happen after aurora uplift.

webgl does indeed have comprehensive tests, and I was failing some. This updated patch still fails 2 tests locally *with or without* these changes. (Just figured that part out!) I brought up the first on #gfx, and was told to ignore it because X is weird. The second is mysterious; it sets a uniform's value to 3 with no error, then retrieves a zero (again with no error). But tbpl just came up all green, so I will now proceed to ignore them both.

Another bogus assertion fix.

(In reply to Steve Fink [:sfink] from comment #113) > The second is mysterious; it sets a > uniform's value to 3 with no error, then retrieves a zero (again with no > error). That's a known bug in the NVIDIA driver.

Shoot, sorry, I noticed I hadn't updated in a while, and the rebase wasn't completely trivial for webgl stuff. Here's a new version. It passes all but those same 2 problematic tests locally. Is there any way in a mochitest to ask if the vendor is nvidia?

philikon -- here's a fully rolled-up patch that you can apply and try out the gonk stuff with. It includes everything (and resembles what I'll end up landing, since I can't land anything separately.) % hg show --stat content/base/public/nsIWebSocket.idl | 4 +- content/base/public/nsIXMLHttpRequest.idl | 6 +- content/base/src/nsContentUtils.cpp | 9 +- content/base/src/nsDOMBlobBuilder.cpp | 20 +- content/base/src/nsDOMBlobBuilder.h | 2 +- content/base/src/nsDOMFileReader.cpp | 10 +- content/base/src/nsWebSocket.cpp | 15 +- content/base/src/nsWebSocket.h | 3 +- content/base/src/nsXMLHttpRequest.cpp | 38 +- content/base/src/nsXMLHttpRequest.h | 45 +- content/base/test/TestGetURL.cpp | 2 +- content/base/test/TestNativeXMLHttpRequest.cpp | 2 +- content/canvas/src/CustomQS_Canvas2D.h | 22 +- content/canvas/src/CustomQS_WebGL.h | 24 +- content/canvas/src/WebGLContext.h | 10 +- content/canvas/src/WebGLContextGL.cpp | 202 ++-- content/canvas/src/WebGLContextValidate.cpp | 10 +- content/canvas/src/nsCanvasRenderingContext2D.cpp | 7 +- content/canvas/src/nsCanvasRenderingContext2DAzure.cpp | 7 +- content/events/src/nsDOMNotifyAudioAvailableEvent.cpp | 9 +- content/html/content/src/nsHTMLAudioElement.cpp | 26 +- content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp | 2 +- dom/bindings/Bindings.conf | 2 +- dom/bindings/Codegen.py | 6 +- dom/bindings/Utils.h | 4 +- dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl | 18 +- dom/system/gonk/SystemWorkerManager.cpp | 21 +- dom/workers/FileReaderSync.cpp | 8 +- dom/workers/XMLHttpRequest.cpp | 10 +- js/src/Makefile.in | 1 - js/src/jsapi-tests/Makefile.in | 1 + js/src/jsapi-tests/testTypedArrays.cpp | 155 +++ js/src/jsclone.cpp | 126 +- js/src/jsfriendapi.h | 276 ++++++ js/src/jsinterpinlines.h | 2 +- js/src/jstypedarray.cpp | 435 ++++++---- js/src/jstypedarray.h | 82 +- js/src/methodjit/PolyIC.cpp | 4 +- js/src/shell/js.cpp | 13 +- js/xpconnect/src/XPCConvert.cpp | 51 +- 40 files changed, 1079 insertions(+), 611 deletions(-)

(In reply to Steve Fink [:sfink] from comment #116) > Created attachment 616331 [details] [diff] [review] > Update typed array JSAPI for content/canvas > > Shoot, sorry, I noticed I hadn't updated in a while, and the rebase wasn't > completely trivial for webgl stuff. I was afraid it would be so, because of bug 732233. But we really had to fix that security and conformance bug. > Here's a new version. It passes all but > those same 2 problematic tests locally. > > Is there any way in a mochitest to ask if the vendor is nvidia? All our test slaves have NVIDIA cards. So if a test fails on NVIDIA, just add it to content/canvas/test/webgl/failing_tests_(win|mac|linux).txt, and when running the test locally on non-NVIDIA hardware, ignore the corresponding unexpected-pass. If a test fails intermittently, edit the mochitest and add it to testsToIgnore. If you really had to detect NVIDIA in a mochitest, since this is privileged JS, you could use nsIGfxInfo as is done in the about:support page: toolkit/content/aboutSupport.js. Here's some stuff typed on the JS prompt in the Web Console: [08:55:14.523] var Cc = Components.classes; [08:55:14.530] undefined -- [08:55:23.826] var Ci = Components.interfaces; [08:55:23.832] undefined -- [08:56:10.141] var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo) [08:56:10.150] undefined -- [08:56:22.254] gfxInfo.getWebGLParameter("full-renderer"); [08:56:22.268] "X.Org -- Gallium 0.4 on AMD RV710 -- 2.1 Mesa 7.11"

I'm trying to actually use the attached API, and I ran into one problem: I want to have a type called ArrayBufferView for my purposes, but the attached patch defines a namespace with that name, outside namespace js. Would it make sense to put the ViewType enum in js::ArrayBufferView instead of a toplevel namespace?

bjacob: you can see the new API at http://people.mozilla.org/~sfink/data/tajsapi.html

(In reply to Boris Zbarsky (:bz) from comment #119) > I'm trying to actually use the attached API, and I ran into one problem: I > want to have a type called ArrayBufferView for my purposes, but the attached > patch defines a namespace with that name, outside namespace js. Would it > make sense to put the ViewType enum in js::ArrayBufferView instead of a > toplevel namespace? I did what? Oops, sorry. I wonder if I can blame merge problems, since some of that file is already inside namespace js... nope. I just messed up. Yes, of course. I'll make the change before landing.

Comment on attachment 614127 [details] [diff] [review] Update typedarray JSAPI usage in gonk I can't assess the correctness of this patch beyond applying common sense to what I see. sfink says the B2G build is green on the try build, so r(ubberstamp)+ from me.

Comment on attachment 616331 [details] [diff] [review] Update typed array JSAPI for content/canvas Review of attachment 616331 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but please have a look at the nits below. ::: content/canvas/src/WebGLContextGL.cpp @@ +4100,3 @@ > static JSObject* > GetTypedArray(JSContext* aCx, const JS::Value& aValue) > { I really don't like templating on function pointers. Please template on the function itself, not on the function pointer. Even better would be if there existed a templated version of JS_IsFloat32Array to which you could forward an integer or typename template parameter. @@ +4143,5 @@ > NS_IMETHODIMP \ > WebGLContext::name(nsIWebGLUniformLocation *aLocation, const JS::Value& aValue, \ > JSContext* aCx) \ > { \ > + JSObject* wa = GetTypedArray<JS_Is ## arrayType ## Array, JS_New ## arrayType ## ArrayFromArray>(aCx, aValue); \ Ouch. Yeah, there was not much else to do until we get rid of these macros. @@ +5234,5 @@ > if (!IsContextStable()) > return NS_OK; > > + if (!JS_IsTypedArrayObject(pixels, cx)) > + return ErrorInvalidValue("TexSubImage2D: pixels are wrong type!"); I don't think that that if() condition can ever be met, since TexImage2D_imageData is not exposed to scripts, it's something that we manually call on the |data| of a ImageData, which is a typed array object. So maybe this should rather be a NS_ABORT_IF_FALSE, if it makes you more comfortable to be checking this? @@ +5405,5 @@ > if (!pixels) > return ErrorInvalidValue("TexSubImage2D: pixels must not be null!"); > > + if (!JS_IsTypedArrayObject(pixels, cx)) > + return ErrorInvalidValue("TexSubImage2D: pixels are wrong type!"); Same comment as above for TexImage2D_imageData. @@ +5428,5 @@ > if (!pixels) > return ErrorInvalidValue("TexSubImage2D: pixels must not be null!"); > > + if (!JS_IsTypedArrayObject(pixels, cx)) > + return ErrorInvalidValue("TexSubImage2D: pixels are wrong type!"); Same comment as above for TexImage2D_imageData. ::: content/canvas/src/WebGLContextValidate.cpp @@ +409,2 @@ > { > ErrorInvalidOperation("%s: invalid typed array type for given format", info); This error message is wrong, can you correct it while you're at it? It should say "%s: invalid typed array type for given texture data type"

(In reply to Benoit Jacob [:bjacob] from comment #123) > Comment on attachment 616331 [details] [diff] [review] > Update typed array JSAPI for content/canvas > > Review of attachment 616331 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, but please have a look at the nits below. > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +4100,3 @@ > > static JSObject* > > GetTypedArray(JSContext* aCx, const JS::Value& aValue) > > { > > I really don't like templating on function pointers. Please template on the > function itself, not on the function pointer. Ok, fixed. > Even better would be if there existed a templated version of > JS_IsFloat32Array to which you could forward an integer or typename template > parameter. Yeah, I think bz wants that too and may have implemented it. Looks like we'll probably grow a bunch of C++-only entry points. But I'll leave that for a followup. > @@ +4143,5 @@ > > NS_IMETHODIMP \ > > WebGLContext::name(nsIWebGLUniformLocation *aLocation, const JS::Value& aValue, \ > > JSContext* aCx) \ > > { \ > > + JSObject* wa = GetTypedArray<JS_Is ## arrayType ## Array, JS_New ## arrayType ## ArrayFromArray>(aCx, aValue); \ > > Ouch. Yeah, there was not much else to do until we get rid of these macros. This is the clean version, too. You should've seen it when JS_Is* could throw an exception. Then again, maybe you did. > @@ +5234,5 @@ > > if (!IsContextStable()) > > return NS_OK; > > > > + if (!JS_IsTypedArrayObject(pixels, cx)) > > + return ErrorInvalidValue("TexSubImage2D: pixels are wrong type!"); > > I don't think that that if() condition can ever be met, since > TexImage2D_imageData is not exposed to scripts, it's something that we > manually call on the |data| of a ImageData, which is a typed array object. Ok. There's no way to set the data to something you're not allowed to unwrap? > So maybe this should rather be a NS_ABORT_IF_FALSE, if it makes you more > comfortable to be checking this? Yes, it's not obvious to me as a naive reader of the code, so I'd rather maintain the invariant that you don't call the other APIs without checking JS_Is* first (or at least, checking it in a debug build.) I switched all these to NS_ABORT_IF_FALSE. > @@ +409,2 @@ > > { > > ErrorInvalidOperation("%s: invalid typed array type for given format", info); > > This error message is wrong, can you correct it while you're at it? It > should say "%s: invalid typed array type for given texture data type" Ok. I changed a couple nearby instances of the same error that were all based on the type.

Comment on attachment 617056 [details] [diff] [review] Update JSAPI for typed arrays, remove jstypedarray.h from exported includes Argh. Accidentally hit Enter too early. This is a rollup of the fully r+ version. [Approval Request Comment] Regression caused by (bug #): 737245 User impact if declined: bug 743000 Testing completed (on m-c, etc.): This is nearly identical to what has passed try already. This rollup contains a couple of minor review changes on top of that, and I have pushed it to try. Risk to taking this patch (and alternatives if risky): This mostly changes the exposed API for using typed arrays. Typed arrays are not very heavily used yet, mostly in canvas and webgl, and there are fairly exhaustive tests in those areas. The changes to handle this new API are largely comprised of just passing around a JSContext pointer and using more type-specific functions. This patch adds new ways for eg JS_IsTypedArrayObject to fail in the future (by making use of the JSContext to do security checks), but does not actually add any additional failure modes yet. (A followup patch will add those, but it's not necessary for fixing bug 743000 so I may not request approval for it.) If this patch is not taken, then an alternative fix for bug 743000 will be needed. String changes made by this patch: none

> Even better would be if there existed a templated version of JS_IsFloat32Array to which > you could forward an integer or typename template parameter. For what it's worth, this code will pretty much go away with the new bindings, so I wouldn't worry too much about it.

Comment on attachment 617056 [details] [diff] [review] Update JSAPI for typed arrays, remove jstypedarray.h from exported includes Whoops, sorry. Aurora is unaffected. I need approval for m-c.

Comment on attachment 617056 [details] [diff] [review] Update JSAPI for typed arrays, remove jstypedarray.h from exported includes let's wait for 15

> let's wait for 15 I've been fuzzing with this patch enabled simply because without the patch, there is too much noise from js bugs related to typedarrays. These js bugs, of which bug 743000 is one example of a bad bug, as well as a few others which I didn't file because they are fixed in this patch, will definitely affect the stability of Mobile if left unfixed by this patch not landing. In addition, note that an alternative fix for Aurora will also be needed for bug 743000 in that case. Re-requesting approval-mozilla-central?.

I see mention of Try builds, but I don't see any links. I'm worried by the shear size of this patch and the number of files touched. The risk assessment in comment 126 makes it seem like the scope is mall and that we have enough testing to find regressions. If this patch is the only way forward to fix the typedarray issues, we could probably let it in.

> I see mention of Try builds, but I don't see any links. Try results: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/sfink@mozilla.com-b2ffaa98058b/ http://hg.mozilla.org/try/pushloghtml?changeset=b2ffaa98058b https://tbpl.mozilla.org/?tree=Try&rev=b2ffaa98058b

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #132) > > I see mention of Try builds, but I don't see any links. > > Try results: > https://tbpl.mozilla.org/?tree=Try&rev=b2ffaa98058b Thanks for the link. I looked over the results and found no mobile failures or talos regressions in that run. Given that and the nature of the bugs fixed, I am approving.

> Comment on attachment 617056 [details] [diff] [review] > Update JSAPI for typed arrays, remove jstypedarray.h from exported includes sfink, this needs to be rebased a little prior to landing on mozilla-inbound - there's a little bit of bitrot. $ hg qpush applying bug-711843-typedarray-jsapi patching file content/canvas/src/WebGLContextGL.cpp Hunk #5 FAILED at 565 1 out of 19 hunks FAILED -- saving rejects to file content/canvas/src/WebGLContextGL.cpp.rej patching file content/canvas/src/WebGLContextValidate.cpp Hunk #1 succeeded at 39 with fuzz 1 (offset 0 lines). patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh bug-711843-typedarray-jsapi

Looks like it was cosmetic. hg pull --rebase fixed it automatically, and both eyeballing the changes and building in content/canvas both looked good. http://mozilla.hg.mozilla.org/integration/mozilla-inbound/rev/7a601537cb88

> http://mozilla.hg.mozilla.org/integration/mozilla-inbound/rev/7a601537cb88 Corrected: http://hg.mozilla.org/integration/mozilla-inbound/rev/7a601537cb88