Closed Bug 1329881 Opened 8 years ago Closed 7 years ago

[Mortar] [Windows] Create an interface to send PDF file name and printing setting to chrome process, then configure print job

Categories

(Core :: Printing: Output, defect)

Product:
Core
Component:
Printing: Output
Version:
45 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: fatseng, Assigned: fatseng)

References

Details

(Whiteboard: [pdfium-viewer-mvpscope] )

Attachments

(3 files, 1 obsolete file)

Bug 1329881 - part2: configure Windows printing job
59 bytes, text/x-review-board-request
brsun
: feedback+
lochang
: feedback+
Details
Bug 1329881 - part1: send PDF file path and printing setting to Chrome process
59 bytes, text/x-review-board-request
brsun
: feedback+
lochang
: feedback+
Details
Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames
59 bytes, text/x-review-board-request
brsun
: feedback+
lochang
: feedback+
Details
No description provided.
Summary: [Mortar] Implement converting PDF to EMF for Windows → [Mortar] Implement converting PDF to EMF and printing for Windows
Blocks: 1269760
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Depends on: 1325032
Since we need PDFium to convert PDF to EMF, we are discussing which process is suitable to do the conversion. See Bug 1325032.
No longer depends on: 1325032
Depends on: 1325032
Summary: [Mortar] Implement converting PDF to EMF and printing for Windows → [Mortar] [meta] Implement converting PDF to EMF and printing for Windows
Depends on: 1339716
Depends on: 1339735
Depends on: 1341509
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process Create a interface, unblock javascript implementation
Attachment #8840715 - Flags: review?(jwatt)
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job In nsPrintEngine::PrintPDF, I utilize printPromptService to setup print session. And create a new function to deliver pdf file path to Chrome process.
Attachment #8840694 - Flags: review?(jwatt)
Summary: [Mortar] [meta] Implement converting PDF to EMF and printing for Windows → [Mortar] Implement converting PDF to EMF and printing for Windows
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames Once receive PDF file path, start to configure printing job, and create a runnable thread to deal with printing. After finishing printing, send a message to inform Content process. I will deal with printing in another patch.
Attachment #8840751 - Flags: review?(jwatt)
Whiteboard: [pdfium-viewer-mvpscope]
Blocks: 1302489
Per update from Farmer, there will be Vidyo discussion with jwatt to walk through patches.
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process https://reviewboard.mozilla.org/r/115148/#review121934 I don't think it's worth having this as a separate patch. Was there a reason you didn't just combine this with part 2?
Attachment #8840715 - Flags: review?(jwatt)
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job https://reviewboard.mozilla.org/r/115124/#review121922 ::: layout/base/nsDocumentViewer.cpp:3812 (Diff revision 2) > + // Let the user know we are not ready to print. > + rv = NS_ERROR_NOT_AVAILABLE; > + > + if (mPrintEngine) { > + mPrintEngine->FirePrintingErrorEvent(rv); > + } > + > + return rv; Depending on how long a second print may be blocked, this may be very annoying to users who want to print multiple documents one after another. Can you open a follow up bug (that depends on this bug) to implement some sort of queue? ::: layout/base/nsDocumentViewer.cpp:3850 (Diff revision 2) > + mPrintEngine = nullptr; > + return rv; > + } > + } > + > + mAutoBeforeAndAfterPrint = autoBeforeAndAfterPrint; Please add a comment like we have elsewhere when we store in mAutoBeforeAndAfterPrint to note why we're storing this. ::: layout/printing/ipc/PRemotePrintJob.ipdl:47 (Diff revision 2) > > // Report a status change to listeners in the parent process. > async StatusChange(nsresult aStatus); > > + // Send PDF file name to print > + async ProcessPDF(nsCString aPDFFileName); Can we call this "PrintPDF"? "Process" is very generic and doesn't give much of an idea what the method does. ::: layout/printing/ipc/PRemotePrintJob.ipdl:58 (Diff revision 2) > > // Inform the child that the latest page has been processed remotely. > async PageProcessed(); > > + // Inform the child that PDF has been printed remotely. > + async PdfProcessed(); And here call it "DonePrintingPDF"? ::: layout/printing/ipc/RemotePrintJobParent.cpp:149 (Diff revision 2) > } > > mozilla::ipc::IPCResult > +RemotePrintJobParent::RecvProcessPDF(const nsCString& aFileName) > +{ > + // XXX TODO: call this in correct place where finish printing In future it would be good to note "fixed in a later patch in the patch series". ::: layout/printing/nsPrintEngine.h:284 (Diff revision 2) > int32_t mLoadCounter; > bool mDidLoadDataForPrinting; > bool mIsDestroying; > bool mDisallowSelectionPrint; > > + nsCOMPtr<nsIDeviceContextSpec> mDevspec; Please add a comment explaining what this is for, and when it is used, and when it is not used. Presumably it is only used when we print a PDF _specifically_ via a PrintPDF is call? ::: layout/printing/nsPrintEngine.cpp:769 (Diff revision 2) > //--------------------------------------------------------------------------------- > NS_IMETHODIMP > +nsPrintEngine::PrintPDF(const nsAString& aPDFFileName, > + nsIPrintSettings* aPrintSettings) > +{ > + SetIsPrinting(true); Add a MOZ_ASSERT(XRE_IsContentProcess()) at the top of this file. ::: layout/printing/nsPrintEngine.cpp:780 (Diff revision 2) > + printSession = do_CreateInstance("@mozilla.org/gfx/printsession;1", &rv); > + NS_ENSURE_SUCCESS(rv, rv); > + aPrintSettings->SetPrintSession(printSession); > + } > + > + bool printSilently = true; It doesn't look like you need this variable. ::: toolkit/components/printingui/ipc/PrintingParent.cpp:124 (Diff revision 2) > - NS_ENSURE_SUCCESS(rv, rv); > - > rv = mPrintSettingsSvc->DeserializeToPrintSettings(aData, settings); > NS_ENSURE_SUCCESS(rv, rv); > > - rv = settings->SetPrintSilent(printSilently); > + // We want to use the print silently setting from the child. Please add another comment line here explaining why the GetPrintSilent() call has to happen after the DeserializeToPrintSettings() call. ::: widget/android/nsDeviceContextAndroid.h:31 (Diff revision 2) > int32_t aStartPage, > int32_t aEndPage) override; > NS_IMETHOD EndDocument() override; > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > + NS_IMETHOD SendPDF(const nsAString& aPDFFileName) override { return NS_OK; } Probably make the body of the no-op overrides like this: NS_NOTYETIMPLEMENTED("We don't need to implement this for Android because XXX"); return NS_ERROR_NOT_IMPLEMENTED; ::: widget/cocoa/nsDeviceContextSpecX.h:34 (Diff revision 2) > }; > NS_IMETHOD EndPage() override { > return NS_OK; > }; > + NS_IMETHOD SendPDF(const nsAString& aPDFFileName) override { > + return NS_OK; Same. ::: widget/gtk/nsDeviceContextSpecG.h:43 (Diff revision 2) > const nsAString& aPrintToFileName, > int32_t aStartPage, int32_t aEndPage) override; > NS_IMETHOD EndDocument() override; > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > + NS_IMETHOD SendPDF(const nsAString& aPDFFileName) override { return NS_OK; } Same. ::: widget/nsIDeviceContextSpec.h:81 (Diff revision 2) > > NS_IMETHOD EndDocument() = 0; > NS_IMETHOD AbortDocument() { return EndDocument(); } > NS_IMETHOD BeginPage() = 0; > NS_IMETHOD EndPage() = 0; > + NS_IMETHOD SendPDF(const nsAString& aPDFFileName) = 0; Call this "PrintPDF"?
Attachment #8840694 - Flags: review?(jwatt)
Attachment #8846492 - Flags: review?(jwatt)
Summary: [Mortar] Implement converting PDF to EMF and printing for Windows → [Mortar] [Windows] Create an interface to send PDF file name and printing setting to chrome process, then configure print job
Blocks: pdf-printing
No longer blocks: 1269760, 1302489
Blocks: 1345789
Attachment #8846492 - Attachment is obsolete: true
Attachment #8846492 - Flags: review?(jwatt)
Attachment #8840715 - Flags: review?(jwatt)
Attachment #8840694 - Flags: review?(jwatt)
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames In order to assign printJobName, nsPrintOptionsX::SerializeToPrintData calls EnumerateDocumentNames while showing print dialogue in Mac. Calling PrintPDF causes crash because nsPrintData wasn't allocated, it hits null point in nsPrintEngine::EnumerateDocumentNames. So, I add check here and call mDocument->GetTitle() instead of calling nsPrintEngine::EnumerateDocumentNames.
Attachment #8840751 - Flags: review?(jwatt)
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job https://reviewboard.mozilla.org/r/115124/#review121922 > Depending on how long a second print may be blocked, this may be very annoying to users who want to print multiple documents one after another. Can you open a follow up bug (that depends on this bug) to implement some sort of queue? Every nsDocumentViewer owns its nsPrintEngine. Therefore, it dosen't block user to print other tabs. Per discussed with jwatt, althrough it doesn't look important, it is still worth to create a bug to investigate. Bug 1351206 - Investigate how to implement some queues to deal with multiple documents printing in one tab
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames https://reviewboard.mozilla.org/r/115176/#review121946 ::: widget/windows/nsDeviceContextSpecWin.cpp:321 (Diff revision 1) > + if (result > 0) { > + NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsDeviceContextSpecWin::PDFPrintjob)); > + } else { > + return NS_ERROR_FAILURE; > + } > + > + return NS_OK; Make this: if (result <= 0 { return NS_ERROR_FAILURE; } return NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsDeviceContextSpecWin::PDFPrintjob)); ::: layout/base/nsDocumentViewer.cpp:4229 (Diff revision 3) > NS_ENSURE_ARG(aCount); > NS_ENSURE_ARG_POINTER(aResult); > - NS_ENSURE_TRUE(mPrintEngine, NS_ERROR_FAILURE); > > + // Check nsPrintData here because nsPrintData wasn't allocated while calling PrintPDF > + if (mPrintEngine && mPrintEngine->IsThereAPrintData()) { Please avoid using the pattern: if (...) { return; } else { ... } Because of the return in the |if| you do not need an |else|. ::: layout/printing/nsPrintEngine.h:210 (Diff revision 3) > void SetDisallowSelectionPrint(bool aDisallowSelectionPrint) > { > mDisallowSelectionPrint = aDisallowSelectionPrint; > } > > + bool IsThereAPrintData() Call this HavePrintData.
(In reply to Jonathan Watt [:jwatt] from comment #22) > Make this: > > if (result <= 0 { > return NS_ERROR_FAILURE; > } > return NS_DispatchToCurrentThread(NewRunnableMethod(this, > &nsDeviceContextSpecWin::PDFPrintjob)); Ignore this comment. Mozreview got confused and published old comments.
Attachment #8840751 - Flags: review?(jwatt)
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process https://reviewboard.mozilla.org/r/115148/#review139866 ::: layout/base/nsDocumentViewer.cpp:3832 (Diff revision 4) > +nsDocumentViewer::PrintPDF(const nsAString& aPDFFileName, > + nsIPrintSettings* aPrintSettings) > +{ > + if (!aPrintSettings) { > + PR_PL(("PrintSettings should not be NULL")); > + return NS_ERROR_FAILURE; Could we just assert this? Or are you not confident that this will never be null for some reason? ::: layout/base/nsDocumentViewer.cpp:3839 (Diff revision 4) > + > + // Printing XUL documents is not supported. > + nsCOMPtr<nsIXULDocument> xulDoc(do_QueryInterface(mDocument)); > + if (xulDoc) { > + PR_PL(("Printing XUL documents is not supported")); > + return NS_ERROR_FAILURE; This seems to come from nsDocumentViewer::Print. We don't need to worry about this though, since we're not printing *this* document, we're printing the specified PDF file. Please remove this. ::: layout/base/nsDocumentViewer.cpp:3842 (Diff revision 4) > + if (xulDoc) { > + PR_PL(("Printing XUL documents is not supported")); > + return NS_ERROR_FAILURE; > + } > + > + nsresult rv = NS_ERROR_FAILURE; Seems like this could be NS_OK. ::: layout/base/nsDocumentViewer.cpp:3862 (Diff revision 4) > + } > + > + // Dispatch 'beforeprint' event and ensure 'afterprint' will be dispatched: > + MOZ_ASSERT(!mAutoBeforeAndAfterPrint, > + "We don't want to dispatch nested beforeprint/afterprint"); > + nsAutoPtr<AutoPrintEventDispatcher> autoBeforeAndAfterPrint( This doesn't look correct. The print events should notify that *this* document is printing, not that some PDF is printing. I think instead this code should be replaced by a comment saying that we don't dispatch these events for that reason. What do you think? ::: layout/base/nsDocumentViewer.cpp:3866 (Diff revision 4) > + "We don't want to dispatch nested beforeprint/afterprint"); > + nsAutoPtr<AutoPrintEventDispatcher> autoBeforeAndAfterPrint( > + new AutoPrintEventDispatcher(mDocument)); > + NS_ENSURE_STATE(!GetIsPrinting()); > + > + if (!mPrintEngine) { We would now have this code duplicated in three places. Can you create an EnsurePrintEngineInitialized() method and call that here? (Pass in the document, and return a boolean - if the returned value is false, we would return here and at the other callers.) ::: layout/printing/ipc/RemotePrintJobParent.cpp:149 (Diff revision 4) > } > > mozilla::ipc::IPCResult > +RemotePrintJobParent::RecvPrintPDF(const nsString& aFileName) > +{ > + // XXX: call this in correct place where finish printing, Make this: // TODO: Actually print the PDF before we call SendDonePrintingPDF() // Fixed in a later patch. ::: layout/printing/nsPrintEngine.cpp:774 (Diff revision 4) > return NS_OK; > } > > //--------------------------------------------------------------------------------- > NS_IMETHODIMP > +nsPrintEngine::PrintPDF(const nsAString& aPDFFileName, I think this should come after nsPrintEngine::PrintPreview for consistent ordering (and because it's not our main print method). ::: layout/printing/nsPrintEngine.cpp:777 (Diff revision 4) > //--------------------------------------------------------------------------------- > NS_IMETHODIMP > +nsPrintEngine::PrintPDF(const nsAString& aPDFFileName, > + nsIPrintSettings* aPrintSettings) > +{ > + MOZ_ASSERT(XRE_IsContentProcess()); If we don't call this when e10s is turned off, it would be useful to have a comment here explaining what the code flow looks like in that case. ::: layout/printing/nsPrintEngine.cpp:799 (Diff revision 4) > + NS_ENSURE_TRUE(domWin, NS_ERROR_FAILURE); > + nsCOMPtr<nsIPrintingPromptService> printPromptService(do_GetService(kPrintingPromptService)); > + if (printPromptService) { > + rv = printPromptService->ShowPrintDialog(domWin, wbp, aPrintSettings); > + if (rv == NS_ERROR_NOT_IMPLEMENTED) { > + // This means the Dialog service was there, Seems like this should be "Print Prompt Service", not "Dialog Service", no? ::: layout/printing/nsPrintEngine.cpp:802 (Diff revision 4) > + rv = printPromptService->ShowPrintDialog(domWin, wbp, aPrintSettings); > + if (rv == NS_ERROR_NOT_IMPLEMENTED) { > + // This means the Dialog service was there, > + // but they choose not to implement this dialog and > + // are looking for default behavior from the toolkit > + rv = NS_OK; Actually, my understanding of the code is different. As far as I can tell your are referring to the NS_ERROR_NOT_IMPLEMENTED comments in: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/browser/nsIPrintingPromptService.idl But I think those comments refer to internal handling within toolkit, and you should never be exposed to NS_ERROR_NOT_IMPLEMENTED here. So I don't think you need this code. ::: toolkit/components/printingui/ipc/PrintingParent.cpp:126 (Diff revision 4) > rv = mPrintSettingsSvc->DeserializeToPrintSettings(aData, settings); > NS_ENSURE_SUCCESS(rv, rv); > > - rv = settings->SetPrintSilent(printSilently); > + // |DeserializeToPrintSettings| takes a PrintData which is sent from child over IPC, > + // and populates a pre-existing nsIPrintSettings with the data from PrintData. > + // Call GetPrintSilent here to get the setting which is assigned from the child. It looks like you're fixing broken code here that is not related to your patch. Can you make this a separate patch, please? ::: widget/android/nsDeviceContextAndroid.cpp:88 (Diff revision 4) > } > + > +NS_IMETHODIMP > +nsDeviceContextSpecAndroid::PrintPDF(const nsAString& aPDFFileName) > +{ > + NS_NOTYETIMPLEMENTED("We don't need to implement this for Android because Fennec just download PDF instead of browsing it."); If we don't plan to implement this, then use: MOZ_ASSERT_UNREACHABLE("On Android we download PDFs, so they're never printed by Firefox"); ::: widget/nsIDeviceContextSpec.h:81 (Diff revision 4) > > NS_IMETHOD EndDocument() = 0; > NS_IMETHOD AbortDocument() { return EndDocument(); } > NS_IMETHOD BeginPage() = 0; > NS_IMETHOD EndPage() = 0; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFileName) = 0; The previous functions are used together, whereas this function is not used with them. I'd put a blank line between this new function and the others above it. (Here and at all the override declarations.) I'm guessing this argument is actually a file path, and not just a name - if so, can you also rename the argument to aPDFFilePath? (Here and in all the override declarations and implementations.)
Attachment #8840715 - Flags: review?(jwatt)
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job https://reviewboard.mozilla.org/r/115124/#review136256 ::: layout/printing/ipc/RemotePrintJobParent.h:82 (Diff revision 3) > > nsCOMPtr<nsIPrintSettings> mPrintSettings; > RefPtr<nsDeviceContext> mPrintDeviceContext; > UniquePtr<PrintTranslator> mPrintTranslator; > nsCOMArray<nsIWebProgressListener> mPrintProgressListeners; > + nsCOMPtr<nsIDeviceContextSpec> mPDFDeviceContextSpec; Rather than having a new nsIDeviceContextSpec, could we not pass the PrintPDF call through mPrintDeviceContext to it's mDeviceContextSpec? ::: widget/windows/nsDeviceContextSpecWin.h (Diff revision 3) > NS_IMETHOD EndDocument() override { return NS_OK; } > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > - NS_IMETHOD PrintPDF(const nsAString& aPDFFileName) override { > + NS_IMETHOD PrintPDF(const nsAString& aPDFFileName) override; > - return NS_ERROR_NOT_IMPLEMENTED; > - } I'm not clear on why we're implementing PrintPDF for Windows. Bug 1345786 seems to imply that we will not pass a PDF to the parent process like this, but actually pass EMF files. Can you clarify what's going on? ::: widget/windows/nsDeviceContextSpecWin.cpp:94 (Diff revision 3) > mDriverName = nullptr; > mDeviceName = nullptr; > mDevMode = nullptr; > - > + mDC = nullptr; > + mRemotePrintJobParent = nullptr; > + mPDFFileName.Truncate(); This should start out empty. Why do you need the Truncate() call? ::: widget/windows/nsDeviceContextSpecWin.cpp:324 (Diff revision 3) > + int result = -1; > + result = ::StartDocW(mDC, &di); > + if (result > 0) { > + NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsDeviceContextSpecWin::PDFPrintjob)); > + } else { > + return NS_ERROR_FAILURE; Make this: if (result <= 0 { return NS_ERROR_FAILURE; } return NS_DispatchToCurrentThread(NewRunnableMethod(this, &nsDeviceContextSpecWin::PDFPrintjob));
Attachment #8840694 - Flags: review?(jwatt)
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process https://reviewboard.mozilla.org/r/115148/#review139910 ::: toolkit/components/printingui/ipc/PrintingParent.cpp:128 (Diff revision 4) > > - rv = settings->SetPrintSilent(printSilently); > + // |DeserializeToPrintSettings| takes a PrintData which is sent from child over IPC, > + // and populates a pre-existing nsIPrintSettings with the data from PrintData. > + // Call GetPrintSilent here to get the setting which is assigned from the child. > + bool printSilently; > + rv = settings->GetPrintSilent(&printSilently); I should have been clearer as to the reasons in my comment, but we deliberately don't want to read the printSilent setting from the child. Otherwise a compromised child process can lie about the pref setting and cause printing to occur without interaction from the user. Printing and printer drivers have been a source of vulnerablities in the past. We read from the settings before this, because those settings can have been passed in by an addon and we check the pref below.
(In reply to Jonathan Watt [:jwatt] from comment #30) > Comment on attachment 8840715 [details] > Bug 1329881 - part1: send PDF file path and printing setting to Chrome > process > > https://reviewboard.mozilla.org/r/115148/#review139866 > > ::: layout/base/nsDocumentViewer.cpp:3862 > (Diff revision 4) > > + } > > + > > + // Dispatch 'beforeprint' event and ensure 'afterprint' will be dispatched: > > + MOZ_ASSERT(!mAutoBeforeAndAfterPrint, > > + "We don't want to dispatch nested beforeprint/afterprint"); > > + nsAutoPtr<AutoPrintEventDispatcher> autoBeforeAndAfterPrint( > > This doesn't look correct. The print events should notify that *this* > document is printing, not that some PDF is printing. I think instead this > code should be replaced by a comment saying that we don't dispatch these > events for that reason. What do you think? > Script listens the 'afterprint' event to remove the PDF file which is used for printing. So, we need this. In the future, we plan to replace it by promise. > ::: layout/printing/nsPrintEngine.cpp:777 > (Diff revision 4) > > //--------------------------------------------------------------------------------- > > NS_IMETHODIMP > > +nsPrintEngine::PrintPDF(const nsAString& aPDFFileName, > > + nsIPrintSettings* aPrintSettings) > > +{ > > + MOZ_ASSERT(XRE_IsContentProcess()); > > If we don't call this when e10s is turned off, it would be useful to have a > comment here explaining what the code flow looks like in that case. > You point out a big problem that I didn't implement printing PDF when e10s is turned off. I will implement it.
(In reply to Jonathan Watt [:jwatt] from comment #31) > Comment on attachment 8840694 [details] > Bug 1329881 - part2: configure Windows printing job > > https://reviewboard.mozilla.org/r/115124/#review136256 > > ::: layout/printing/ipc/RemotePrintJobParent.h:82 > (Diff revision 3) > > > > nsCOMPtr<nsIPrintSettings> mPrintSettings; > > RefPtr<nsDeviceContext> mPrintDeviceContext; > > UniquePtr<PrintTranslator> mPrintTranslator; > > nsCOMArray<nsIWebProgressListener> mPrintProgressListeners; > > + nsCOMPtr<nsIDeviceContextSpec> mPDFDeviceContextSpec; > > Rather than having a new nsIDeviceContextSpec, could we not pass the > PrintPDF call through mPrintDeviceContext to it's mDeviceContextSpec? > > ::: widget/windows/nsDeviceContextSpecWin.h > (Diff revision 3) > > NS_IMETHOD EndDocument() override { return NS_OK; } > > NS_IMETHOD BeginPage() override { return NS_OK; } > > NS_IMETHOD EndPage() override { return NS_OK; } > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFileName) override { > > + NS_IMETHOD PrintPDF(const nsAString& aPDFFileName) override; > > - return NS_ERROR_NOT_IMPLEMENTED; > > - } > > I'm not clear on why we're implementing PrintPDF for Windows. Bug 1345786 > seems to imply that we will not pass a PDF to the parent process like this, > but actually pass EMF files. Can you clarify what's going on? > As we talked on IRC. I pass PDF file path from the Content process to the Chrome process by this function. Once nsDeviceContextSpecWin's PrintPDF() is called. nsDeviceContextSpecWin passes the file path to the plugin process. Then, ask plugin process provide EMF to print page by page.
Attachment #8840694 - Flags: review?(jwatt) → feedback?(brsun)
Attachment #8840715 - Flags: review?(jwatt) → feedback?(brsun)
Attachment #8840751 - Flags: review?(jwatt) → feedback?(brsun)
Attachment #8840694 - Flags: feedback?(lochang)
Attachment #8840715 - Flags: feedback?(lochang)
Attachment #8840751 - Flags: feedback?(lochang)
Attachment #8840694 - Flags: review?(jwatt)
Attachment #8840694 - Flags: feedback?(lochang)
Attachment #8840694 - Flags: feedback?(brsun)
Attachment #8840715 - Flags: review?(jwatt)
Attachment #8840715 - Flags: feedback?(lochang)
Attachment #8840715 - Flags: feedback?(brsun)
Attachment #8840751 - Flags: review?(jwatt)
Attachment #8840751 - Flags: feedback?(lochang)
Attachment #8840751 - Flags: feedback?(brsun)
Attachment #8840715 - Flags: feedback?(lochang)
Attachment #8840715 - Flags: feedback?(brsun)
Attachment #8840694 - Flags: feedback?(lochang)
Attachment #8840694 - Flags: feedback?(brsun)
Attachment #8840751 - Flags: feedback?(lochang)
Attachment #8840751 - Flags: feedback?(brsun)
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames https://reviewboard.mozilla.org/r/115176/#review141922 ::: layout/base/nsDocumentViewer.cpp:4216 (Diff revision 8) > NS_ENSURE_ARG(aCount); > NS_ENSURE_ARG_POINTER(aResult); > - NS_ENSURE_TRUE(mPrintEngine, NS_ERROR_FAILURE); > > + // Check nsPrintData here because nsPrintData wasn't allocated while calling PrintPDF > + if (!mPrintEngine || !mPrintEngine->HavePrintData()) { Is there any chance that printEngine exists but printData doesn't? If no, then we don't have to double check whether printData exists?
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames https://reviewboard.mozilla.org/r/115176/#review141922 > Is there any chance that printEngine exists but printData doesn't? If no, then we don't have to double check whether printData exists? Yes, we hit this condition when calling PrintPDF().
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process https://reviewboard.mozilla.org/r/115148/#review141944 Only some wording suggestions and a question. Please see the comments below, thanks. ::: layout/base/nsDocumentViewer.cpp:3847 (Diff revision 7) > + NS_ENSURE_STATE(mPrintEngine); > + > + // if we are printing another URL, then exit > + // the reason we check here is because this method can be called while > + // another is still in here (the printing dialog is a good example). > + // the only time we can print more than one job at a time is the regression tests Return here if we are printing PDF on the other tab. PrintPDF might be called while the other tab is still printing. We can only print more than one job at a time in regression test. ::: layout/base/nsDocumentViewer.cpp:3849 (Diff revision 7) > + // if we are printing another URL, then exit > + // the reason we check here is because this method can be called while > + // another is still in here (the printing dialog is a good example). > + // the only time we can print more than one job at a time is the regression tests > + if (GetIsPrinting()) { > + // Let the user know we are not ready to print. Notify users that we are not ready to print. ::: layout/base/nsDocumentViewer.cpp:3856 (Diff revision 7) > + return NS_ERROR_NOT_AVAILABLE; > + } > + > + // Dispatch 'beforeprint' event and ensure 'afterprint' will be dispatched. > + // Script listens the 'afterprint' event to remove the PDF file which is used > + // for printing. In the future, we will replace this event by promise. Dispatch 'beforeprint' event and ensure 'afterprint' will be dispatched. Script should listen to 'afterprint' event to remove PDF files used for printing. We will replace dispatching event by resolving promise. ::: layout/printing/ipc/PRemotePrintJob.ipdl:46 (Diff revision 7) > long aMaxTotalProgress); > > // Report a status change to listeners in the parent process. > async StatusChange(nsresult aStatus); > > + // Send PDF file to printer Send PDF file path to printer ::: layout/printing/nsPrintEngine.h:289 (Diff revision 7) > int32_t mLoadCounter; > bool mDidLoadDataForPrinting; > bool mIsDestroying; > bool mDisallowSelectionPrint; > > + // This is only used when we print a PDF via |PrintPDF|. This -> mDevspec ::: toolkit/components/browser/nsIWebBrowserPrint.idl:153 (Diff revision 7) > */ > void exitPrintPreview(); > > + /** > + * Show print dialog and setup a printing IPC between the Chrome process and > + * the Content process Show the print dialog and setup a printing IPC between the chrome process and the content process ::: toolkit/components/browser/nsIWebBrowserPrint.idl:158 (Diff revision 7) > + * the Content process > + * > + * @param aThePrintSettings - Printer Settings for the print job > + * @return void > + */ > + void showPrintDialog(in nsIPrintSettings aThePrintSettings); Maybe we have to think of a name that cover constructing IPC and showing the print dialog?
Attachment #8840715 - Flags: feedback?(lochang) → feedback+
Attachment #8840751 - Flags: feedback?(lochang) → feedback+
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job https://reviewboard.mozilla.org/r/115124/#review142298 ::: layout/printing/ipc/RemotePrintJobParent.cpp:267 (Diff revision 8) > } > > void > RemotePrintJobParent::ActorDestroy(ActorDestroyReason aWhy) > { > + mPDFDeviceContextSpec = nullptr; Why do we only have to set mPDFDeviceContextSpec to nullptr but not other member variables such as mPrintSettings?
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job Only one question as comment 47.
Attachment #8840694 - Flags: feedback?(lochang) → feedback+
Comment on attachment 8840715 [details] Bug 1329881 - part1: send PDF file path and printing setting to Chrome process https://reviewboard.mozilla.org/r/115148/#review142918 ::: layout/base/nsDocumentViewer.cpp:3831 (Diff revision 7) > #ifdef NS_PRINTING > > NS_IMETHODIMP > +nsDocumentViewer::ShowPrintDialog(nsIPrintSettings* aPrintSettings) > +{ > + MOZ_ASSERT(aPrintSettings && mDocument); Suggest to use individual assertions for null checking, unless this logical-AND (&&) operation does have its logical meanings at this line. ::: layout/base/nsDocumentViewer.cpp:3851 (Diff revision 7) > + // another is still in here (the printing dialog is a good example). > + // the only time we can print more than one job at a time is the regression tests > + if (GetIsPrinting()) { > + // Let the user know we are not ready to print. > + mPrintEngine->FirePrintingErrorEvent(NS_ERROR_NOT_AVAILABLE); > + return NS_ERROR_NOT_AVAILABLE; I guess the intention here is to use the same value (i.e. NS_ERROR_NOT_AVAILABLE) for firing the error event and for the return value. If this is the case, how about using one variable to keep this value to enforce this intention just like what nsDocumentViewer::Print(...) does? ::: layout/base/nsDocumentViewer.cpp:4353 (Diff revision 7) > + > + mPrintEngine = new nsPrintEngine(); > + nsresult rv = mPrintEngine->Initialize(this, mContainer, aDocument, > + float(mDeviceContext->AppUnitsPerCSSInch()) / > + float(mDeviceContext->AppUnitsPerDevPixel()) / > + mPageZoom, How about using one float variable with a proper name to store this DPI value locally? ::: layout/printing/nsPrintEngine.cpp:3137 (Diff revision 7) > > return true; > } > > +bool > +nsPrintEngine::DonePrintingPDF() The return value is always true. The return value is not used by caller either. Do we really need any return value for this function? ::: layout/printing/nsPrintEngine.cpp:3139 (Diff revision 7) > } > > +bool > +nsPrintEngine::DonePrintingPDF() > +{ > + PR_PL(("****** In DV::DonePrintingPDF \n")); I suppose this kind of logs would be symmetric. How about adding corresponding messages as well before printing PDF files to have a better matching? ::: toolkit/components/browser/nsIWebBrowserPrint.idl:158 (Diff revision 7) > + * the Content process > + * > + * @param aThePrintSettings - Printer Settings for the print job > + * @return void > + */ > + void showPrintDialog(in nsIPrintSettings aThePrintSettings); If it is possible to digest a smaller function to construct PRemotePrintJob, probably we can save our burden to re-design our ShowPrintDialog. ::: toolkit/components/browser/nsIWebBrowserPrint.idl:166 (Diff revision 7) > + * Print the specified PDF file > + * > + * @param aPDFFilePath - file path > + * @return void > + */ > + void printPDF(in AString aPDFFilePath); Base on the comment on the previous function, if we really can have a function to the construct IPC which we need, I would suggest: - to pass the nsIPrintSettings from JavaScript back to Gecko again through this function; and - to create one nsIPrintSession and hold it locally; and - to construct PRemotePrintJob IPC (by the newly created function) within this function; and - to create nsIDeviceContextSpec locally to avoid one additional member variable. ::: widget/cocoa/nsDeviceContextSpecX.mm:243 (Diff revision 7) > > +NS_IMETHODIMP nsDeviceContextSpecX::PrintPDF(const nsAString& aPDFFilePath) > +{ > + NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT; > + > + NS_NOTYETIMPLEMENTED("Doesn't implement yet, please write me"); NS_NOTYETIMPLEMENTED("nsDeviceContextSpecX::PrintPDF"); ::: widget/gtk/nsDeviceContextSpecG.cpp:352 (Diff revision 7) > } > return NS_OK; > } > +NS_IMETHODIMP nsDeviceContextSpecGTK::PrintPDF(const nsAString& aPDFFilePath) > +{ > + NS_NOTYETIMPLEMENTED("Doesn't implement yet, please write me"); NS_NOTYETIMPLEMENTED("nsDeviceContextSpecGTK::PrintPDF"); ::: widget/windows/nsDeviceContextSpecWin.h:36 (Diff revision 7) > int32_t aEndPage) override { return NS_OK; } > NS_IMETHOD EndDocument() override { return NS_OK; } > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) override { NS_NOTYETIMPLEMENTED("nsDeviceContextSpecWin::PrintPDF"); By the way, why the implementation for Windows exists in the header file, but the implementation for others exists in the source file? Suggest using the same style except for special cases.
Comment on attachment 8840694 [details] Bug 1329881 - part2: configure Windows printing job https://reviewboard.mozilla.org/r/115124/#review142940 ::: layout/printing/ipc/RemotePrintJobChild.cpp:78 (Diff revision 8) > RemotePrintJobChild::RecvDonePrintingPDF() > { > MOZ_ASSERT(mPrintEngine); > > mPrintEngine->DonePrintingPDF(); > + Unused << SendFinalizePrint(); I guess this probably belongs to the previous patch. ::: layout/printing/ipc/RemotePrintJobParent.h:82 (Diff revision 8) > > nsCOMPtr<nsIPrintSettings> mPrintSettings; > RefPtr<nsDeviceContext> mPrintDeviceContext; > UniquePtr<PrintTranslator> mPrintTranslator; > nsCOMArray<nsIWebProgressListener> mPrintProgressListeners; > + nsCOMPtr<nsIDeviceContextSpec> mPDFDeviceContextSpec; mPDFDeviceContextSpec is only used in RemotePrintJobParent::RecvPrintPDF(). Do we need to save this nsIDeviceContextSpec as a member variable? ::: widget/android/nsDeviceContextAndroid.h:33 (Diff revision 8) > NS_IMETHOD EndDocument() override; > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) override; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr Suggest not to use default values if it is possible. ::: widget/cocoa/nsDeviceContextSpecX.h:22 (Diff revision 8) > > nsDeviceContextSpecX(); > > - NS_IMETHOD Init(nsIWidget *aWidget, nsIPrintSettings* aPS, bool aIsPrintPreview) override; > + NS_IMETHOD Init(nsIWidget *aWidget, > + nsIPrintSettings* aPS, > + bool aIsPrintPreview) override; Please keep irrelevant lines untouched. ::: widget/cocoa/nsDeviceContextSpecX.h:37 (Diff revision 8) > NS_IMETHOD EndPage() override { > return NS_OK; > }; > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) override; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr Suggest not to use default values if it is possible. ::: widget/gtk/nsDeviceContextSpecG.h:36 (Diff revision 8) > NS_DECL_ISUPPORTS > > virtual already_AddRefed<PrintTarget> MakePrintTarget() final; > > - NS_IMETHOD Init(nsIWidget *aWidget, nsIPrintSettings* aPS, > + NS_IMETHOD Init(nsIWidget *aWidget, > + nsIPrintSettings* aPS, Please keep irrelevant lines untouched. ::: widget/gtk/nsDeviceContextSpecG.h:46 (Diff revision 8) > NS_IMETHOD EndDocument() override; > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) override; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr Suggest not to use default values if it is possible. ::: widget/nsDeviceContextSpecProxy.h:34 (Diff revision 8) > { > public: > NS_DECL_ISUPPORTS > > - NS_IMETHOD Init(nsIWidget* aWidget, nsIPrintSettings* aPrintSettings, > + NS_IMETHOD Init(nsIWidget* aWidget, > + nsIPrintSettings* aPrintSettings, Please keep irrelevant lines untouched. ::: widget/nsDeviceContextSpecProxy.h:59 (Diff revision 8) > NS_IMETHOD BeginPage() final; > > NS_IMETHOD EndPage() final; > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) final; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr Suggest not to use default values if it is possible. ::: widget/nsIDeviceContextSpec.h:87 (Diff revision 8) > NS_IMETHOD AbortDocument() { return EndDocument(); } > NS_IMETHOD BeginPage() = 0; > NS_IMETHOD EndPage() = 0; > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) = 0; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr) = 0; Suggest not to use default values if it is possible. ::: widget/windows/nsDeviceContextSpecWin.h:37 (Diff revision 8) > NS_IMETHOD EndDocument() override { return NS_OK; } > NS_IMETHOD BeginPage() override { return NS_OK; } > NS_IMETHOD EndPage() override { return NS_OK; } > > - NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath) override { > - return NS_ERROR_NOT_IMPLEMENTED; > + NS_IMETHOD PrintPDF(const nsAString& aPDFFilePath, > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr Suggest not to use default values if it is possible. ::: widget/windows/nsDeviceContextSpecWin.h:42 (Diff revision 8) > - return NS_ERROR_NOT_IMPLEMENTED; > - } > - > - NS_IMETHOD Init(nsIWidget* aWidget, nsIPrintSettings* aPS, bool aIsPrintPreview) override; > + mozilla::layout::RemotePrintJobParent* aRemotePrintJobParent = nullptr > + ) override; > + > + NS_IMETHOD Init(nsIWidget* aWidget, > + nsIPrintSettings* aPS, > + bool aIsPrintPreview) override; Please keep irrelevant lines untouched. ::: widget/windows/nsDeviceContextSpecWin.h:76 (Diff revision 8) > + HDC mDC; > > nsCOMPtr<nsIPrintSettings> mPrintSettings; > int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative; > + > + void PDFPrintjob(); PDFPrintJob ::: widget/windows/nsDeviceContextSpecWin.h:79 (Diff revision 8) > int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative; > + > + void PDFPrintjob(); > + > + nsString mPDFFilePath; > + mozilla::layout::RemotePrintJobParent* mRemotePrintJobParent; Suppose these two variables are not necessary be member variables. Passing them to PDFPrintjob() probably would be suffice. ::: widget/windows/nsDeviceContextSpecWin.cpp:315 (Diff revision 8) > + return NS_ERROR_FAILURE; > + } > + > + DOCINFOW di; > + di.cbSize = sizeof(di); > + di.lpszDocName = L"Mozilla Document"; Is this the proper document name? ::: widget/windows/nsDeviceContextSpecWin.cpp:326 (Diff revision 8) > + if (result <= 0) { > + return NS_ERROR_FAILURE; > + } > + > + return NS_DispatchToCurrentThread( > + NewRunnableMethod(this, &nsDeviceContextSpecWin::PDFPrintjob)); Passing aPDFFilePath and aRemotePrintJobParent in NewRunnableMethod() as well would save two member variables.
Comment on attachment 8840751 [details] Bug 1329881 - fix crash in Mac while calling EnumerateDocumentNames https://reviewboard.mozilla.org/r/115176/#review143006 ::: layout/base/nsDocumentViewer.cpp (Diff revision 8) > - char16_t*** aResult) > + char16_t*** aResult) > { > #ifdef NS_PRINTING > NS_ENSURE_ARG(aCount); > NS_ENSURE_ARG_POINTER(aResult); > - NS_ENSURE_TRUE(mPrintEngine, NS_ERROR_FAILURE); In what case this checking would fail? ::: layout/base/nsDocumentViewer.cpp:4215 (Diff revision 8) > #ifdef NS_PRINTING > NS_ENSURE_ARG(aCount); > NS_ENSURE_ARG_POINTER(aResult); > - NS_ENSURE_TRUE(mPrintEngine, NS_ERROR_FAILURE); > > + // Check nsPrintData here because nsPrintData wasn't allocated while calling PrintPDF Why not create a proper nsPrintData as well when PrintPDF is called? So that we can reuse the same normal flow here. ::: layout/base/nsDocumentViewer.cpp:4235 (Diff revision 8) > + if (docTitleStr.IsEmpty() && !docURLStr.IsEmpty()) { > + docTitleStr = docURLStr; > + } > + > + char16_t** array = (char16_t**) moz_xmalloc(sizeof(char16_t*)); > + if (!array) { Probably you don't need to check the return value of xmalloc(). If you would like to control the allocation failure here, malloc() would suffice.
Attachment #8840715 - Flags: feedback?(brsun) → feedback+
Attachment #8840694 - Flags: feedback?(brsun) → feedback+
Attachment #8840751 - Flags: feedback?(brsun) → feedback+
No longer blocks: pdf-printing
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: