Closed Bug 1370488 Opened 3 years ago Closed 2 years ago

Add support for having printing on Windows print via Skia PDF and PDFium

Categories

(Core :: Printing: Output, enhancement)

Unspecified
Windows
enhancement
Not set

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.
Blocks: pdf-printing
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Attached file pepperpdfium.dll (WIP) (obsolete) —
Attached patch printEMF.patch (WIP) (obsolete) — Splinter Review
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
Attached patch printEMF.patch V1 (WIP) (obsolete) — Splinter Review
Attachment #8875195 - Attachment is obsolete: true
Attachment #8875193 - Attachment is obsolete: true
Attachment #8875203 - Attachment is obsolete: true
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
Depends on: 1372108
Summary: Print EMF → Enable PDF to EMF printing on Windows
Attachment #8876068 - Attachment is obsolete: true
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 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)?
Duplicate of this bug: 1321496
Attachment #8877494 - Flags: feedback?(brsun)
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.
Attachment #8877494 - Flags: feedback?(brsun)
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 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 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+
Summary: Enable PDF to EMF printing on Windows → Add support for having printing on Windows print via Skia PDF and PDFium
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
https://hg.mozilla.org/mozilla-central/rev/b611ec2a42bf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.