Closed Bug 1082575 Opened 10 years ago Closed 10 years ago

Make PrintUtils and printPreviewBindings.xml e10s friendly

Categories

(Toolkit :: Printing, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m3+ ---

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

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.
This looks like it will have to involve printPreviewBindings.xml as well.
Summary: Make PrintUtils e10s friendly → Make PrintUtils and printPreviewBindings.xml e10s friendly
Attachment #8505213 - Attachment description: Make PrintUtils e10s friendly. r=? → [WIP] Make PrintUtils e10s friendly. r=?
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+
Depends on: 1085537
Attachment #8505213 - Attachment is obsolete: true
Attachment #8510359 - Attachment is obsolete: true
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)
Keywords: dev-doc-needed
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-
Attachment #8510379 - Attachment is obsolete: true
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+
Depends on: 1089815
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]
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)
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+
Attachment #8511973 - Attachment is obsolete: true
Whiteboard: [fixed-in-fx-team]
Whiteboard: [fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Depends on: 1090995
See Also: → 1091126
Depends on: 1092811
Depends on: 1092826
Depends on: 1117936
Depends on: 1136855
Depends on: 1141734
Depends on: 1144751
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: