526957 - WebGL arrays need indexed property access

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Mark Steele]

Attachment: implement nsIXPCScriptable for array index access

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 410726 [details] [diff] [review]
implement nsIXPCScriptable for array index access

Asking Blake for review here when he gets a chance -- he knows nsIXPCScriptable a lot better than I do

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 410726 [details] [diff] [review]
implement nsIXPCScriptable for array index access

Blake's in a weird timezone; this looks fine to me, but would still like him to take a look after the fact.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 410726 [details] [diff] [review]
implement nsIXPCScriptable for array index access

diff -r 700b6d0b5b43 content/canvas/src/WebGLArrays.cpp
+PRBool WebGLFloatArray::JSValToIndex(JSContext *cx, jsval id, PRUint32 *retval) {

Nit: This brace wants to go on the next line.

+    if (JSVAL_IS_INT(id)) {
+        index = JSVAL_TO_INT(id);
+        ok = PR_TRUE;
+    } else {
+        ok = JS_ValueToECMAUint32(cx, id, &index);
+    }
+
+    if (!ok || index >= mLength)
+        return PR_FALSE;

This is a little weird for a couple of reasons. First, this function can return false having either set an exception or not, which makes it hard for callers to know if they have to throw. Second, if index >= mLength, returning false will cause the callers to throw, which means that |42 in myShortArray| will throw. IMO, it'd be better to return true here and let myShortArray[42] return undefined.

+NS_IMETHODIMP WebGLFloatArray::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
+                                           JSObject * obj, jsval id, jsval * vp, PRBool *_retval)
+{
+    PRUint32 index;
+    

Uber-nit: trailing white space here.

+
+    float val;
+    Get(index, &val);
+    *_retval = JS_NewNumberValue(cx, val, vp);
+
+    return NS_OK;
+}
+
+NS_IMETHODIMP WebGLFloatArray::SetProperty(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
+                       JSObject * obj, jsval id, jsval * vp, PRBool *_retval)
+{
+    if (!JSValToIndex(cx, id, &index)) {
+        *_retval = PR_FALSE;
+        return NS_ERROR_INVALID_ARG;
+    }

Unlike get, this might want to throw if index is too big, but I'm not sure (how much are these things supposed to behave like regular JS arrays?).

+    if (JSVAL_IS_INT(*vp)) {
+        val = JSVAL_TO_INT(*vp);
+        ok = PR_TRUE;
+    } else {
+        jsdouble dval;
+        ok = JS_ValueToNumber(cx, *vp, &dval);
+        val = dval;
+    }

JS_ValueToNumber does the JSVAL_IS_INT check for you.

+NS_IMETHODIMP WebGLFloatArray::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
+                      JSObject * obj, jsval id, PRUint32 flags, JSObject * *objp,
+                      PRBool *_retval)
+{
+    PRUint32 index;
+    PRBool ok = JSValToIndex(cx, id, &index);

I'd rather see:

if (!JSValToIndex(cx, id, &index)) {
    *_retval = PR_FALSE;
    return NS_ERROR_FAILURE;
}

to avoid the if/else.

Also, you need to actually define your property on the object here. The way this is written now, getting will work (because we will eventually call your getProperty hook), but things like |0 in array| won't. So, you probably want something like:

JS_DefineElement(cx, obj, index, JSVAL_VOID, NULL, NULL, JSPROP_SHARED);

I counted 5 or 6 additional copies of this code (I assume the types changed a little, but didn't check). Is there no way that this code can be shared? Either way, my comments stand for all of them.

diff -r 700b6d0b5b43 content/canvas/src/WebGLArrays.h
@@ -80,7 +81,13 @@
                           jsval* aArgv);
 };
 
+class WebGLArray
+{
+    
+};

What's this for?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(Hm, the Cc list somehow got nuked)

(In reply to comment #4)
> +    if (JSVAL_IS_INT(*vp)) {
> +        val = JSVAL_TO_INT(*vp);
> +        ok = PR_TRUE;
> +    } else {
> +        jsdouble dval;
> +        ok = JS_ValueToNumber(cx, *vp, &dval);
> +        val = dval;
> +    }
> 
> JS_ValueToNumber does the JSVAL_IS_INT check for you.

But at the expense of a function call.. JSVAL_IS_INT is going to be the by-far common case, so seems worth it to do this like so.


> Also, you need to actually define your property on the object here. The way
> this is written now, getting will work (because we will eventually call your
> getProperty hook), but things like |0 in array| won't. So, you probably want
> something like:
> 
> JS_DefineElement(cx, obj, index, JSVAL_VOID, NULL, NULL, JSPROP_SHARED);

Ugh.. really?  For each index?  That's pretty horrible -- we're talking about arrays that can easily have hundreds of thousands of elements.

Regardless, this code is really temporary until we get this implemented natively JS-side.

Comment 6

[Mark Steele]

(In reply to comment #5)
> > Also, you need to actually define your property on the object here. The way
> > this is written now, getting will work (because we will eventually call your
> > getProperty hook), but things like |0 in array| won't. So, you probably want
> > something like:
> > 
> > JS_DefineElement(cx, obj, index, JSVAL_VOID, NULL, NULL, JSPROP_SHARED);
> 
> Ugh.. really?  For each index?  That's pretty horrible -- we're talking about
> arrays that can easily have hundreds of thousands of elements.
> 
> Regardless, this code is really temporary until we get this implemented
> natively JS-side.

Would this speed up subsequent access? It would be nice to be able to short-circuit this by just specifying the index range ahead of time.

Comment 7

[Mark Steele]

Attachment: fix nits.

Just fixed style nits. This excludes the exception and iterator changes.
I think it's ok to throw for now because people have been assuming this worked for a while now by using properties on the wrapper object. I think the plan is to eventually do what WebIDL says, but it may be confusing to assume the array can/did grow by setting properties off the end of the base array.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Man, it sure would be nice if bugzilla auto-cc'd you on bugs where you've reviewed a patch.

(In reply to comment #6)
> Would this speed up subsequent access? It would be nice to be able to
> short-circuit this by just specifying the index range ahead of time.

It wouldn't really speed it up. Yeah, it would definitely be nice if there was a better way to implement an array in the JS API.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/7b4ea23da1fd
http://hg.mozilla.org/mozilla-central/rev/ecc10a6d0a98