Last Comment Bug 759726 - Make nsICanvasRenderingContextInternal::SetCanvasElement return void
: Make nsICanvasRenderingContextInternal::SetCanvasElement return void
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-30 06:12 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-06-06 03:30 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (51.98 KB, patch)
2012-06-04 08:19 PDT, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
bzbarsky: feedback+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-05-30 06:12:06 PDT

    
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-06-04 08:19:44 PDT
Created attachment 629787 [details] [diff] [review]
Patch v1

I ended up doing a bit more.
Comment 2 Boris Zbarsky [:bz] 2012-06-05 11:32:11 PDT
Comment on attachment 629787 [details] [diff] [review]
Patch v1

Looks good to me.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-06-05 11:45:19 PDT
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) 2012-06-05 11:55:55 PDT
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 (⌚ UTC+1/+2) 2012-06-05 11:58:39 PDT
<Ms2ger> sorta, mCanvasElement was an nsIDOMHTMLCanvasElement
<Ms2ger> It could have used HTMLCanvasElement()
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-06-06 03:30:03 PDT
https://hg.mozilla.org/mozilla-central/rev/6338a8988917

Note You need to log in before you can comment on or make changes to this bug.