Closed Bug 730554 Opened 8 years ago Closed 8 years ago

Cleanup nsIDOMWebGLRenderingContext_Tex{,Sub}Image2D

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

(Whiteboard: [gfx.relnote.13])

Attachments

(3 files, 4 obsolete files)

Attached patch Part a: Deduplicate code (obsolete) — Splinter Review
A lot of duplicated and not-very-well-written code here.
Attachment #600636 - Flags: review?(bzbarsky)
Apparently throwing twice on the same context isn't appreciated
Attachment #600637 - Flags: review?(bzbarsky)
This is basically the same as bug 706052, but it actually passes try.
Attachment #600638 - Flags: review?(bzbarsky)
Comment on attachment 600638 [details] [diff] [review]
Part c: Unwrap to nsGenericElement

r=me
Attachment #600638 - Flags: review?(bzbarsky) → review+
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?
Attachment #600636 - Flags: review?(bzbarsky) → review-
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?.
Attachment #600637 - Flags: review?(bzbarsky) → review-
(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?
Because that makes it clearer what it's doing?
Doesn't really for me... Maybe something like DoCallForImageData?
That's fine too.
Attachment #600636 - Attachment is obsolete: true
Attachment #604645 - Flags: review?(bzbarsky)
Attachment #604646 - Flags: review?(bzbarsky)
Attachment #600637 - Attachment is obsolete: true
Attachment #600638 - Attachment is obsolete: true
And this is why you got the reviews, btw :)
Comment on attachment 604645 [details] [diff] [review]
Part a: Deduplicate code (v2)

r=me
Attachment #604645 - Flags: review?(bzbarsky) → review+
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....
Attachment #604646 - Flags: review?(bzbarsky) → review-
Er, indeed so. I was sure I fixed it.
Attachment #604646 - Attachment is obsolete: true
Attachment #604649 - Flags: review?(bzbarsky)
Comment on attachment 604649 [details] [diff] [review]
Part b: Check the result of JS_GetProperty (v3)

r=me
Attachment #604649 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 706052
Whiteboard: [gfx.relnote.13]
You need to log in before you can comment on or make changes to this bug.