1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

Status: RESOLVED FIXED

Description

[Farmer Tseng[:fatseng](inactive)]

I would like to Implement Skia PDF for Printing in Windows.

Comment 1

[Farmer Tseng[:fatseng](inactive)]

Attachment: Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

This patch is step 1 for generating Skia PDF file.
If pref print.print_via_pdf_encoder=skia-pdf, print target becomes
PrintTargetSkPDF

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

Comment 2

[Bruce Sun [:brsun]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

https://reviewboard.mozilla.org/r/148368/#review153424

::: widget/windows/nsDeviceContextSpecWin.h:47
(Diff revision 1)
>    void GetDriverName(wchar_t *&aDriverName) const   { aDriverName = mDriverName;     }
>    void GetDeviceName(wchar_t *&aDeviceName) const   { aDeviceName = mDeviceName;     }
>  
>    // The GetDevMode will return a pointer to a DevMode
>    // whether it is from the Global memory handle or just the DevMode
> -  // To get the DevMode from the Global memory Handle it must lock it 
> +  // To get the DevMode from the Global memory Handle it must lock it

nit: suggest not to include this modification if it is not related to this bug.

::: widget/windows/nsDeviceContextSpecWin.cpp:99
(Diff revision 1)
>    mDriverName    = nullptr;
>    mDeviceName    = nullptr;
>    mDevMode       = nullptr;
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  mPrintViaSkPDF = false;
> +  mDC            = nullptr;

Suggest not to add this member variables unless it is not negligible.

::: widget/windows/nsDeviceContextSpecWin.cpp:251
(Diff revision 1)
>  
>  already_AddRefed<PrintTarget> nsDeviceContextSpecWin::MakePrintTarget()
>  {
>    NS_ASSERTION(mDevMode, "DevMode can't be NULL here");
>  
> +  double width, height;

Suggest not to move these two variables out of its original scope.

::: widget/windows/nsDeviceContextSpecWin.cpp:266
(Diff revision 1)
>      // convert twips to points
>      width  /= TWIPS_PER_POINT_FLOAT;
>      height /= TWIPS_PER_POINT_FLOAT;
>  
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {

nit: indent properly

::: widget/windows/nsDeviceContextSpecWin.cpp:296
(Diff revision 1)
>      if (!dc) {
>        gfxCriticalError(gfxCriticalError::DefaultOptions(false))
>          << "Failed to create device context in GetSurfaceForPrinter";
>        return nullptr;
>      }
> +#ifdef MOZ_ENABLE_SKIA_PDF

Suggest to merge the logic of returning PrintTargetSkPDF into one, so that we can reduce the impact of original workflows.

::: widget/windows/nsDeviceContextSpecWin.cpp:297
(Diff revision 1)
>        gfxCriticalError(gfxCriticalError::DefaultOptions(false))
>          << "Failed to create device context in GetSurfaceForPrinter";
>        return nullptr;
>      }
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {

nit: indent properly

::: widget/windows/nsDeviceContextSpecWin.cpp:301
(Diff revision 1)
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {
> +    MOZ_ASSERT(!mDC);
>  
> +    mDC = dc;
> +    mPrintSettings->GetEffectivePageSize(&width, &height);

Suggest to declare width and height locally within this scope.

::: widget/windows/nsDeviceContextSpecWin.cpp:331
(Diff revision 1)
> +
> +    nsAutoCString printFile("tmp-printing");
> +    printFile.Append(nsPrintfCString("%s.pdf", uuidChars));
> +    rv = mPDFTempFile->AppendNative(printFile);
> +    NS_ENSURE_SUCCESS(rv, nullptr);
> +    mPDFTempFile->GetNativePath(printFile);

Suggest not to use the same variable to store filename string and filepath string at different line. Suggest to use different local variables for different purposes separately.

::: widget/windows/nsDeviceContextSpecWin.cpp:348
(Diff revision 1)
>  float
>  nsDeviceContextSpecWin::GetDPI()
>  {
>    // To match the previous printing code we need to return 72 when printing to
>    // PDF and 144 when printing to a Windows surface.
> -  return mOutputFormat == nsIPrintSettings::kOutputFormatPDF ? 72.0f : 144.0f;
> +#ifdef MOZ_ENABLE_SKIA_PDF

<p>Since this function is related small, probably we can keep the original implementation untouched:</p>
<h1>ifdef MOZ_ENABLE_SKIA_PDF</h1>
<p>// your new implmentation</p>
<h1>else</h1>
<p>return mOutputFormat == nsIPrintSettings::kOutputFormatPDF ? 72.0f : 144.0f;</p>
<h1>endif</h1>

Comment 3

[Bruce Sun [:brsun]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

Hi Farmer, please see comments inline.

Comment 4

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 5

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 6

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 7

[Bruce Sun [:brsun]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

https://reviewboard.mozilla.org/r/148368/#review153860

::: widget/windows/nsDeviceContextSpecWin.cpp:121
(Diff revision 4)
>      psWin->SetDriverName(nullptr);
>      psWin->SetDevMode(nullptr);
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {
> +    mPDFTempFile->Remove(false);

Check whether the pointer stored in mPDFTempFile is valid or not before deferencing it.

::: widget/windows/nsDeviceContextSpecWin.cpp:269
(Diff revision 4)
> +      nsAutoCString printFile(NS_ConvertUTF16toUTF8(filename).get());
> +      auto skStream = MakeUnique<SkFILEWStream>(printFile.get());
> +      return PrintTargetSkPDF::CreateOrNull(Move(skStream), size);
> +    }
> +
> +    nsresult rv =

As we discussed, let's add |if (mDevMode) {...}| back to this patch.

Comment 8

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 9

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 10

[Bruce Sun [:brsun]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

https://reviewboard.mozilla.org/r/148368/#review154312

::: widget/windows/nsDeviceContextSpecWin.cpp:121
(Diff revision 6)
>      psWin->SetDeviceName(nullptr);
>      psWin->SetDriverName(nullptr);
>      psWin->SetDevMode(nullptr);
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {

If mPDFTempFile is not null, probably we always need to remove it anyway. Checking for mPrintViaSkPDF seems a little redundant here.

::: widget/windows/nsDeviceContextSpecWin.cpp:122
(Diff revision 6)
>      psWin->SetDriverName(nullptr);
>      psWin->SetDevMode(nullptr);
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {
> +    if (mPDFTempFile) {

Add assertion for mPrintViaSkPDF here instead (if it is necessary.)

::: widget/windows/nsDeviceContextSpecWin.cpp:360
(Diff revision 6)
> +    return 72.0f;
> +  }
> +  return 144.0f;
> +#else
>    return mOutputFormat == nsIPrintSettings::kOutputFormatPDF ? 72.0f : 144.0f;
> +#endif

This seems good already. How about further making codes more compact by:

#ifdef MOZ_ENABLE_SKIA_PDF
  if (mPrintViaSkPDF) {
    return 72.0f;
  }
#endif
  return mOutputFormat == nsIPrintSettings::kOutputFormatPDF ? 72.0f : 144.0f;

Comment 11

[Bruce Sun [:brsun]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

It looks good to me. Suggest to have layout experts' comments as well.

Comment 12

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 13

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

https://reviewboard.mozilla.org/r/148368/#review155004

::: widget/windows/nsDeviceContextSpecWin.h:71
(Diff revision 8)
>    nsCOMPtr<nsIPrintSettings> mPrintSettings;
>    int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative;
> +
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  nsCOMPtr<nsIFile> mPDFTempFile;
> +  bool mPrintViaSkPDF;

Please add a comment before this variable saying something like:

  // This variable is independant of nsIPrintSettings::kOutputFormatPDF.
  // It controls both whether normal printing is done via PDF using Skia and whether print-to-PDF uses Skia.

::: widget/windows/nsDeviceContextSpecWin.cpp:98
(Diff revision 8)
>  {
>    mDriverName    = nullptr;
>    mDeviceName    = nullptr;
>    mDevMode       = nullptr;
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  mPDFTempFile   = nullptr;

nsCOMPtr has a constructor that does this, so you can remove this line.

::: widget/windows/nsDeviceContextSpecWin.cpp:122
(Diff revision 8)
>      psWin->SetDriverName(nullptr);
>      psWin->SetDevMode(nullptr);
>    }
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPDFTempFile) {
> +    mPDFTempFile->Remove(false);

Make this:

  mPDFTempFile->Remove(/* aRecursive */ false);

::: widget/windows/nsDeviceContextSpecWin.cpp:148
(Diff revision 8)
>  {
>    mPrintSettings = aPrintSettings;
>  
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  const nsAdoptingString& printViaPdf =
> +    mozilla::Preferences::GetString("print.print_via_pdf_encoder");

Please put this inside the:

  if (aPrintSettings) {

section. Checking prefs has a perf impact which is negligable during printing, but is not a cost we want to incur when creating normal windows.

::: widget/windows/nsDeviceContextSpecWin.cpp:296
(Diff revision 8)
> +      NS_ENSURE_SUCCESS(rv, nullptr);
> +
> +      nsAutoCString filePath;
> +      mPDFTempFile->GetNativePath(filePath);
> +      auto skStream = MakeUnique<SkFILEWStream>(filePath.get());
> +      return PrintTargetSkPDF::CreateOrNull(Move(skStream), size);

Okay, so this doesn't actually do anything with the file when we print to a printer, but I guess that functionality will be added in bug 1370488. I guess that's okay.

::: widget/windows/nsDeviceContextSpecWin.cpp:365
(Diff revision 8)
>  nsDeviceContextSpecWin::GetPrintingScale()
>  {
>    MOZ_ASSERT(mPrintSettings);
> -
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  if (mPrintViaSkPDF) {
> +    return 1.0f;

Make this:

  return 1.0f; // PDF is vector based, so we don't need a scale

Comment 15

[Jonathan Watt [:jwatt]]

As discussed on IRC, the mDevMode section is really incomplete code that probably belongs over in bug 1370488. Can you move that code over there and then rename this bug to "Add support for having Save-as-PDF use Skia PDF on Windows"?

Make sure you update the commit message as well.

Comment 16

[Jonathan Watt [:jwatt]]

All the mPDFTempFile code belongs over there too.

Comment 17

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

https://reviewboard.mozilla.org/r/148368/#review155090

::: widget/windows/nsDeviceContextSpecWin.h:70
(Diff revision 9)
>  
>    nsCOMPtr<nsIPrintSettings> mPrintSettings;
>    int16_t mOutputFormat = nsIPrintSettings::kOutputFormatNative;
> +
> +#ifdef MOZ_ENABLE_SKIA_PDF
> +  // This variable is independant of nsIPrintSettings::kOutputFormatPDF.

I think this comment could move to the other bug.

Comment 19

[Farmer Tseng[:fatseng](inactive)]

Comment on attachment 8877029 [details]
Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows

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

Comment 20

[Farmer Tseng[:fatseng](inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03208504e66c9ab1c39fd936d5451260c3519128
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbd1e36d39dcb65ebaaecf3c048dac7ec7ff6d4f

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57e457a24368
Add support for having Save-as-PDF use Skia PDF on Windows r=jwatt

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57e457a24368