Closed
Bug 1309272
Opened 8 years ago
Closed 7 years ago
Implement printing via Skia PDF for macOS
Categories
(Core :: Printing: Output, defect)
Core
Printing: Output
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox52 | --- | wontfix |
People
(Reporter: jwatt, Assigned: jwatt)
References
(Blocks 3 open bugs)
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 files, 8 obsolete files)
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 |
part 6 - Add page start/end arguments to the PrintTarget::BeginPrinting virtual method and overloads
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 |
Use SkPDF to print whenever PDF is supported.
Assignee | ||
Updated•8 years ago
|
Component: Graphics → Printing: Output
Comment 1•8 years ago
|
||
Before we do this we need to improve the state of our printing tests.
Depends on: 1299848
Assignee | ||
Comment 2•8 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•8 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 4•8 years ago
|
||
Attachment #8804773 -
Flags: review?(lsalzman)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8804775 -
Flags: review?(lsalzman)
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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() ? ... : ...
Comment 8•8 years ago
|
||
This is required for SkPDF to work, since it only gives us an SkCanvas without a wrapping SkSurface.
Attachment #8804825 -
Flags: review?(mchang)
Updated•8 years ago
|
Attachment #8804825 -
Flags: review?(mchang) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8804771 -
Attachment is obsolete: true
Attachment #8804771 -
Flags: review?(lsalzman)
Attachment #8805359 -
Flags: review?(lsalzman)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8804773 -
Attachment is obsolete: true
Attachment #8804773 -
Flags: review?(lsalzman)
Attachment #8805360 -
Flags: review?(lsalzman)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8805360 -
Attachment is obsolete: true
Attachment #8805360 -
Flags: review?(lsalzman)
Attachment #8805363 -
Flags: review?(lsalzman)
Comment 12•8 years ago
|
||
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•8 years ago
|
||
Attachment #8805359 -
Attachment is obsolete: true
Attachment #8805359 -
Flags: review?(lsalzman)
Attachment #8805399 -
Flags: review?(lsalzman)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8805363 -
Attachment is obsolete: true
Attachment #8805363 -
Flags: review?(lsalzman)
Attachment #8805400 -
Flags: review?(lsalzman)
Updated•8 years ago
|
Attachment #8805400 -
Flags: review?(lsalzman) → review+
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8805399 -
Attachment is obsolete: true
Attachment #8805399 -
Flags: review?(lsalzman)
Attachment #8805521 -
Flags: review?(lsalzman)
Updated•8 years ago
|
Attachment #8805521 -
Flags: review?(lsalzman) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 16•8 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
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89304c262266 https://hg.mozilla.org/mozilla-central/rev/c974afea821b https://hg.mozilla.org/mozilla-central/rev/ee15183d0572
Comment 18•7 years ago
|
||
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•7 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•7 years ago
|
||
Attachment #8804775 -
Attachment is obsolete: true
Attachment #8814359 -
Flags: review?(lsalzman)
Assignee | ||
Comment 20•7 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•7 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•7 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•7 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•7 years ago
|
Summary: Use SkPDF (Skia PDF) to print whenever PDF is supported → Implement printing via Skia PDF for macOS
Assignee | ||
Updated•7 years ago
|
User Story: (updated)
Assignee | ||
Comment 24•7 years ago
|
||
Attachment #8814367 -
Attachment is obsolete: true
Attachment #8814367 -
Flags: review?(lsalzman)
Attachment #8814390 -
Flags: review?(lsalzman)
Assignee | ||
Comment 25•7 years ago
|
||
Try seems happy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e63234d8948ee5677bcc5465a29a3481fbaab1
Updated•7 years ago
|
Attachment #8814359 -
Flags: review?(lsalzman) → review+
Updated•7 years ago
|
Attachment #8814364 -
Flags: review?(lsalzman) → review+
Updated•7 years ago
|
Attachment #8814362 -
Flags: review?(lsalzman) → review+
Updated•7 years ago
|
Attachment #8814366 -
Flags: review?(lsalzman) → review+
Updated•7 years ago
|
Attachment #8814390 -
Flags: review?(lsalzman) → review+
Comment 26•7 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•7 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•7 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•7 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•7 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•7 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•7 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)
Comment 36•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/44360d87a266 https://hg.mozilla.org/mozilla-central/rev/c5c8bd440978
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2ed482c5658d https://hg.mozilla.org/mozilla-central/rev/39c61d16e30f https://hg.mozilla.org/mozilla-central/rev/749e21617b58
Comment 38•7 years ago
|
||
Too late for firefox 52, mass-wontfix.
Assignee | ||
Comment 39•7 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•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Whiteboard: [behind pref print.print_via_pdf_encoder=skia-pdf]
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•