Last Comment Bug 800556 - remove the nsIDOMCanvasRenderingContext2D interface
: remove the nsIDOMCanvasRenderingContext2D interface
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 15 Branch
: All All
: -- normal (vote)
: mozilla19
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on: 1019551 734668 812453
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 14:24 PDT by Nick Cameron [:nrc]
Modified: 2015-05-10 07:51 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (81.56 KB, patch)
2012-11-10 21:49 PST, Nick Cameron [:nrc]
Ms2ger: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
test for mozDashOffset (2.09 KB, patch)
2012-11-10 21:50 PST, Nick Cameron [:nrc]
Ms2ger: review+
Details | Diff | Splinter Review
patch with changes (82.78 KB, patch)
2012-11-13 08:46 PST, Nick Cameron [:nrc]
ncameron: review+
ncameron: superreview+
Details | Diff | Splinter Review
test with changes (2.80 KB, patch)
2012-11-13 08:47 PST, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-10-11 14:24:09 PDT
See some discussion in bug 734668.
Comment 1 :Ms2ger 2012-10-15 12:25:43 PDT
I believe that, after bug 801712 landed, this will also allow us to have ImageData not inherit from nsISupports.
Comment 2 Boris Zbarsky [:bz] 2012-10-15 13:18:01 PDT
That's correct.
Comment 3 :Ms2ger 2012-11-10 11:50:36 PST
Nick, would you mind putting your dashoffset test in a file of it own?
Comment 4 Nick Cameron [:nrc] 2012-11-10 17:39:55 PST
try push: https://tbpl.mozilla.org/?tree=Try&pusher=ncameron@mozilla.com
Comment 5 Nick Cameron [:nrc] 2012-11-10 21:49:45 PST
Created attachment 680406 [details] [diff] [review]
patch
Comment 6 Nick Cameron [:nrc] 2012-11-10 21:50:30 PST
Created attachment 680407 [details] [diff] [review]
test for mozDashOffset
Comment 7 Boris Zbarsky [:bz] 2012-11-10 23:35:55 PST
Comment on attachment 680406 [details] [diff] [review]
patch

I don't see why you need ToHTMLImageOrCanvasOrVideoElement in nsHTMLCanvasElement.cpp.  You know you havea <canvas> there.  Just do this:

   HTMLImageOrCanvasOrVideoElement element;
   element.SetasHTMLCanvasElement() = this;

Equals(NS_LITERAL_STRING("2d")) should be EqualsLiteral("2d").

You need to change the iid for nsIDOMCanvasRenderingContext2D.

I assume you left the interface in place just so that its constants can be used somewhere?  Worth documenting that, as well as what uses those constants or might use them.

sr=me with that.
Comment 8 :Ms2ger 2012-11-11 10:51:43 PST
Comment on attachment 680407 [details] [diff] [review]
test for mozDashOffset

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

::: content/canvas/test/test_mozDashOffset.html
@@ +28,5 @@
> +  ctx.mozDashOffset = 1;
> +  ctx.mozDashOffset = NaN;
> +  ok(ctx.mozDashOffset === 1, "ctx.mozDashOffset === 1");
> +} catch(e) {
> +  throw e;

Drop this line
Comment 9 :Ms2ger 2012-11-11 11:21:54 PST
Comment on attachment 680406 [details] [diff] [review]
patch

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

LGTM, thanks! r=me with the comments below and bz's comments addressed.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +525,3 @@
>    NS_INTERFACE_MAP_ENTRY(nsICanvasRenderingContextInternal)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports,
>                                     nsICanvasRenderingContextInternal)

I bet this can simply be NS_INTERFACE_MAP_ENTRY(nsISupports) now.

@@ +1386,2 @@
>  void
>  CanvasRenderingContext2D::SetFillRule(const nsAString& aString)

Followup to make fillRule use an enum, please.

