730554 - Cleanup nsIDOMWebGLRenderingContext_Tex{,Sub}Image2D

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Deduplicate code

A lot of duplicated and not-very-well-written code here.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Check the result of JS_GetProperty

Apparently throwing twice on the same context isn't appreciated

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Unwrap to nsGenericElement

This is basically the same as bug 706052, but it actually passes try.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 600638 [details] [diff] [review]
Part c: Unwrap to nsGenericElement

r=me

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 600636 [details] [diff] [review]
Part a: Deduplicate code

>+++ b/content/canvas/src/CustomQS_WebGL.h
>+    nsresult ImageData(WebGLsizei width, WebGLsizei height, JSObject* pixels)

Maybe call this TryImageData?

>+    nsresult Element(nsIDOMElement* elt)

And TryElement?

>+TexImage2DImageDataOrElement(JSContext* cx, T* self, JS::Value* object)

I'd prefer that |self| be a T& here, since it can't be null.

>+    // bail out immediately, don't try to interprete as ImageData

"interpret"

>+    // failed to interprete object as a DOMElement, now try to interprete it as ImageData

"interpret" twice

>+    if (!JS_ValueToECMAInt32(cx, js_width, &int_width) ||
>+        !JS_ValueToECMAInt32(cx, js_height, &int_height))
>+    {
>+        return NS_ERROR_FAILURE;

This is wrong, I think.  The callee here can throw on cx, and then you'll attempt to throw as well when the error return is seen in the caller.  Or am I missing something?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 600637 [details] [diff] [review]
Part b: Check the result of JS_GetProperty

>+    if (!JS_GetProperty(cx, imageData, "width", &js_width) ||
>+        !JS_GetProperty(cx, imageData, "height", &js_height) ||
>+        !JS_GetProperty(cx, imageData, "data", &js_data) ||
...
>         return NS_ERROR_FAILURE;

This has the same problem as part a, right?.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 600636 [details] [diff] [review]
> Part a: Deduplicate code
> 
> >+++ b/content/canvas/src/CustomQS_WebGL.h
> >+    nsresult ImageData(WebGLsizei width, WebGLsizei height, JSObject* pixels)
> 
> Maybe call this TryImageData?
> 
> >+    nsresult Element(nsIDOMElement* elt)
> 
> And TryElement?

Why's that?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Because that makes it clearer what it's doing?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Doesn't really for me... Maybe something like DoCallForImageData?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That's fine too.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: Deduplicate code (v2)

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Check the result of JS_GetProperty (v2)

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part c: Unwrap to nsGenericElement (r=bz)

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And this is why you got the reviews, btw :)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604645 [details] [diff] [review]
Part a: Deduplicate code (v2)

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604646 [details] [diff] [review]
Part b: Check the result of JS_GetProperty (v2)

This is still trying to throw if JS_GetProperty returned false, but if it returned false it already threw....

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: Check the result of JS_GetProperty (v3)

Er, indeed so. I was sure I fixed it.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604649 [details] [diff] [review]
Part b: Check the result of JS_GetProperty (v3)

r=me

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/7cf52fda8a40
https://hg.mozilla.org/mozilla-central/rev/975335c0de31
https://hg.mozilla.org/mozilla-central/rev/ff72521021c5