One of nsCanvasRenderingContext2D and nsCanvasRenderingContext2DAzure needs to go, if only for my sanity.

roc agreed... You don't happen to be a manager? ;)

I figured that we had both because we're still transitioning to Azure.

Hopefully being a manager has nothing to do with it! We definitely need this to happen soonish; it's a development burden. For this to happen, we need to make the cairo Azure backend work well enough to pass tests at least. Nick might be able to work on this.

Instead of using the cairo backend for Azure where we're not using the D2D or CoreGraphics backends, what if we used the Skia backend instead? Reasons in favour: -- It's probably faster -- It's currently more complete (IIUC) -- We want to use it on mobile and getting more usage of it will help Reasons against: -- Julian Viereck is working on adding a new platform feature for pdf.js --- the ability to render high-quality print output through a canvas rendering context. I don't know what support Skia has for printing or how much work it would be for us to hook up. I don't think this is a big issue though.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > > Reasons against: > -- Julian Viereck is working on adding a new platform feature for pdf.js --- > the ability to render high-quality print output through a canvas rendering > context. I don't know what support Skia has for printing or how much work it > would be for us to hook up. I don't think this is a big issue though. Why use canvas for printing and not svg?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5) > Reasons against: > -- Julian Viereck is working on adding a new platform feature for pdf.js --- > the ability to render high-quality print output through a canvas rendering > context. I don't know what support Skia has for printing or how much work it > would be for us to hook up. I don't think this is a big issue though. Skia has a PDF rendering backend. Not sure how complete it is though, but I *think* Chromium uses it for printing.

I am really, really not in a hurry to re-debug our printing support. The tons of bugs we've found and fixed in corner cases, on particular printers, etc make me want to hang on to Cairo for printing with all of our might.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6) > Why use canvas for printing and not svg? I don't know, but I suspect the SVG DOM is too big when you're printing a very large document.

(In reply to Joe Drew (:JOEDREW!) from comment #8) > I am really, really not in a hurry to re-debug our printing support. The > tons of bugs we've found and fixed in corner cases, on particular printers, > etc make me want to hang on to Cairo for printing with all of our might. OK. That means we should use the cairo-Azure backend for printing, and for Julian's canvas feature. I think that's fine, but I still think (and IRC #gfx seemed to reach consensus) that enabling Skia-Azure for non-printing-canvas on all platforms is the way to go.

Yes, the consensus was/is to do Skia first. I presume (perhaps incorrectly!) that canvases currently print as bitmaps anyways, so there shouldn't be too much to do to hook a skia-generated canvas into a cairo-generated print job.

Right. In fact, I believe nothing has to be done as long as the existing Azure/Thebes interop APIs work. It's only a problem for Julian's future work that I mentioned in comment #5.

Comment on attachment 669804 [details] [diff] [review] remove thebes canvas Review of attachment 669804 [details] [diff] [review]: ----------------------------------------------------------------- nsHTMLCanvasElement::GetContext needs to have some thebes-related stuff removed as well. That can happen in a separate patch.

Also I think in Bindings.conf we can declare CanvasRenderingContext2D as non-prefable.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > Comment on attachment 669804 [details] [diff] [review] > remove thebes canvas > > Review of attachment 669804 [details] [diff] [review]: > ----------------------------------------------------------------- > > nsHTMLCanvasElement::GetContext needs to have some thebes-related stuff > removed as well. That can happen in a separate patch. That is in Bug 748113

Is this all that is needed?

Comment on attachment 669804 [details] [diff] [review] remove thebes canvas Review of attachment 669804 [details] [diff] [review]: ----------------------------------------------------------------- Yay! ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +843,5 @@ > mTarget = layerManager->CreateDrawTarget(size, format); > } else { > mTarget = gfxPlatform::GetPlatform()->CreateOffscreenDrawTarget(size, format); > } > + This change looks wrong ::: dom/base/nsDOMClassInfo.cpp @@ +4553,5 @@ > // overwrite some values. > mozilla::dom::oldproxybindings::Register(nameSpaceManager); > > + nameSpaceManager->RegisterDefineDOMInterface(NS_LITERAL_STRING("CanvasRenderingContext2D"), > + nullptr, nullptr); This is wrong. This line should be removed instead.

