Make PrintUtils and printPreviewBindings.xml e10s friendly

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

({dev-doc-needed})

unspecified
mozilla36
x86
All
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm3+)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
This looks like it will have to involve printPreviewBindings.xml as well.
Summary: Make PrintUtils e10s friendly → Make PrintUtils and printPreviewBindings.xml e10s friendly
(Assignee)

Comment 2

4 years ago
Created attachment 8505213 [details] [diff] [review]
[WIP] Make PrintUtils e10s friendly. r=?
(Assignee)

Updated

4 years ago
Attachment #8505213 - Attachment description: Make PrintUtils e10s friendly. r=? → [WIP] Make PrintUtils e10s friendly. r=?
(Assignee)

Updated

4 years ago
Attachment #8505213 - Flags: feedback?(dtownsend+bugmail)
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.
Attachment #8505213 - Flags: feedback?(dtownsend+bugmail) → feedback+
(Assignee)

Updated

4 years ago
Depends on: 1085537
(Assignee)

Comment 4

4 years ago
Created attachment 8510359 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?
(Assignee)

Updated

4 years ago
Attachment #8505213 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
Created attachment 8510379 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?
(Assignee)

Updated

4 years ago
Attachment #8510359 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
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.
Attachment #8510379 - Flags: review?(dtownsend+bugmail)
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 7

4 years ago
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?
Attachment #8510379 - Flags: review?(dtownsend+bugmail) → review-
(Assignee)

Comment 9

4 years ago
Created attachment 8511973 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly. r=?
(Assignee)

Updated

4 years ago
Attachment #8510379 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
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.
Attachment #8511973 - Flags: review?(dtownsend+bugmail)
Attachment #8511973 - Flags: review?(dtownsend+bugmail) → review+
(Assignee)

Updated

4 years ago
Depends on: 1089815
(Assignee)

Comment 11

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/4e5999f39bb7
Whiteboard: [fixed-in-fx-team]
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
Flags: needinfo?(mconley)
(Assignee)

Comment 13

4 years ago
Created attachment 8512718 [details] [diff] [review]
Make PrintUtils and printPreviewBindings.xml more e10s friendly (r=Mossop)
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8512718 - Attachment description: Make PrintUtils and printPreviewBindings.xml more e10s friendly → Make PrintUtils and printPreviewBindings.xml more e10s friendly (r=Mossop)
Attachment #8512718 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8511973 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 17

4 years ago
Landed with test fix on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/24a95294f600
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/24a95294f600
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
(Assignee)

Updated

4 years ago
Depends on: 1090995

Updated

4 years ago
See Also: → bug 1091126

Updated

4 years ago
Depends on: 1092811

Updated

4 years ago
Depends on: 1092826

Updated

4 years ago
Depends on: 1117936
(Assignee)

Updated

4 years ago
Depends on: 1136855
Depends on: 1141734

Updated

4 years ago
Depends on: 1144751
You need to log in before you can comment on or make changes to this bug.