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.

Nick, would you mind putting your dashoffset test in a file of it own?

try push: https://tbpl.mozilla.org/?tree=Try&pusher=ncameron@mozilla.com

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 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 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.

(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

carrying r=Ms2ger

https://hg.mozilla.org/mozilla-central/rev/f2efbff15c83 https://hg.mozilla.org/mozilla-central/rev/70859e1ddfe7