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

RESOLVED FIXED in Firefox 56

Status

()

Product:
Core
Component:
Printing: Output
Type:
enhancement
Status:
RESOLVED FIXED
Reported:
2 years ago
Modified:
2 years ago

People

(Reporter: fatseng, Assigned: fatseng)

Tracking

(Blocks 2 bugs)

Version:
unspecified
Target:
mozilla56
Platform:
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

User Story

To use this functionality you need to build with:

  ac_add_options --enable-skia-pdf

and set the pref:

  print.print_via_pdf_encoder=skia-pdf

Attachments

(1 attachment)

Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows
59 bytes, text/x-review-board-request
jwatt
: review+
Details
(Assignee)

Description

•
2 years ago 
I would like to Implement Skia PDF for Printing in Windows.
 (Assignee)

Updated

•
2 years ago
User Story: (updated)
 (Assignee)

Updated

•
2 years ago
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
 (Assignee)

Updated

•
2 years ago
Blocks: pdf-printing
Comment hidden (mozreview-request)
(Assignee)

Updated

•
2 years ago
Attachment #8877029 - Flags: feedback?(brsun)
 (Assignee)

Updated

•
2 years ago
Blocks: 1370488
 (Assignee)

Updated

•
2 years ago
Depends on: 1369302

Comment 2

•
2 years ago
mozreview-review 
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

•
2 years ago 
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.
Attachment #8877029 - Flags: feedback?(brsun)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

•
2 years ago
Attachment #8877029 - Flags: feedback?(brsun)

Comment 7

•
2 years ago
mozreview-review 
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.
Blocks: 1295109
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

•
2 years ago
Attachment #8877029 - Flags: feedback?(brsun)

Comment 10

•
2 years ago
mozreview-review 
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

•
2 years ago 
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.
Attachment #8877029 - Flags: feedback?(brsun) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

•
2 years ago
mozreview-review 
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
Attachment #8877029 - Flags: review?(jwatt) → review+

Comment 15

•
2 years ago 
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

•
2 years ago 
All the mPDFTempFile code belongs over there too.
Comment hidden (mozreview-request)

Comment 18

•
2 years ago
mozreview-review 
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 hidden (mozreview-request)
(Assignee)

Updated

•
2 years ago
Summary: Enable Skia PDF for printing in Windows → Add support for having Save-as-PDF use Skia PDF on Windows

Comment 21

•
2 years ago 
Pushed by fatseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57e457a24368
Add support for having Save-as-PDF use Skia PDF on Windows r=jwatt

Comment 22

•
2 years ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/57e457a24368
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.