Closed Bug 1372108 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Printing: Output, enhancement)

Product:
Core
Component:
Printing: Output
Platform:
Unspecified
Windows
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: fatseng, Assigned: fatseng)

References

(Blocks 2 open bugs)

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 file)

Bug 1372108 - Add support for having Save-as-PDF use Skia PDF on Windows
59 bytes, text/x-review-board-request
jwatt
: review+
Details 
I would like to Implement Skia PDF for Printing in Windows.
User Story: (updated)
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Blocks: pdf-printing
Attachment #8877029 - Flags: feedback?(brsun)
Blocks: 1370488
Depends on: 1369302
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 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)
Attachment #8877029 - Flags: feedback?(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.
Blocks: 1295109
Attachment #8877029 - Flags: feedback?(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 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 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+
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.
All the mPDFTempFile code belongs over there too.
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.
Summary: Enable Skia PDF for printing in Windows → Add support for having Save-as-PDF use Skia PDF on Windows
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
https://hg.mozilla.org/mozilla-central/rev/57e457a24368
Status: ASSIGNED → RESOLVED
Closed: 6 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.