Remove one of the two canvas 2D context implementations

RESOLVED FIXED in mozilla19

Status

()

Core
Canvas: 2D
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Ms2ger, Assigned: nrc)

Tracking

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

Trunk
mozilla19
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
One of nsCanvasRenderingContext2D and nsCanvasRenderingContext2DAzure needs to go, if only for my sanity.
(Reporter)

Comment 1

6 years ago
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.
Depends on: 694351
(Assignee)

Updated

6 years ago
Duplicate of this bug: 721424
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → ncameron
(Assignee)

Updated

6 years ago
Depends on: 718849
(Assignee)

Updated

6 years ago
Depends on: 746883
(Assignee)

Updated

6 years ago
Assignee: ncameron → nobody
(Assignee)

Updated

6 years ago
Depends on: 748113
(Assignee)

Updated

6 years ago
Depends on: 748116
(Assignee)

Updated

6 years ago
Depends on: 773460
(Assignee)

Updated

5 years ago
Assignee: nobody → ncameron
(Assignee)

Comment 13

5 years ago
Created attachment 669804 [details] [diff] [review]
remove thebes canvas
Attachment #669804 - Flags: review?(roc)
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.
Attachment #669804 - Flags: review?(roc) → review+
Also I think in Bindings.conf we can declare CanvasRenderingContext2D as non-prefable.
(Assignee)

Comment 16

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

Comment 17

5 years ago
Created attachment 669847 [details] [diff] [review]
patch 2: Bindings.conf change

Is this all that is needed?
Attachment #669847 - Flags: review?(roc)
Attachment #669847 - Flags: review?(roc) → review+
(Reporter)

Comment 18

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

Comment 19

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

Updated

5 years ago
Duplicate of this bug: 734334
(Reporter)

Updated

5 years ago
Blocks: 776242
Can we, as a second patch, s/Azure// and hg mv to the non-Azure.cpp file?
(Reporter)

Comment 22

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

Comment 23

5 years ago
Created attachment 670254 [details] [diff] [review]
patch1: remove thebes canvas

removed one brace too many, carrying r=roc
Attachment #669804 - Attachment is obsolete: true
Attachment #670254 - Flags: review+
(Assignee)

Comment 24

5 years ago
Created attachment 670256 [details] [diff] [review]
patch 3: Minor removal

Minor correction requested by Ms2ger
Attachment #670256 - Flags: review?(Ms2ger)
(Assignee)

Comment 25

5 years ago
Created attachment 670257 [details] [diff] [review]
patch 4: rename all the things!
Attachment #670257 - Flags: review?(roc)
(Assignee)

Comment 26

5 years ago
Created attachment 670259 [details] [diff] [review]
patch 5: remove quickstubs
Attachment #670259 - Flags: review?(Ms2ger)
(Assignee)

Updated

5 years ago
Attachment #669847 - Attachment description: Bindings.conf change → patch 2: Bindings.conf change
(Assignee)

Updated

5 years ago
Attachment #670254 - Attachment description: remove thebes canvas → patch1: remove thebes canvas
(Assignee)

Updated

5 years ago
Attachment #670256 - Attachment description: Minor removal → patch 3: Minor removal
(Assignee)

Updated

5 years ago
Attachment #670257 - Attachment description: rename all the things! → patch 4: rename all the things!
(Reporter)

Comment 27

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

Comment 28

5 years ago
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
Attachment #670259 - Flags: review?(Ms2ger) → review+
(Reporter)

Updated

5 years ago
Attachment #670256 - Flags: review?(Ms2ger) → review+
(Assignee)

Comment 29

5 years ago
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.
Attachment #670257 - Flags: review?(roc) → review+
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.
(Reporter)

Comment 34

5 years ago
(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.
Keywords: addon-compat
(Assignee)

Updated

5 years ago
Blocks: 800553
(Assignee)

Updated

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

Comment 38

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

Comment 39

5 years ago
Created attachment 670726 [details] [diff] [review]
patch 6: reviewer's requested changes

carrying r=roc,Ms2ger
Attachment #670726 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #670726 - Attachment description: reviewer's requested changes → patch 6: reviewer's requested changes
(Assignee)

Comment 40

5 years ago
Created attachment 670728 [details] [diff] [review]
patch 7: sort indents

r=whitespace
Attachment #670728 - Flags: review+
(Assignee)

Comment 41

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d2b658828771
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
So can we also get rid of IsAzureEnabled() in content/canvas/test/test_canvas.html ?
Depends on: 801991
(Assignee)

Comment 44

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

Updated

5 years ago
Blocks: 801993
(Reporter)

Updated

5 years ago
Duplicate of this bug: 722080
Blocks: 1065182
You need to log in before you can comment on or make changes to this bug.