Closed
Bug 1370488
Opened 7 years ago
Closed 7 years ago
Add support for having printing on Windows print via Skia PDF and PDFium
Categories
(Core :: Printing: Output, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: fatseng, Assigned: fatseng)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
No description provided.
Assignee | ||
Updated•7 years ago
|
Blocks: pdf-printing
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
How to test printing via Skia PDF:
1. Copy attachment 8875193 [details] to c:\
2. apply attachment 8875195 [details] [diff] [review]
3. add "ac_add_options --enable-skia-pdf" to mozconfig
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8875195 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8875193 -
Attachment is obsolete: true
Attachment #8875203 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
How to test printing via Skia PDF:
1. apply patches from Bug 1368948
2. apply attachment 8875675 [details] [diff] [review]
3. add "ac_add_options --enable-skia-pdf" to mozconfig
4. set the pref: print.print_via_pdf_encoder=skia-pdf
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8875675 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
[Build] - Pass
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c64d860e7e753c45f8c7f12bb06f91ddcecf27f8
[Test] - Fail
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32cf0d387f885615d6227262bcfdd4318fc0f9e
Failed in running browser_DownloadPDFSaver.js. I am looking at it.
https://treeherder.mozilla.org/logviewer.html#?job_id=105781489&repo=try&lineNumber=4171
Assignee | ||
Comment 9•7 years ago
|
||
Fix running browser_DownloadPDFSaver.js failure.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=444b2148d8b8d028c0ebc02975813029d573f68e
Updated•7 years ago
|
Summary: Print EMF → Enable PDF to EMF printing on Windows
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8876068 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
In this patch, I dispatch a runnable thread to convert PDF to EMF and replay EMF on printer DC.
Attachment #8877494 -
Flags: feedback?(brsun)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
https://reviewboard.mozilla.org/r/148952/#review153850
::: widget/windows/nsDeviceContextSpecWin.cpp:104
(Diff revision 3)
> mDriverName = nullptr;
> mDeviceName = nullptr;
> mDevMode = nullptr;
> #ifdef MOZ_ENABLE_SKIA_PDF
> mPrintViaSkPDF = false;
> + mDC = nullptr;
The type of HDC is somewhat opaque. Suggest to use the value as document says: NULL[1]
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd183490(v=vs.85).aspx
::: widget/windows/nsDeviceContextSpecWin.cpp:130
(Diff revision 3)
> psWin->SetDevMode(nullptr);
> }
> #ifdef MOZ_ENABLE_SKIA_PDF
> if (mPrintViaSkPDF) {
> mPDFTempFile->Remove(false);
> + if (mDC) {
Since HDC is a opaque type and NULL is also a somewhat opaque value, suggest to compare mDC with NULL to respect the document[1].
[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd183490(v=vs.85).aspx
::: widget/windows/nsDeviceContextSpecWin.cpp:280
(Diff revision 3)
> nsAutoCString printFile(NS_ConvertUTF16toUTF8(filename).get());
> auto skStream = MakeUnique<SkFILEWStream>(printFile.get());
> return PrintTargetSkPDF::CreateOrNull(Move(skStream), size);
> }
>
> + if (mDevMode) {
As we discussed, let's move this conditional checking back to bug 1372108.
::: widget/windows/nsDeviceContextSpecWin.cpp:402
(Diff revision 3)
> }
>
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +void
> +nsDeviceContextSpecWin::PDFPrintjob()
> +{
Add assertion to check the value of mDC, mPDFPrintHelper and mPDFTempFile.
::: widget/windows/nsDeviceContextSpecWin.cpp:408
(Diff revision 3)
> + bool isprinted = false;
> + if (::StartPage(mDC) > 0) {
> + isprinted = mPDFPrintHelper->DrawPage(mDC, mPDFPageNum++,
> + ::GetDeviceCaps(mDC, HORZRES),
> + ::GetDeviceCaps(mDC, VERTRES));
> + ::EndPage(mDC);
Should we check the return value of ::EndPage() as well?
::: widget/windows/nsDeviceContextSpecWin.cpp:445
(Diff revision 3)
> + nsresult rv = NS_OK;
> +#ifdef MOZ_ENABLE_SKIA_PDF
> + if (mPrintViaSkPDF && (mOutputFormat != nsIPrintSettings::kOutputFormatPDF)) {
> + const uint32_t DOC_TITLE_LENGTH = MAX_PATH - 1;
> + mPDFPrintHelper = MakeUnique<PDFViaEMFPrintHelper>();
> + mPDFPrintHelper->OpenDocument(mPDFTempFile);
Suggest to check the return value of OpenDocument.
::: widget/windows/nsDeviceContextSpecWin.cpp:466
(Diff revision 3)
> + di.lpszOutput = mPrintToFileName.Length() > 0 ?
> + mPrintToFileName.get() : nullptr;
> + di.lpszDatatype = nullptr;
> + di.fwType = 0;
> +
> + if (::StartDocW(mDC, &di) <= 0) {
Could we call ::StartDocW in nsDeviceContextSpecWin::BeginDocument so that we can avoid adding two member variables (i.e. mTitle and mPrintToFileName)?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8877494 -
Flags: feedback?(brsun)
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
https://reviewboard.mozilla.org/r/148952/#review154834
::: widget/windows/nsDeviceContextSpecWin.cpp:130
(Diff revision 4)
> psWin->SetDeviceName(nullptr);
> psWin->SetDriverName(nullptr);
> psWin->SetDevMode(nullptr);
> }
> #ifdef MOZ_ENABLE_SKIA_PDF
> + if (mIsPrinting > 0) {
Check the value of mDC for both ::EndDoc(mDC) and ::DeleteDC(mDC) here. Check mIsPrinting for ::EndDoc(mDC) additionally.
::: widget/windows/nsDeviceContextSpecWin.cpp:404
(Diff revision 4)
> }
>
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +void
> +nsDeviceContextSpecWin::PDFPrintjob()
> +{
Add one assertion for mIsPrinting.
::: widget/windows/nsDeviceContextSpecWin.cpp:415
(Diff revision 4)
> + int endPageResult = 0;
> + if (::StartPage(mDC) > 0) {
> + isPrinted = mPDFPrintHelper->DrawPage(mDC, mPDFPageNum++,
> + ::GetDeviceCaps(mDC, HORZRES),
> + ::GetDeviceCaps(mDC, VERTRES));
> + endPageResult = ::EndPage(mDC);
nit: If the return value has only two meanings (i.e successful or not successful), suggest to convert the logical meaning of the return value to a boolean value here. So that we can avoid mystery integer checking later.
::: widget/windows/nsDeviceContextSpecWin.cpp:419
(Diff revision 4)
> + ::GetDeviceCaps(mDC, VERTRES));
> + endPageResult = ::EndPage(mDC);
> + }
> +
> + if (mPDFPageNum < mPDFPageCount && isPrinted && endPageResult > 0) {
> + NS_DispatchToCurrentThread(NewRunnableMethod(this,
Should we do any error handling if this line fails?
::: widget/windows/nsDeviceContextSpecWin.cpp:458
(Diff revision 4)
> + di.lpszOutput = printToFileName.Length() > 0 ?
> + printToFileName.get() : nullptr;
> + di.lpszDatatype = nullptr;
> + di.fwType = 0;
> +
> + mIsPrinting = ::StartDocW(mDC, &di);
Suggest to use a boolean type for mIsPrinting to sync with its variable name.
::: widget/windows/nsDeviceContextSpecWin.cpp:488
(Diff revision 4)
> + mPDFPrintHelper = nullptr;
> + return NS_ERROR_FAILURE;
> + }
> + mPDFPageNum = 0;
> +
> + rv = NS_DispatchToCurrentThread(NewRunnableMethod(this,
Please help to double check whether we can release resources properly or not if this line fails.
Updated•7 years ago
|
Attachment #8877494 -
Flags: feedback?(brsun)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
https://reviewboard.mozilla.org/r/148952/#review154954
::: widget/windows/nsDeviceContextSpecWin.cpp:475
(Diff revision 6)
> + printToFileName.get() : nullptr;
> + di.lpszDatatype = nullptr;
> + di.fwType = 0;
> +
> + if (::StartDocW(mDC, &di) <= 0) {
> + // Defer calling Cleanup() in deconstructor because PDF temp file is not
nit: destructor
Comment 21•7 years ago
|
||
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
It looks good to me. Suggest to have layout experts' comments as well.
Attachment #8877494 -
Flags: feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium
https://reviewboard.mozilla.org/r/148952/#review155098
::: widget/windows/nsDeviceContextSpecWin.h:78
(Diff revision 9)
>
> nsCOMPtr<nsIPrintSettings> mPrintSettings;
> int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative;
>
> #ifdef MOZ_ENABLE_SKIA_PDF
> + void PDFPrintjob();
I think this should be called FinishPrintViaPDF.
::: widget/windows/nsDeviceContextSpecWin.h:79
(Diff revision 9)
> nsCOMPtr<nsIPrintSettings> mPrintSettings;
> int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative;
>
> #ifdef MOZ_ENABLE_SKIA_PDF
> + void PDFPrintjob();
> + void Cleanp();
And this should be called CleanupPrintViaPDF.
::: widget/windows/nsDeviceContextSpecWin.h:87
(Diff revision 9)
> // It controls both whether normal printing is done via PDF using Skia and
> // whether print-to-PDF uses Skia.
> bool mPrintViaSkPDF;
> + nsCOMPtr<nsIFile> mPDFTempFile;
> + HDC mDC;
> + bool mIsPrinting;
Call this mPrintViaPDFInProgress. That helps make it clear to anyone looking at this code and debugging non-skpdf scenarios that they don't need to worry about this variable.
::: widget/windows/nsDeviceContextSpecWin.h:88
(Diff revision 9)
> // whether print-to-PDF uses Skia.
> bool mPrintViaSkPDF;
> + nsCOMPtr<nsIFile> mPDFTempFile;
> + HDC mDC;
> + bool mIsPrinting;
> + mozilla::UniquePtr<mozilla::widget::PDFViaEMFPrintHelper> mPDFPrintHelper;
Add:
typedef mozilla::widget::PDFViaEMFPrintHelper PDFViaEMFPrintHelper;
right at the top of this class (before the 'private:' line) then remove the namespace qualification here.
::: widget/windows/nsDeviceContextSpecWin.h:90
(Diff revision 9)
> + nsCOMPtr<nsIFile> mPDFTempFile;
> + HDC mDC;
> + bool mIsPrinting;
> + mozilla::UniquePtr<mozilla::widget::PDFViaEMFPrintHelper> mPDFPrintHelper;
> + int mPDFPageCount;
> + int mPDFPageNum;
Call this mPDFCurrentPageNum.
::: widget/windows/nsDeviceContextSpecWin.cpp:130
(Diff revision 9)
> psWin->SetDriverName(nullptr);
> psWin->SetDevMode(nullptr);
> }
>
> +#ifdef MOZ_ENABLE_SKIA_PDF
> + Cleanp();
Wrap this in an 'if' check for mIsPrinting.
::: widget/windows/nsDeviceContextSpecWin.cpp:278
(Diff revision 9)
> auto skStream = MakeUnique<SkFILEWStream>(printFile.get());
> return PrintTargetSkPDF::CreateOrNull(Move(skStream), size);
> }
> +
> + if (mDevMode) {
> + NS_WARNING_ASSERTION(mDriverName, "No driver!");
At the top here, just inside the |if| block please add the comment:
// When printing to a printer via Skia PDF we open a temporary file that we draw the print output into as PDF output, then once we reach EndDcoument we'll convert that PDF file to EMF page by page to print each page. Here we create the temporary file and wrap it in a PrintTargetSkPDF that we return.
::: widget/windows/nsDeviceContextSpecWin.cpp:279
(Diff revision 9)
> return PrintTargetSkPDF::CreateOrNull(Move(skStream), size);
> }
> +
> + if (mDevMode) {
> + NS_WARNING_ASSERTION(mDriverName, "No driver!");
> + mDC = ::CreateDCW(mDriverName, mDeviceName, nullptr, mDevMode);
As discussed on IRC please move the mDC code to BeginDocument.
::: widget/windows/nsDeviceContextSpecWin.cpp:405
(Diff revision 9)
> + mPDFPrintHelper = nullptr;
> + mPDFPageCount = 0;
> + }
> +
> + if (mPDFTempFile) {
> + mPDFTempFile->Remove(false);
Add /* aRecursive */ before the false value.
::: widget/windows/nsDeviceContextSpecWin.cpp:458
(Diff revision 9)
> + int32_t aStartPage,
> + int32_t aEndPage)
> +{
> +#ifdef MOZ_ENABLE_SKIA_PDF
> + if (mPrintViaSkPDF && (mOutputFormat != nsIPrintSettings::kOutputFormatPDF)) {
> + const uint32_t DOC_TITLE_LENGTH = MAX_PATH - 1;
At the top of this block just inside the |if| statement please add this comment:
// Here we create mDC which we'll draw each page from our temporary PDF file to once we reach EndDocument. The only reason we create it here rather than in EndDocument is so that we don't need to store aTitle and aPrintToFileName as member data.
::: widget/windows/nsDeviceContextSpecWin.cpp:483
(Diff revision 9)
> + }
> +
> + mIsPrinting = true;
> + }
> +
> +#endif
Nit: put the blank line after the #endif instead of before it.
::: widget/windows/nsDeviceContextSpecWin.cpp:492
(Diff revision 9)
> +nsresult
> +nsDeviceContextSpecWin::EndDocument()
> +{
> + nsresult rv = NS_OK;
> +#ifdef MOZ_ENABLE_SKIA_PDF
> + if (mPrintViaSkPDF && (mOutputFormat != nsIPrintSettings::kOutputFormatPDF) &&
This line is too long. Please break these checks over three lines to make it easier to read.
Attachment #8877494 -
Flags: review?(jwatt) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Summary: Enable PDF to EMF printing on Windows → Add support for having printing on Windows print via Skia PDF and PDFium
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
Assignee | ||
Comment 29•7 years ago
|
||
Comment 30•7 years ago
|
||
Pushed by fatseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b611ec2a42bf
Add support for having printing on Windows print via Skia PDF and PDFium r=jwatt
Comment 31•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 32•7 years ago
|
||
This caused a 1.8 MB increase in installer size for Win 64 PGO.
You need to log in
before you can comment on or make changes to this bug.
Description
•