The default bug view has changed. See this FAQ.

Make nsICanvasRenderingContextInternal::SetCanvasElement return void

RESOLVED FIXED in mozilla16

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 629787 [details] [diff] [review]
Patch v1

I ended up doing a bit more.
Attachment #629787 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
Attachment #629787 - Flags: feedback?(bzbarsky)
Comment on attachment 629787 [details] [diff] [review]
Patch v1

Looks good to me.
Attachment #629787 - Flags: feedback?(bzbarsky) → feedback+
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 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?
Attachment #629787 - Flags: review?(bjacob) → review+
(Assignee)

Comment 5

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

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6338a8988917
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.