734668 - Remove one of the two canvas 2D context implementations

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 2

[Kelsey Gilbert [:jgilbert]]

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 7

[George Wright (:gw280) (needinfo me!)]

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

Comment 8

[Joe Drew (not getting mail)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 11

[Joe Drew (not getting mail)]

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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 13

[Nick Cameron [:nrc]]

Attachment: remove thebes canvas

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 16

[Nick Cameron [:nrc]]

(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

Comment 17

[Nick Cameron [:nrc]]

Attachment: patch 2: Bindings.conf change

Is this all that is needed?

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 21

[Joe Drew (not getting mail)]

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

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 23

[Nick Cameron [:nrc]]

Attachment: patch1: remove thebes canvas

removed one brace too many, carrying r=roc

Comment 24

[Nick Cameron [:nrc]]

Attachment: patch 3: Minor removal

Minor correction requested by Ms2ger

Comment 25

[Nick Cameron [:nrc]]

Attachment: patch 4: rename all the things!

Comment 26

[Nick Cameron [:nrc]]

Attachment: patch 5: remove quickstubs

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 29

[Nick Cameron [:nrc]]

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

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

mozilla::CanvasRenderingContext2D pretty please.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 35

[Andrew McCreight [:mccr8]]

Jorge may have some sense of how likely it is.

Comment 36

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 38

[Nick Cameron [:nrc]]

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

Comment 39

[Nick Cameron [:nrc]]

Attachment: patch 6: reviewer's requested changes

carrying r=roc,Ms2ger

Comment 40

[Nick Cameron [:nrc]]

Attachment: patch 7: sort indents

r=whitespace

Comment 41

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d2b658828771

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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

Comment 44

[Nick Cameron [:nrc]]

(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