1146454 - Remove CPOW usage in printing

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

[Dave Townsend [:mossop]]

Using contentWindowAsCPOW in printing is sort of clunky and I think there is a straightforward way to remove it. Ideally we should only need the browser we print in and preview in to have message managers and nothing else so we can eventually drop contentWindowAsCPOW entirely.

Comment 1

[Dave Townsend [:mossop]]

Attachment: WIP

Having some difficulty getting printing to work at all on my machine but this sort of thing seems like a good idea. What do you think to switching to outer window IDs?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8582002 [details] [diff] [review]
WIP

This is a good idea.

We might want to mark the pattern of passing the nsIDOMWindow directly as deprecated, and file a bug to remove it entirely down the line.

Comment 3

[Dave Townsend [:mossop]]

Not high priority so I won't get to this soon.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

This blocks / solves one of my M8's, so marking this M8 and taking.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?felipe

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12617/#review11081

::: toolkit/components/printing/content/printUtils.js:106
(Diff revision 1)
> -   * @param aBrowser (optional for non-remote browsers)
> +   * @param aBrowser (optional if aWindow is a non-remote nsIDOMWindow)

Nit - aWindow -> aWindowOrWindowID

::: toolkit/components/printing/content/printUtils.js:108
(Diff revision 1)
>     *        not necessary if aWindow came from a non-remote browser, but is

Nit - aWindow -> aWindowOrWindowID

::: toolkit/components/printing/content/printUtils.js:111
(Diff revision 1)
> -   *        browser must have its type attribute set to "content",
> +   *        1) aWindowOrWindowID comes from a remote browser and aBrowser is
> +   *           not provided.

Not true, and should probably just be removed. We should always pass the browser if we're passing the outer window ID, full stop.

