Implement printing via Skia PDF for macOS

RESOLVED FIXED in mozilla54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks 4 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix)

Details

(Whiteboard: [behind pref print.print_via_pdf_encoder=skia-pdf])

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

(8 attachments, 8 obsolete attachments)

2.12 KB, patch
mchang
: review+
Details | Diff | Splinter Review
5.70 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
6.14 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
7.24 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
14.29 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
7.15 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
9.36 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
10.21 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Use SkPDF to print whenever PDF is supported.
(Assignee)

Updated

3 years ago
Component: Graphics → Printing: Output
(Assignee)

Updated

3 years ago
Blocks: 1278259
Before we do this we need to improve the state of our printing tests.
Depends on: 1299848
(Assignee)

Comment 2

3 years ago
We should definitely do that before shipping any substantial changes like this. I'd like to get some code landed behind a pref for testing though.
(Assignee)

Comment 3

3 years ago
Lee, this fixes up the generated files directly, but doesn't include any of the changes needed in the generator scripts. Would you be able to fix those scripts when you land your Skia 55 update?
Attachment #8804771 - Flags: review?(lsalzman)
(Assignee)

Comment 5

3 years ago
Attachment #8804775 - Flags: review?(lsalzman)
Comment on attachment 8804775 [details] [diff] [review]
part 3 - Implement PrintTargetSkPDF

Review of attachment 8804775 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/PrintTargetSkPDF.cpp
@@ +14,5 @@
> +PrintTargetSkPDF::PrintTargetSkPDF(const IntSize& aSize,
> +                                   SkDocument* aPDFDoc,
> +                                   UniquePtr<SkWStream> aStream)
> +  : PrintTarget(/* not using cairo_surface_t */ nullptr, aSize)
> +  , mPDFDoc(aPDFDoc)

Beware: sk_sp does not ref in its explicit constructor, so you want to use sk_ref_sp(aPDFDoc) here.

