1082575 - Make PrintUtils and printPreviewBindings.xml e10s friendly

Status: RESOLVED FIXED

(Toolkit :: Printing, defect)

Description

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

PrintUtils is the toolkit utility script for print and print preview functionality, and is the primary entrypoint for printing activities from the front-end of Gecko-based applications.

PrintUtils currently assumes that it has direct access to things like the browser docshells, which is simply not the case for remote browsers.

We need to make PrintUtils more e10s friendly. Some exploratory work was done in bug 927188, but I'm splitting it off because I think this work can (and should) land separately from the rest of bug 927188.

Comment 1

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

This looks like it will have to involve printPreviewBindings.xml as well.

Comment 2

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

Attachment: [WIP] Make PrintUtils e10s friendly. r=?

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8505213 [details] [diff] [review]
[WIP] Make PrintUtils e10s friendly. r=?

Review of attachment 8505213 [details] [diff] [review]:
-----------------------------------------------------------------

Looks on the right path to me. Do we know who uses those methods that are being deprecated?

I'd love it if we had notes somewhere, maybe at the top of printUtils about the flow of method calls and messaging that we see from printing and previewing.

::: toolkit/components/printing/content/printUtils.js
@@ +70,5 @@
>      if (this.bailOut()) {
>        return;
>      }
> +    let printSettings = this.getPrintSettings();
> +    let browserMM = gBrowser.selectedBrowser.messageManager;

Use gBrowser.getBrowserForContentWindow to make sure you have the right browser element for aWindow here.

@@ +141,1 @@
>      // an error code could be returned if the Prgress Dialog is already displayed

While you're here, s/Prgress/Progress/

