remove the nsIDOMCanvasRenderingContext2D interface

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

(Depends on: 1 bug, {addon-compat})

15 Branch
mozilla19
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
See some discussion in bug 734668.
Keywords: addon-compat
I believe that, after bug 801712 landed, this will also allow us to have ImageData not inherit from nsISupports.
That's correct.
(Assignee)

Updated

5 years ago
Assignee: nobody → ncameron
Nick, would you mind putting your dashoffset test in a file of it own?
(Assignee)

Comment 4

5 years ago
try push: https://tbpl.mozilla.org/?tree=Try&pusher=ncameron@mozilla.com
(Assignee)

Comment 5

5 years ago
Created attachment 680406 [details] [diff] [review]
patch
Attachment #680406 - Flags: superreview?(bzbarsky)
Attachment #680406 - Flags: review?(Ms2ger)
(Assignee)

Comment 6

5 years ago
Created attachment 680407 [details] [diff] [review]
test for mozDashOffset
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+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 681062 [details] [diff] [review]
patch with changes

made all reviewers changes, carrying r=Ms2ger,sr=bz
Attachment #680406 - Attachment is obsolete: true
Attachment #681062 - Flags: superreview+
Attachment #681062 - Flags: review+
(Assignee)

Comment 13

5 years ago
Created attachment 681063 [details] [diff] [review]
test with changes

carrying r=Ms2ger
Attachment #680407 - Attachment is obsolete: true
Attachment #681063 - Flags: review+
(Assignee)

Comment 14

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=70859e1ddfe7
https://hg.mozilla.org/mozilla-central/rev/f2efbff15c83
https://hg.mozilla.org/mozilla-central/rev/70859e1ddfe7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Depends on: 812453

Updated

2 years ago
Depends on: 1019551
You need to log in before you can comment on or make changes to this bug.