@@ +77,5 @@
> +
> +nsresult
> +PrintTargetSkPDF::BeginPage()
> +{
> +  mPageCanvas = mPDFDoc->beginPage(mSize.width, mSize.height);

You would want to use sk_ref_sp(...) here as well.

::: gfx/thebes/PrintTargetSkPDF.h
@@ +50,5 @@
> +  // The stream that the SkDocument outputs to.
> +  UniquePtr<SkWStream> mOStream;
> +
> +  // The SkCanvas and its wrapping DrawTarget for the current page:
> +  SkCanvas* mPageCanvas;

This should use sk_sp<SkCanvas>, otherwise this will end up leaking.
Comment on attachment 8804773 [details] [diff] [review]
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas

Review of attachment 8804773 [details] [diff] [review]:
-----------------------------------------------------------------

For this to work I will need to add another patch to this bug that allows DrawTargetSkia to work with just an SkCanvas without an accompanying SkSurface (a change that came along with the Skia update). Otherwise, this should be okay.

::: gfx/2d/DrawTargetSkia.cpp
@@ +1682,5 @@
> +  bool isBackedByPixels = imageInfo.colorType() != kUnknown_SkColorType;
> +  if (isBackedByPixels) {
> +    // Note for PDF backed SkCanvas |alphaType == kUnknown_SkAlphaType|.
> +    SkAlphaType alphaType = imageInfo.alphaType();
> +    SkColor clearColor = (alphaType == kOpaque_SkAlphaType) ? SK_ColorBLACK : SK_ColorTRANSPARENT;

It would be much easier here to just do:
imageInfo.isOpaque() ? ... : ...
This is required for SkPDF to work, since it only gives us an SkCanvas without a wrapping SkSurface.
Attachment #8804825 - Flags: review?(mchang)
Attachment #8804825 - Flags: review?(mchang) → review+
(Assignee)

Comment 9

3 years ago
Attachment #8804771 - Attachment is obsolete: true
Attachment #8804771 - Flags: review?(lsalzman)
Attachment #8805359 - Flags: review?(lsalzman)
(Assignee)

Comment 10

3 years ago
Attachment #8804773 - Attachment is obsolete: true
Attachment #8804773 - Flags: review?(lsalzman)
Attachment #8805360 - Flags: review?(lsalzman)
(Assignee)

Comment 11

3 years ago
Attachment #8805360 - Attachment is obsolete: true
Attachment #8805360 - Flags: review?(lsalzman)
Attachment #8805363 - Flags: review?(lsalzman)
Comment on attachment 8805363 [details] [diff] [review]
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas

Review of attachment 8805363 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Factory.cpp
@@ +704,5 @@
>  
>  #endif // USE_SKIA_GPU
>  
> +already_AddRefed<DrawTarget>
> +Factory::CreateDrawTargetWithSkCanvas(SkCanvas* aCanvas)

Needs to be guarded by #ifdef USE_SKIA, otherwise it won't compile with Skia not build in (a config we still support).
(Assignee)

Comment 13

3 years ago
Attachment #8805359 - Attachment is obsolete: true
Attachment #8805359 - Flags: review?(lsalzman)
Attachment #8805399 - Flags: review?(lsalzman)
(Assignee)

Comment 14

3 years ago
Attachment #8805363 - Attachment is obsolete: true
Attachment #8805363 - Flags: review?(lsalzman)
Attachment #8805400 - Flags: review?(lsalzman)
Attachment #8805400 - Flags: review?(lsalzman) → review+
(Assignee)

Comment 15

3 years ago
Attachment #8805399 - Attachment is obsolete: true
Attachment #8805399 - Flags: review?(lsalzman)
Attachment #8805521 - Flags: review?(lsalzman)
Attachment #8805521 - Flags: review?(lsalzman) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 16

3 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89304c262266
part 1 - Add an --enable-skia-pdf configuration option. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/c974afea821b
part 2 - Support the creation of a DrawTargetSkia for an SkCanvas. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee15183d0572
part 3 - Allow DrawTargetSkia to work without an SkSurface. r=mchang
(Assignee)

Updated

3 years ago
Depends on: 1316299
Comment on attachment 8804775 [details] [diff] [review]
part 3 - Implement PrintTargetSkPDF

Just closing this out as r- for now since there are still the above review comments that need addressing.
Attachment #8804775 - Flags: review?(lsalzman) → review-
(Assignee)

Updated

3 years ago
Attachment #8804825 - Attachment description: part 4 - allow DrawTargetSkia to work without an SkSurface → part 3 - allow DrawTargetSkia to work without an SkSurface
(Assignee)

Comment 19

3 years ago
Attachment #8804775 - Attachment is obsolete: true
Attachment #8814359 - Flags: review?(lsalzman)
(Assignee)

Comment 20

3 years ago
Right now nsDeviceContext::BeginPage etc. have hacks to replace the PrintTargetCG for each page that is printed, because the CGGraphicsContext returned by PMSessionGetCGGraphicsContext is only valid for a single page. We should really be encapsulating that in the PrintTargetCG::BeginPage/EndPage methods. Doing that and removing the hacks means that I don't need to add hacks to the hacks in order to avoid replacing the PrintTarget for each page when we use PrintTargetSkPDF (the use of which will depends on a configure option, a pref and the exact printing action of the user.
Attachment #8814362 - Flags: review?(lsalzman)
(Assignee)

Comment 21

3 years ago
Factoring this out from the next patch in the series to make that patch a bit cleaner.
Attachment #8814364 - Flags: review?(lsalzman)
(Assignee)

Comment 22

3 years ago
This moves logic to where it really belongs, and makes the code that touches nsDeviceContextSpecX in the next patch cleaner by requiring fewer ifdefs.
Attachment #8814366 - Flags: review?(lsalzman)
(Assignee)

Comment 23

3 years ago
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
Attachment #8814367 - Flags: review?(lsalzman)
(Assignee)

Updated

3 years ago
Summary: Use SkPDF (Skia PDF) to print whenever PDF is supported → Implement printing via Skia PDF for macOS
(Assignee)

Updated

3 years ago
User Story: (updated)
(Assignee)

Comment 24

3 years ago
Attachment #8814367 - Attachment is obsolete: true
Attachment #8814367 - Flags: review?(lsalzman)
Attachment #8814390 - Flags: review?(lsalzman)
Attachment #8814359 - Flags: review?(lsalzman) → review+
Attachment #8814364 - Flags: review?(lsalzman) → review+
Attachment #8814362 - Flags: review?(lsalzman) → review+
Attachment #8814366 - Flags: review?(lsalzman) → review+
Attachment #8814390 - Flags: review?(lsalzman) → review+

Comment 26

3 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/125e6fb37319
part 4 - Implement a PrintTarget sub-class for printing via Skia PDF. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f6271115de
part 5 - Rework the macOS printing code to get rid of the hacks that create a new PrintTarget for each page. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fdab31ab51
part 6 - Add page start/end arguments to the PrintTarget::BeginPrinting virtual method and overloads. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2046ba27486
part 7 - Restructure the PMSessionBeginCGDocumentNoDialog related code to live in PrintTargetCG. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecb0db80f15
part 8 - Implement printing via Skia PDF for macOS (behind pref print.print_via_pdf_encoder=skia-pdf). r=lsalzman
On the push after this landed, osx static builds started failing like https://treeherder.mozilla.org/logviewer.html#?job_id=39956059&repo=mozilla-inbound and haven't stopped failing since.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bc39d8cde9ab7f7615a0098f17396691c7354e86
Flags: needinfo?(jwatt)
If it matters, the static build on the push after yours was a clobber build.
(Assignee)

Comment 29

3 years ago
Apparently you're supposed to clobber whenever you rename a file that creates an object file with same name as the previous file (as I did in part 5, renaming PrintTargetCG.cpp to PrintTargetCG.mm). I'll reland touching CLOBBER.
Flags: needinfo?(jwatt)

Comment 30

3 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44360d87a266
part 4 - Implement a PrintTarget sub-class for printing via Skia PDF. r=lsalzman

Comment 31

3 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c8bd440978
part 5 - Rework the macOS printing code to get rid of the hacks that create a new PrintTarget for each page. r=lsalzman
Were there other things still needing to be re-landed here? I backed out patches 4-8 but you've only relanded 4-5.
Flags: needinfo?(jwatt)

Comment 33

3 years ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed482c5658d
part 6 - Add page start/end arguments to the PrintTarget::BeginPrinting virtual method and overloads. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/39c61d16e30f
part 7 - Restructure the PMSessionBeginCGDocumentNoDialog related code to live in PrintTargetCG. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/749e21617b58
part 8 - Implement printing via Skia PDF for macOS (behind pref print.print_via_pdf_encoder=skia-pdf). r=lsalzman
(Assignee)

Comment 34

3 years ago
(In reply to Wes Kocher (:KWierso) from comment #32)
> Were there other things still needing to be re-landed here? I backed out
> patches 4-8 but you've only relanded 4-5.

I wanted to make sure that the file rename in part 5 did indeed stick with a clobber before landing the subsequent patches.
(Assignee)

Comment 35

3 years ago
Comment on attachment 8814390 [details] [diff] [review]
part 8 - Implement printing via Skia PDF for macOS

Markus, I forgot, I also wanted your review of this.
Flags: needinfo?(jwatt)
Attachment #8814390 - Flags: review?(mstange)
(Assignee)

Updated

3 years ago
Blocks: 1321689
(Assignee)

Updated

3 years ago
No longer blocks: 1278259
(Assignee)

Updated

3 years ago
Blocks: 1295109
Blocks: 1322653

Updated

2 years ago
No longer blocks: 1264551
Too late for firefox 52, mass-wontfix.
(Assignee)

Updated

2 years ago
Blocks: 1359329
(Assignee)

Comment 39

2 years ago
Comment on attachment 8814390 [details] [diff] [review]
part 8 - Implement printing via Skia PDF for macOS

All of the patches here have landed so it's a bit misleading to leave it open. I opened bug 1359329 to cover this needinfo and I will close this bug.
Attachment #8814390 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

2 years ago
Depends on: 1360350
Blocks: 1372506
Whiteboard: [behind pref print.print_via_pdf_encoder=skia-pdf]
No longer blocks: 1295109
No longer depends on: 1299848
You need to log in before you can comment on or make changes to this bug.