@@ +169,2 @@
>    getWebBrowserPrint: function (aWindow)
>    {

Can we log a deprecation warning here? Or maybe just in e10s mode

@@ +188,2 @@
>    getPrintPreview: function() {
> +    if (gMultiProcessBrowser) {

Same again

@@ +231,5 @@
> +      }
> +
> +      case "Printing:PrintPreview:StateChange": {
> +        // Gross hack because we don't get the observer notification for when
> +        // the dialog is closing.

Why is this a hack? What is it supporting?

@@ +307,5 @@
> +    let browserMM = ppBrowser.messageManager;
> +    let printSettings = this.getPrintSettings();
> +    browserMM.sendAsyncMessage("Printing:EnterPrintPreview", null, {
> +      printSettings: printSettings,
> +      contentWindow: this._sourceBrowser.contentWindowAsCPOW,

I'm guessing this isn't right for printing an inner frame.

::: toolkit/content/browser-content.js
@@ +457,5 @@
> +  },
> +
> +  showPrinterProperties(parent, printerName, printSettings) {
> +    throw Cr.NS_ERROR_NOT_IMPLEMENTED;
> +  },

Pro-tip, you don't need to list all these methods, xpconnect will just throw NS_ERROR_NOT_IMPLEMENTED if they aren't defined on the JS object anyway.

Comment 4

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

Attachment: Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

Comment 5

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

Attachment: Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

Comment 6

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

Comment on attachment 8510379 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

I'm so sorry to point this review request at you - it's really not the most pleasant area of the code-base. Unfortunately, I'm not sure who else to point it at.

Remind me to buy you a beer in Portland.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8510379 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

Review of attachment 8510379 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, a couple of questions I'd like to see answers to first though.

::: browser/base/content/browser.js
@@ +6920,5 @@
>  
> +function getBrowserForContentWindow(aWindow) {
> +  return gBrowser.getBrowserForContentWindow(aWindow);
> +}
> +

Ick :(

::: toolkit/components/printing/content/printPreviewBindings.xml
@@ +161,5 @@
>  
>        <method name="navigate">
>          <parameter name="aDirection"/>
>          <parameter name="aPageNum"/>
>          <parameter name="aHomeOrEnd"/>

Damn, who designed this interface??

@@ +165,5 @@
>          <parameter name="aHomeOrEnd"/>
>          <body>
> +        <![CDATA[
> +          const nsIWebBrowserPrint = Components.interfaces.nsIWebBrowserPrint;
> +          let navType, pageNum;

pageNum doesn't seem to ever be used

@@ +188,5 @@
> +            pageNum = this.mPageTextBox.valueNumber;
> +          } else {
> +            // We're going to a specific page (aPageNum)
> +            navType = nsIWebBrowserPrint.PRINTPREVIEW_GOTO_PAGENUM;
> +            pageNum = aPageNum;

this.mPageTextBox.value probably

::: toolkit/components/printing/content/printUtils.js
@@ +137,3 @@
>      }
> +
> +    let browser = this.getBrowserForContentWindow(aWindow);

I really dislike this. Any reason we can't just have the caller pass in the browser as well as the window?

@@ +309,5 @@
> +
> +  getBrowserForContentWindow(aWindow) {
> +    let browser;
> +
> +    if (this.usingRemoteTabs) {

You could just test if the window is a CPOW or not.

@@ +421,5 @@
> +    let mm = ppBrowser.messageManager;
> +    let printSettings = this.getPrintSettings();
> +    mm.sendAsyncMessage("Printing:Preview:Enter", null, {
> +      printSettings: printSettings,
> +      contentWindow: this._sourceBrowser.contentWindowAsCPOW,

I didn't see an answer to this, how does this cope with printing inner-frames?

Comment 9

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

Attachment: Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

Comment 10

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

Comment on attachment 8511973 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?

(In reply to Dave Townsend [:mossop] from comment #8)
> Comment on attachment 8510379 [details] [diff] [review]
> Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?
> 
> Review of attachment 8510379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there, a couple of questions I'd like to see answers to first though.
> 
> ::: browser/base/content/browser.js
> @@ +6920,5 @@
> >  
> > +function getBrowserForContentWindow(aWindow) {
> > +  return gBrowser.getBrowserForContentWindow(aWindow);
> > +}
> > +
> 
> Ick :(

Removed - I went with your suggestion of having the caller pass in the browser.

> 
> ::: toolkit/components/printing/content/printPreviewBindings.xml
> @@ +161,5 @@
> >  
> >        <method name="navigate">
> >          <parameter name="aDirection"/>
> >          <parameter name="aPageNum"/>
> >          <parameter name="aHomeOrEnd"/>
> 
> Damn, who designed this interface??

Not I. :)

> 
> @@ +165,5 @@
> >          <parameter name="aHomeOrEnd"/>
> >          <body>
> > +        <![CDATA[
> > +          const nsIWebBrowserPrint = Components.interfaces.nsIWebBrowserPrint;
> > +          let navType, pageNum;
> 
> pageNum doesn't seem to ever be used

Good eye! Fixed.

> 
> @@ +188,5 @@
> > +            pageNum = this.mPageTextBox.valueNumber;
> > +          } else {
> > +            // We're going to a specific page (aPageNum)
> > +            navType = nsIWebBrowserPrint.PRINTPREVIEW_GOTO_PAGENUM;
> > +            pageNum = aPageNum;
> 
> this.mPageTextBox.value probably
> 

Fixed.

> ::: toolkit/components/printing/content/printUtils.js
> @@ +137,3 @@
> >      }
> > +
> > +    let browser = this.getBrowserForContentWindow(aWindow);
> 
> I really dislike this. Any reason we can't just have the caller pass in the
> browser as well as the window?
> 

Done.

> @@ +309,5 @@
> > +
> > +  getBrowserForContentWindow(aWindow) {
> > +    let browser;
> > +
> > +    if (this.usingRemoteTabs) {
> 
> You could just test if the window is a CPOW or not.
> 

Done.

> @@ +421,5 @@
> > +    let mm = ppBrowser.messageManager;
> > +    let printSettings = this.getPrintSettings();
> > +    mm.sendAsyncMessage("Printing:Preview:Enter", null, {
> > +      printSettings: printSettings,
> > +      contentWindow: this._sourceBrowser.contentWindowAsCPOW,
> 
> I didn't see an answer to this, how does this cope with printing
> inner-frames?

This is for print preview - and we cannot print preview inner frames.

Please note that with this patch, printing and print preview is busted in the view source window. This is because, for some reason, I cannot get a message manager for the print preview browser. If it's all the same to you, I'd like to fix that in a follow-up.

Comment 11

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

Thanks for going through that, Mossop. Drinks are on me.

remote:   https://hg.mozilla.org/integration/fx-team/rev/4e5999f39bb7

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/908b844c13c8 for bc1 orange on at least Windows 8 x64:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1012206&repo=fx-team

Comment 13

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

Attachment: Make PrintUtils and printPreviewBindings.xml more e10s friendly (r=Mossop)

Comment 14

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

So the problem is that in layout/printing/nsPrintData's onEndPrinting[1], two STATE_STOP web progress listener state changes are fired. For Windows, in embedding/components/printingui/win/nsPrintingPromptService.cpp[2], we set mWebProgressListener to nullptr after the first STATE_STOP. Subsequent listener events cause the nsIWebProgressListener methods to return NS_ERROR_FAILURE.

I suspect we were always returning NS_ERROR_FAILURE, and we just didn't care. The fact that this patch causes us to glue things together with Javascript means that when things return NS_ERROR_FAILURE, we throw an exception, which our test handler cares about.

So I've added some code to remove the state / progress message listeners after the first STATE_STOP event. This will all get torn out once bug 1088061 is fixed.

Comment 15

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

Ugh, footnotes:

[1]: http://hg.mozilla.org/mozilla-central/file/a255a234946e/layout/printing/nsPrintData.cpp#l113
[2]: http://hg.mozilla.org/mozilla-central/file/a255a234946e/embedding/components/printingui/win/nsPrintingPromptService.cpp#l315

Comment 16

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

Try push with the patch from bug 1082579:

https://tbpl.mozilla.org/?tree=Try&rev=f1aa062a86f9

Comment 17

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

Landed with test fix on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/24a95294f600

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24a95294f600

Comment 19

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1082575.html