(In reply to Nick Cameron [:nrc] from comment #17) > Created attachment 669847 [details] [diff] [review] > Bindings.conf change > > Is this all that is needed? You'll also need to remove the quickstubs (js/xpconnect/src/dom_quickstubs.qsconf, content/canvas/src/CustomQS_Canvas2D.h, content/canvas/src/CustomQS_Canvas.h).

Can we, as a second patch, s/Azure// and hg mv to the non-Azure.cpp file?

(In reply to Joe Drew (:JOEDREW! \o/) from comment #21) > Can we, as a second patch, s/Azure// and hg mv to the non-Azure.cpp file? If we're going to do that, mozilla::dom::CanvasRenderingContext2D please.

removed one brace too many, carrying r=roc

Minor correction requested by Ms2ger

Comment on attachment 670257 [details] [diff] [review] patch 4: rename all the things! Review of attachment 670257 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +123,5 @@ > + Telemetry::Accumulate(Telemetry::CANVAS_2D_USED, 1); > + nsRefPtr<nsIDOMCanvasRenderingContext2D> ctx = > + new mozilla::dom::CanvasRenderingContext2D(); > + if (!ctx) > + return NS_ERROR_OUT_OF_MEMORY; This can't happen @@ +125,5 @@ > + new mozilla::dom::CanvasRenderingContext2D(); > + if (!ctx) > + return NS_ERROR_OUT_OF_MEMORY; > + > + *aResult = ctx.forget().get(); ctx.forget(aResult); @@ +166,1 @@ > const Point &aEndOrigin, Float aEndRadius) Indentation... I bet there are more places like this :) @@ +1970,5 @@ > > const ContextState &state = CurrentState(); > > if (state.patternStyles[STYLE_FILL]) { > + CanvasPattern::RepeatMode repeat = While you're here, trailing whitespace :) ::: content/canvas/src/nsCanvasRenderingContext2DAzure.h @@ +40,2 @@ > **/ > +class CanvasGradient : public nsIDOMCanvasGradient Please file a followup to move those to WebIDL too. ::: content/canvas/src/Makefile.in @@ +28,3 @@ > > EXPORTS_mozilla/dom = \ > ImageData.h \ Add CanvasRenderingContext2D.h here, please. (Alphabetical ordering.) ::: dom/bindings/Bindings.conf @@ +105,5 @@ > }], > > 'CanvasRenderingContext2D': [ > { > + 'nativeType': 'mozilla::dom::CanvasRenderingContext2D', This should be the default, so you can remove this line @@ +106,5 @@ > > 'CanvasRenderingContext2D': [ > { > + 'nativeType': 'mozilla::dom::CanvasRenderingContext2D', > + 'headerFile': 'CanvasRenderingContext2D.h', And drop this too, after you've exported.

Comment on attachment 670259 [details] [diff] [review] patch 5: remove quickstubs Review of attachment 670259 [details] [diff] [review]: ----------------------------------------------------------------- Seeing this code die makes me weep tears of joy. ::: js/xpconnect/src/dom_quickstubs.qsconf @@ +963,5 @@ > 'nsIDOMWindow_SetOnmouseleave' : { > 'thisType' : 'nsIDOMWindow', > 'unwrapThisFailureFatal' : False > }, > + Trailing whitespace

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0f61a66423e6

(In reply to :Ms2ger from comment #22) > If we're going to do that, mozilla::dom::CanvasRenderingContext2D please. mozilla::CanvasRenderingContext2D pretty please.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > (In reply to :Ms2ger from comment #22) > > If we're going to do that, mozilla::dom::CanvasRenderingContext2D please. > > mozilla::CanvasRenderingContext2D pretty please. But I guess mozilla::dom is OK, and it's what WebIDL expects.

Comment on attachment 670257 [details] [diff] [review] patch 4: rename all the things! Review of attachment 670257 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp @@ +588,1 @@ > bool *triedToWrap) Fix indent @@ +595,1 @@ > nscolor* aColor) Fix indent @@ +660,1 @@ > Style whichStyle) Fix indent @@ +674,2 @@ > nsISupports *aInterface, > Style aWhichStyle) Fix indent @@ +701,2 @@ > CanvasMultiGetterType& aType, > Style aWhichStyle) Fix indent @@ +907,5 @@ > state->shadowColor = NS_RGBA(0,0,0,0); > } > > NS_IMETHODIMP > +CanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *shell, gfxASurface *surface, int32_t width, int32_t height) Wrap line somewhere @@ +995,2 @@ > const PRUnichar *aEncoderOptions, > nsIInputStream **aStream) Fix indent @@ +1205,2 @@ > double m22, double dx, double dy, > ErrorResult& error) Fix indent @@ +1233,3 @@ > double m21, double m22, > double dx, double dy, > ErrorResult& error) Fix indent @@ +1311,2 @@ > JSObject& currentTransform, > ErrorResult& error) Fix indent ... oh, I give up. Just fix them all.

I believe we can also remove nsIDOMCanvasRenderingContext2D, can't we? And once we do that, we can remove all the crappy XPCOM-ish versions of the methods on CanvasRenderingContext2D.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33) > I believe we can also remove nsIDOMCanvasRenderingContext2D, can't we? > > And once we do that, we can remove all the crappy XPCOM-ish versions of the > methods on CanvasRenderingContext2D. Assuming that no addons use it; might be worth having a look at the addons MXR at least.

Jorge may have some sense of how likely it is.

There are some add-ons that use it (not many): https://mxr.mozilla.org/addons/search?string=nsIDOMCanvasRenderingContext2D It seems to be only used to get to some constants: > DRAW_WINDOW_FLAGS : Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DRAW_VIEW | > Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DRAW_CARET | > Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DO_NOT_FLUSH If we have a viable alternative for developers, I see no problem in removing the interface.

(In reply to Jorge Villalobos [:jorgev] from comment #36) > It seems to be only used to get to some constants: > > > DRAW_WINDOW_FLAGS : Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DRAW_VIEW | > > Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DRAW_CARET | > > Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DO_NOT_FLUSH > > If we have a viable alternative for developers, I see no problem in removing > the interface. I think it would be fine to leave the constants in and gut the rest of the interface and not use it elsewhere. At least for a while.

Another try push, this time with the canvas tests turn on (the m1 fails are getting starred on inbound): https://tbpl.mozilla.org/?tree=Try&rev=1302f25d0979

carrying r=roc,Ms2ger

r=whitespace

https://hg.mozilla.org/mozilla-central/rev/ab05eadb67c9 https://hg.mozilla.org/mozilla-central/rev/49718303988e https://hg.mozilla.org/mozilla-central/rev/a082a3573e11 https://hg.mozilla.org/mozilla-central/rev/dfc923f59b16 https://hg.mozilla.org/mozilla-central/rev/f5c8dabab38f https://hg.mozilla.org/mozilla-central/rev/6dae073c5dff https://hg.mozilla.org/mozilla-central/rev/d2b658828771

So can we also get rid of IsAzureEnabled() in content/canvas/test/test_canvas.html ?

(In reply to Boris Zbarsky (:bz) from comment #43) > So can we also get rid of IsAzureEnabled() in > content/canvas/test/test_canvas.html ? Yes, sorry, missed that one