Closed Bug 800556 Opened 7 years ago Closed 7 years ago

remove the nsIDOMCanvasRenderingContext2D interface

Categories

(Core :: DOM: Core & HTML, defect)

15 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: nrc, Assigned: nrc)

References

Details

(Keywords: addon-compat)

Attachments

(2 files, 2 obsolete files)

See some discussion in bug 734668.
I believe that, after bug 801712 landed, this will also allow us to have ImageData not inherit from nsISupports.
That's correct.
Assignee: nobody → ncameron
Nick, would you mind putting your dashoffset test in a file of it own?
Attached patch patch (obsolete) — Splinter Review
Attachment #680406 - Flags: superreview?(bzbarsky)
Attachment #680406 - Flags: review?(Ms2ger)
Attached patch test for mozDashOffset (obsolete) — Splinter Review
Attachment #680407 - Flags: review?(Ms2ger)
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.
Attachment #680406 - Flags: superreview?(bzbarsky) → superreview+
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
Attachment #680407 - Flags: review?(Ms2ger) → review+
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.
Attachment #680406 - Flags: review?(Ms2ger) → review+
(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!
(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.
made all reviewers changes, carrying r=Ms2ger,sr=bz
Attachment #680406 - Attachment is obsolete: true
Attachment #681062 - Flags: superreview+
Attachment #681062 - Flags: review+
carrying r=Ms2ger
Attachment #680407 - Attachment is obsolete: true
Attachment #681063 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f2efbff15c83
https://hg.mozilla.org/mozilla-central/rev/70859e1ddfe7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 812453
Depends on: 1019551
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.