759726 - Make nsICanvasRenderingContextInternal::SetCanvasElement return void

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Comment 1

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

Attachment: Patch v1

I ended up doing a bit more.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 629787 [details] [diff] [review]
Patch v1

Looks good to me.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 629787 [details] [diff] [review]
Patch v1

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

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +47,5 @@
>    };
>  
> +  void SetCanvasElement(nsHTMLCanvasElement* aParentCanvas)
> +  {
> +    mCanvasElement = aParentCanvas;

This is making me nervous: there was this comment saying exactly to NOT do this, but not explaining why that would be a bad thing; now you're doing this; I need an explanation.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 629787 [details] [diff] [review]
Patch v1

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

r=me modulo a nontrivial question about the removed do_QueryInterface. Also some style nits, not duplicated everywhere where they apply. Note that comments/questions on the xxx2d file apply as well to the xxx2dAzure file.

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +839,5 @@
>      nsresult rv;
>      nscolor color;
>  
>      if (!aStr.IsVoid()) {
> +        nsIDocument* document = mCanvasElement ? mCanvasElement->OwnerDoc() : nsnull;

Suggest MFBT style:

 x = a
     ? b
     : c;

@@ +2434,5 @@
>       * string is equal to the old one.
>       */
>  
> +    if (!mCanvasElement && !mDocShell) {
> +        NS_WARNING("Canvas element must be non-null or a docshell must be provided");

If this case is a plain c++ coding mistake (which I don't know) then this should be a fatal-in-debug assert.

@@ +2812,5 @@
>      // arg, and there is no way to tell if the default was used
>      if (aMaxWidth < 0)
>          return NS_ERROR_INVALID_ARG;
>  
> +    if (!mCanvasElement && !mDocShell) {

Was the do_QueryInterface useless?

Comment 5

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

<Ms2ger> sorta, mCanvasElement was an nsIDOMHTMLCanvasElement
<Ms2ger> It could have used HTMLCanvasElement()

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/6338a8988917