@@ -1709,5 @@
> -
> -NS_IMETHODIMP
> -CanvasRenderingContext2D::SetFillStyle_multi(const nsAString& aStr, nsISupports *aInterface)
> -{
> -  SetStyleFromStringOrInterface(aStr, aInterface, STYLE_FILL);

I think you can remove SetStyleFromStringOrInterface too.

@@ +2756,1 @@
>      if (NS_FAILED(rv)) {

Just make this if (err.Failed()), and remove the rv variable and the assertion below.

@@ -3565,5 @@
>    }
> -  ContextState& state = CurrentState();
> -  if (!state.dash.IsEmpty()) {
> -    state.dashOffset = offset;
> -  }

This check seems to have disappeared?

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +7,5 @@
>  
>  #include <vector>
>  #include "nsIDOMCanvasRenderingContext2D.h"
>  #include "nsICanvasRenderingContextInternal.h"
> +#include "nsIDOMXULElement.h"

Forward declare nsIDOMXULElement, please.

@@ +493,5 @@
>    // nsISupports interface + CC
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>  
>    NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS(CanvasRenderingContext2D,
> +                                                                   nsICanvasRenderingContextInternal)

This should no longer be ambiguous either.

@@ +542,5 @@
>    nsresult GetImageDataArray(JSContext* aCx, int32_t aX, int32_t aY,
>                               uint32_t aWidth, uint32_t aHeight,
>                               JSObject** aRetval);
>  
> +  nsresult PutImageData_explicit(int32_t x, int32_t y, uint32_t w, uint32_t h,

Feel free to come up with a better name for this function.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +351,5 @@
>  
> +    HTMLImageOrCanvasOrVideoElement element;
> +    if (!ToHTMLImageOrCanvasOrVideoElement(self, element)) {
> +      return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +    }

As bz said, and you can probably replace 'self' by 'this' here.

@@ +717,5 @@
>  
> +  if (aContextId.Equals(NS_LITERAL_STRING("2d"))) {
> +    Telemetry::Accumulate(Telemetry::CANVAS_2D_USED, 1);
> +    nsRefPtr<mozilla::dom::CanvasRenderingContext2D> ctx =
> +      new mozilla::dom::CanvasRenderingContext2D();

You shouldn't need the mozilla::dom::

@@ +750,1 @@
>    if (rv == NS_ERROR_OUT_OF_MEMORY) {

No need for this added newline

::: widget/windows/TaskbarPreview.cpp
@@ +40,5 @@
>  
>  // Shared by all TaskbarPreviews to avoid the expensive creation process.
>  // Manually refcounted (see gInstCount) by the ctor and dtor of TaskbarPreview.
>  // This is done because static constructors aren't allowed for perf reasons.
> +mozilla::dom::CanvasRenderingContext2D* gCtx = NULL;

Use 'dom::', not 'mozilla::dom::' in this file.

@@ +62,2 @@
>      // create the canvas rendering context
> +    Telemetry::Accumulate(Telemetry::CANVAS_2D_USED, 1);

Not sure if we actually want this, but sure.
Comment 10 Nick Cameron [:nrc] 2012-11-12 10:18:41 PST
(In reply to :Ms2ger from comment #9)
> Comment on attachment 680406 [details] [diff] [review]
> patch
> 
> Review of attachment 680406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ -3565,5 @@
> >    }
> > -  ContextState& state = CurrentState();
> > -  if (!state.dash.IsEmpty()) {
> > -    state.dashOffset = offset;
> > -  }
> 
> This check seems to have disappeared?
> 

Yeah, I wasn't sure about this, I checked the spec for dashOffset and I think this check is wrong, I don't see why we should ignore the dash offset if there is no dash. What do you think?

> ::: content/canvas/src/CanvasRenderingContext2D.h
> @@ +493,5 @@
> >    // nsISupports interface + CC
> >    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> >  
> >    NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS(CanvasRenderingContext2D,
> > +                                                                   nsICanvasRenderingContextInternal)
> 
> This should no longer be ambiguous either.
> 

How should this work without ambiguous? We still inherit from nsICanvasRenderingContextInternal, but using just that gives a compile error (I have no idea what NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS[_AMBIGUOUS] does).

> @@ +542,5 @@
> >    nsresult GetImageDataArray(JSContext* aCx, int32_t aX, int32_t aY,
> >                               uint32_t aWidth, uint32_t aHeight,
> >                               JSObject** aRetval);
> >  
> > +  nsresult PutImageData_explicit(int32_t x, int32_t y, uint32_t w, uint32_t h,
> 
> Feel free to come up with a better name for this function.
> 
PutImageDataInternal? I'm not sure that's better. I'm not good at names, sorry.
> ::: content/html/content/src/nsHTMLCanvasElement.cpp
> @@ +62,2 @@
> >      // create the canvas rendering context
> > +    Telemetry::Accumulate(Telemetry::CANVAS_2D_USED, 1);
> 
> Not sure if we actually want this, but sure.

We had this in the old method, so I just moved it, I assume it is still required.

Everything is done, thanks for the review!
Comment 11 :Ms2ger 2012-11-12 12:28:04 PST
(In reply to Nick Cameron [:nrc] from comment #10)
> (In reply to :Ms2ger from comment #9)
> > Comment on attachment 680406 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 680406 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ -3565,5 @@
> > >    }
> > > -  ContextState& state = CurrentState();
> > > -  if (!state.dash.IsEmpty()) {
> > > -    state.dashOffset = offset;
> > > -  }
> > 
> > This check seems to have disappeared?
> > 
> 
> Yeah, I wasn't sure about this, I checked the spec for dashOffset and I
> think this check is wrong, I don't see why we should ignore the dash offset
> if there is no dash. What do you think?

I'd prefer not changing behaviour here for now.

> > ::: content/canvas/src/CanvasRenderingContext2D.h
> > @@ +493,5 @@
> > >    // nsISupports interface + CC
> > >    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > >  
> > >    NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS(CanvasRenderingContext2D,
> > > +                                                                   nsICanvasRenderingContextInternal)
> > 
> > This should no longer be ambiguous either.
> > 
> 
> How should this work without ambiguous? We still inherit from
> nsICanvasRenderingContextInternal, but using just that gives a compile error
> (I have no idea what
> NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS[_AMBIGUOUS] does).

I think

NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS(CanvasRenderingContext2D)

should work.
Comment 12 Nick Cameron [:nrc] 2012-11-13 08:46:35 PST
Created attachment 681062 [details] [diff] [review]
patch with changes

made all reviewers changes, carrying r=Ms2ger,sr=bz
Comment 13 Nick Cameron [:nrc] 2012-11-13 08:47:48 PST
Created attachment 681063 [details] [diff] [review]
test with changes

carrying r=Ms2ger

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