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

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement)

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: pepperpdfium.dll (WIP)

Comment 2

[Farmer Tseng[:fatseng](inactive)]

Attachment: printEMF.patch (WIP)

Comment 3

[Farmer Tseng[:fatseng](inactive)]

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

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Attachment: printEMF.patch V1 (WIP)

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Attachment: Use PDFium functions of XUL instead of dynamic loading library (WIP)

Comment 6

[Farmer Tseng[:fatseng](inactive)]

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

Comment 7

[Farmer Tseng[:fatseng](inactive)]

Attachment: Use PDFium functions of XUL instead of dynamic loading library V1.1

Comment 8

[Farmer Tseng[:fatseng](inactive)]

[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

Comment 9

[Farmer Tseng[:fatseng](inactive)]

Fix running browser_DownloadPDFSaver.js failure.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=444b2148d8b8d028c0ebc02975813029d573f68e

Comment 10

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

1. Convert PDF to EMF via PDFViaEMFPrintHelper.
2. Replay EMF on printer DC.

Review commit: https://reviewboard.mozilla.org/r/148952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148952/

Comment 11

[Farmer Tseng[:fatseng](inactive)]

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.

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/1-2/

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/2-3/

Comment 14

[Bruce Sun [: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)?

Comment 16

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/3-4/

Comment 17

[Bruce Sun [: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.

Comment 18

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/4-5/

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/5-6/

Comment 20

[Bruce Sun [: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 21

[Bruce Sun [:brsun]]

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.

Comment 22

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/6-7/

Comment 23

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/7-8/

Comment 24

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/8-9/

Comment 25

[Jonathan Watt [:jwatt]]

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.

Comment 26

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/9-10/

Comment 27

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877494 [details]
Bug 1370488 - Add support for having printing on Windows print via Skia PDF and PDFium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148952/diff/10-11/

Comment 28

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2337bee2a4018accec445ee5bc51290f9dc4cfad

Comment 29

[Farmer Tseng[:fatseng](inactive)]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=886e0dbc0cb2a6504822105c60c49e594beb9481
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b346af553e26285694d8e09b9ee5b838407cb8e8

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b611ec2a42bf

Comment 32

[Kevin Brosnan [Ex-Mozilla]]

This caused a 1.8 MB increase in installer size for Win 64 PGO.