Closed
Bug 1372108
Opened 8 years ago
Closed 8 years ago
Add support for having Save-as-PDF use Skia PDF on Windows
Categories
(Core :: Printing: Output, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
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)
I would like to Implement Skia PDF for Printing in Windows.
Assignee | ||
Updated•8 years ago
|
User Story: (updated)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → fatseng
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Blocks: pdf-printing
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8877029 -
Flags: feedback?(brsun)
Comment 2•8 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•8 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•8 years ago
|
Attachment #8877029 -
Flags: feedback?(brsun)
Comment 7•8 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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8877029 -
Flags: feedback?(brsun)
Comment 10•8 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•8 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•8 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•8 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•8 years ago
|
||
All the mPDFTempFile code belongs over there too.
Comment hidden (mozreview-request) |
![]() |
||
Comment 18•8 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•8 years ago
|
Summary: Enable Skia PDF for printing in Windows → Add support for having Save-as-PDF use Skia PDF on Windows
Assignee | ||
Comment 20•8 years ago
|
||
Comment 21•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•