672266 - add TypedArray support to nsIVariant

Status: RESOLVED WONTFIX

(Core :: XPConnect, enhancement)

Description

[Cellier fabien [:fcellier]]

Having XPCOM array from TypedArray is not possible right now(wihtout quicstubs). 

we can't write :

idl:
#include "nsISupports.idl"
[scriptable, uuid(cd247fc5-472e-4ee9-9bf2-15a22ec9a08a)]
interface nsITest : nsISupports
{ void test(in nsIVariant array);};

js:
var int32View = new Int32Array(buffer);
mITest.test(int32View);

Comment 1

[Cellier fabien [:fcellier]]

Attachment: add typedArray

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It supports jsvals doesn't it?

Comment 3

[Cellier fabien [:fcellier]]

(In reply to comment #2)
> It supports jsvals doesn't it?

If someone is in gecko, there is no real problem. It's not as simple as it could be (call xpconnect and nsIThreadJSContextStack etc..) and I thought it was nsIvariant purpose to help with that stuff. 

Futhermore if somebody make an extension, it can't access to typedArray (error in linkage), am I wrong?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 546564 [details] [diff] [review]
add typedArray

Hi Fabien,

This patch looks pretty good. I did just land a couple of changes that changed the API a little (see bug 664249). I do have a couple of nits, though. Would you mind attaching a second patch addressing them?

The PRSize variable shouldn't be named mSize as the m prefix indicates a member of a class. size would work better.

Since XPConnect has the ugliest coding style on earth, please delete the spaces after "if (" and "switch (".

Case TYPE_UINT8 appears to be misindented.

Thanks a lot for your patch!

Comment 5

[Cellier fabien [:fcellier]]

Attachment: add typedArray in variant (JS -> C++)

Sorry for the delay. Some corrections for the patch ( indentation and new TypedArray API etc)

Comment 6

[Cellier fabien [:fcellier]]

Attachment: add typedArray in xpcconvert (JS <-> C++)

With this patch, nsIWritableVariant will use typedArray for the js representation (I don't know if it's a good idea but perf are better for big arrays :) .

Some parts are very ugly and I must find a way to change that.

In fact, 2 things really hurt my eyes:
 Some repetitions in XPCVariant and xpcconvert
 macro problems with AUTO_MARK_JSVAL and the ugly solution I wrote(perhaps it's better to replace macro by class helper with templates)


If I have time, I'll fix that soon. I attach the patch just in case

Comment 7

[Cellier fabien [:fcellier]]

Attachment: add typedArray in xpcconvert (JS <-> C++)

sorry bad patch

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 551595 [details] [diff] [review]
add typedArray in variant (JS -> C++)

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

Looks good. One small comment to address. r=me with it.

::: js/src/xpconnect/src/xpcvariant.cpp
@@ +405,5 @@
> +    
> +    if(js_IsTypedArray(jsobj))
> +    {
> +        JSObject* typedArray = js::TypedArray::getTypedArray(jsobj);
> +        if(typedArray)

I don't think this getTypedArray call or the null check are needed after js_IsTypedArray returns true.

Comment 9

[Cellier fabien [:fcellier]]

Attachment: add typedArray in variant (JS -> C++)

Comment 10

[Cellier fabien [:fcellier]]

It was done in XPCConvert. Don't need it anymore