::: toolkit/components/printing/content/printUtils.js:156
(Diff revision 1)
> -    if (!aBrowser) {
> +        if (!aBrowser) {
> -      throw new Error("PrintUtils.print could not resolve content window " +
> +          throw new Error("PrintUtils.print could not resolve content window " +
> -                      "to a browser.");
> +                          "to a browser.");

Move this outside again.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?felipe

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?felipe

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12615/#review11295

Hey billm - normally, I'd get Mossop or felipe to review something like this, but both are on PTO. Are you comfortable doing it?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

https://reviewboard.mozilla.org/r/12617/#review11305

This looks good, but I'd like a few changes.

::: toolkit/content/browser-content.js:396
(Diff revision 2)
> -        this.enterPrintPreview(objects.contentWindow);
> +        this.enterPrintPreview(Services.wm.getOuterWindowWithId(data.windowID));

It seems we don't support print preview for frames, so maybe we shouldn't bother sending the windowID and should just use |content| here.

::: toolkit/components/printing/content/printUtils.js:118
(Diff revision 2)
> -  print: function (aWindow, aBrowser)
> +  print: function (aWindowOrWindowID, aBrowser)

There aren't very many callers of this function in Firefox, although there are a bunch of add-on callers. Let's do this:

- Make a separate printWindow function that requires a <browser> and a window ID as arguments. We'll use this function throughout Firefox.
- Provide a print method for add-on compat. It ignores all its arguments. It tries to find a gBrowser symbol on the global and prints gBrowser.selectedBrowser. (I know this is toolkit code, but it's only for backwards compat.) Please comment that this function exists only for add-on compatibility.

If you do this, then you can drop the PrintWindow message entirely.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12617/#review11305

> It seems we don't support print preview for frames, so maybe we shouldn't bother sending the windowID and should just use |content| here.

When we send this message down from PrintUtils, we're sending the message through the message manager of the "print preview" browser - that's the browser in which we will be showing the print preview, and not the browser which is holding the content that we want to print preview.

So we have to pass the outer window ID down, I'm afraid.

> There aren't very many callers of this function in Firefox, although there are a bunch of add-on callers. Let's do this:
> 
> - Make a separate printWindow function that requires a <browser> and a window ID as arguments. We'll use this function throughout Firefox.
> - Provide a print method for add-on compat. It ignores all its arguments. It tries to find a gBrowser symbol on the global and prints gBrowser.selectedBrowser. (I know this is toolkit code, but it's only for backwards compat.) Please comment that this function exists only for add-on compatibility.
> 
> If you do this, then you can drop the PrintWindow message entirely.

Not just add-ons, I'm afraid - Thunderbird still uses this, so I'm going to cut them a break and support the original API for this method for now, but mark it deprecated.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?billm

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12615/#review11337

I've made the suggested changes, but I still need to run this through some tests. Will re-request review tomorrow, thanks!

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> Not just add-ons, I'm afraid - Thunderbird still uses this, so I'm going to
> cut them a break and support the original API for this method for now, but
> mark it deprecated.

I just searched through comm-central and they only use the no-argument PrintUtils.print version. So I think my proposal would not break Thunderbird.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #14)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #11)
> > Not just add-ons, I'm afraid - Thunderbird still uses this, so I'm going to
> > cut them a break and support the original API for this method for now, but
> > mark it deprecated.
> 
> I just searched through comm-central and they only use the no-argument
> PrintUtils.print version. So I think my proposal would not break Thunderbird.

Ok, fair enough - though I won't use gBrowser.selectedBrowser, since other apps probably don't have those defined. I'll use window.content for now.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?billm

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?billm

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

Bug 1146454 - Stop using CPOWs for printing. r?billm

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Interdiff: https://reviewboard.mozilla.org/r/12617/diff/2-5/

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8629498 [details]
MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm

https://reviewboard.mozilla.org/r/12617/#review11541

Looks great! Thanks for making the changes.

::: toolkit/components/viewsource/content/viewPartialSource.xul:42
(Diff revision 5)
> -  <command id="cmd_print" oncommand="PrintUtils.print();"/>
> +  <command id="cmd_print" oncommand="PrintUtils.printWindow(gBrowser.outerWindowID, gBrowser);"/>

Does tabbrowser.xml define an outerWindowID property?

::: toolkit/components/viewsource/content/viewSource.xul:42
(Diff revision 5)
> -  <command id="cmd_print" oncommand="PrintUtils.print(gBrowser.contentWindowAsCPOW, gBrowser);"/>
> +  <command id="cmd_print" oncommand="PrintUtils.printWindow(gBrowser.outerWindowID, gBrowser);"/>

Same.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bill McCloskey (:billm) from comment #20)
> Comment on attachment 8629498 [details]
> MozReview Request: Bug 1146454 - Stop using CPOWs for printing. r?billm
> 
> https://reviewboard.mozilla.org/r/12617/#review11541
> 
> Looks great! Thanks for making the changes.
> 
> ::: toolkit/components/viewsource/content/viewPartialSource.xul:42
> (Diff revision 5)
> > -  <command id="cmd_print" oncommand="PrintUtils.print();"/>
> > +  <command id="cmd_print" oncommand="PrintUtils.printWindow(gBrowser.outerWindowID, gBrowser);"/>
> 
> Does tabbrowser.xml define an outerWindowID property?
> 
> ::: toolkit/components/viewsource/content/viewSource.xul:42
> (Diff revision 5)
> > -  <command id="cmd_print" oncommand="PrintUtils.print(gBrowser.contentWindowAsCPOW, gBrowser);"/>
> > +  <command id="cmd_print" oncommand="PrintUtils.printWindow(gBrowser.outerWindowID, gBrowser);"/>
> 
> Same.

It does not! But it probably should. Thanks - good catch!

Comment 22

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12617/#review11541

> Does tabbrowser.xml define an outerWindowID property?

Ah, I see what happened here. In the context of the viewSource.xul and viewPartialSource.xul documents, gBrowser refers to the <xul:browser> showing the source, not a <xul:tabbrowser>.

Kinda confusing to use gBrowser between different windows to mean different things, but, well, there we have it.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> Ah, I see what happened here. In the context of the viewSource.xul and
> viewPartialSource.xul documents, gBrowser refers to the <xul:browser>
> showing the source, not a <xul:tabbrowser>.

Ah, I see. Disregard my comment then.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

url:        https://hg.mozilla.org/integration/fx-team/rev/dfd2a128d19ce8f485543428e09328f450d96791
changeset:  dfd2a128d19ce8f485543428e09328f450d96791
user:       Mike Conley <[e-mail]>
date:       Fri Jul 03 16:06:04 2015 -0400
description:
Bug 1146454 - Stop using CPOWs for printing. r=billm

We were passing around content window CPOWs before to indicate
which content to send to the printer. This was, naturally, causing
unsafe CPOW usage warnings - especially when attempting to get at
the content window of an iframe with the context menu printing
command.

This patch changes the printing mechanism to use outer window IDs
instead of CPOWs.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dfd2a128d19c