Skip to content

refactor: printToPDF should be headless - #33654

Merged
codebytere merged 30 commits into
mainfrom
print-to-pdf-wip
May 31, 2022
Merged

refactor: printToPDF should be headless#33654
codebytere merged 30 commits into
mainfrom
print-to-pdf-wip

Conversation

@codebytere

@codebytere codebytere commented Apr 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Description of Change

BREAKING CHANGE

Closes #24947.
Closes #30753.
Closes #27605.
Closes #29324.

This PR refactors our printToPDF implementation to match that of Chromium's headless implementation, seen here. Per https://bugs.chromium.org/p/chromium/issues/detail?id=1101596, we'd hit the limit with our previous approach, since with out of process iframes enabled we were unable to properly composite subframes. This removes that hacked-together implementation in favor of headless'.

Checklist

Release Notes

Notes: Refactored webContents.printToPDF to align with the Chrome Devtools implementation.

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Apr 7, 2022
@codebytere
codebytere force-pushed the print-to-pdf-wip branch 2 times, most recently from 0239b82 to b84824b Compare April 8, 2022 14:19
@codebytere codebytere changed the title wip: refactor printToPDF to use headless where appropriate refactor: printToPDF to use headless where appropriate Apr 8, 2022
@codebytere codebytere added the semver/major incompatible API changes label Apr 11, 2022
@codebytere
codebytere force-pushed the print-to-pdf-wip branch 2 times, most recently from 1b29ac3 to 9bdd930 Compare April 11, 2022 11:11
@itsananderson

itsananderson commented Apr 11, 2022

Copy link
Copy Markdown
Member

API LGTM

itsananderson
itsananderson approved these changes Apr 11, 2022
View reviewed changes

@itsananderson itsananderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I forgot how Github's review workflow works and didn't submit my comment.

Comment thread docs/api/web-contents.md Outdated
@codebytere codebytere changed the title refactor: printToPDF to use headless where appropriate refactor: printToPDF should be headless Apr 11, 2022
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Apr 14, 2022
@pushkin-

pushkin- commented Apr 27, 2022

Copy link
Copy Markdown

@codebytere By the way, I'm seeing a similar issue when using the print method instead of printToPDF on a window that's loading a PDF. Will that also be fixed by this PR, or should I create a new issue?

@codebytere

codebytere commented May 9, 2022

Copy link
Copy Markdown
Member Author

@pushkin- that's a totally different problem & there are already open issues for it :)

@codebytere
codebytere marked this pull request as ready for review May 9, 2022 12:57
@codebytere
codebytere requested review from a team as code owners May 9, 2022 12:57
@pushkin-

pushkin- commented May 9, 2022

Copy link
Copy Markdown

@pushkin- that's a totally different problem & there are already open issues for it :)

Cool, I found this one. If that's the one you're talking about, can the "blocked" label be removed since there is a repro gist now? And can the version tag be updated? Thank you @codebytere

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label May 12, 2022
ckerr
ckerr reviewed May 13, 2022
View reviewed changes

@ckerr ckerr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full review forthcoming; just sending this one suggestion out for rn

Comment thread docs/api/web-contents.md
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label May 16, 2022
@jkleinsc jkleinsc mentioned this pull request May 17, 2022
Merged
4 tasks
jkleinsc
jkleinsc reviewed May 17, 2022
View reviewed changes

@jkleinsc jkleinsc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

ckerr
ckerr reviewed May 17, 2022
View reviewed changes

@ckerr ckerr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM although if possible, backwards compatiability discussed here would be nice-to-have

codebytere added 12 commits May 24, 2022 11:41
@codebytere
chore: remove patch redundancy
53ba1eb
@codebytere
test: fix printToPDF parameter types spec
483dcb6
@codebytere
fix: add back ignore_invalid_page_ranges
69101f9
@codebytere
chore: fix pageRanges param
a492a6e
@codebytere
docs: clarify headerTemplate
ab7648b
@codebytere
chore: handle rebase changes
bf9552d
@codebytere
3579838: Add failure reason to PrintManagerHost::PrintingFailed()
4c29849 
https://chromium-review.googlesource.com/c/chromium/src/+/3579838
@codebytere
3583139: Remove support for ignore_invalid_page_ranges when printing PDF
e64be1b 
https://chromium-review.googlesource.com/c/chromium/src/+/3583139
@codebytere
3581548: Pass page ranges rather than list of pages to print in Print…
e1473ce 
…PagesParams

https://chromium-review.googlesource.com/c/chromium/src/+/3581548
@codebytere
3586363: Introduce PrintRenderFrame.PrintWithParams() for batch print…
e4ea2d8 
…ing to PDF

https://chromium-review.googlesource.com/c/chromium/src/+/3586363
@codebytere
3583239: Pass max uint value for end of open page range
a88c111 
https://chromium-review.googlesource.com/c/chromium/src/+/3583239
@codebytere
chore: fixup lint
db2be74
@codebytere
codebytere requested review from ckerr and jkleinsc May 24, 2022 10:43
VerteDinde
VerteDinde approved these changes May 26, 2022
View reviewed changes

@VerteDinde VerteDinde left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time reviewing this yesterday on both Mac and Windows, using these two gists for testing (one, two). I didn't hit any errors, and the code looks good to me. Happy to do some smoke testing on Linux, but this looks good to me 👍

@codebytere
codebytere requested a review from zcbenz May 30, 2022 08:37
zcbenz
zcbenz approved these changes May 31, 2022
View reviewed changes

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API LGTM

@codebytere
codebytere merged commit 93b39b9 into main May 31, 2022
@codebytere
codebytere deleted the print-to-pdf-wip branch May 31, 2022 06:21
@release-clerk

release-clerk Bot commented May 31, 2022

Copy link
Copy Markdown

Release Notes Persisted

Refactored webContents.printToPDF to align with the Chrome Devtools implementation.

@ghost ghost mentioned this pull request Aug 1, 2022
Open
@sassouiabd sassouiabd mentioned this pull request Aug 30, 2022
Closed
@NguyenTranHoangSym NguyenTranHoangSym mentioned this pull request Oct 17, 2022
Closed
@88250 88250 mentioned this pull request Oct 22, 2022
Closed
@t3chguy t3chguy mentioned this pull request Nov 18, 2022
Merged
@ErikSin ErikSin mentioned this pull request Jan 26, 2023
Closed
3 tasks
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@codebytere @khalwa
refactor: printToPDF should be headless (electron#33654)
b478543
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VerteDinde VerteDinde VerteDinde approved these changes
@ckerr ckerr Awaiting requested review from ckerr
@jkleinsc jkleinsc Awaiting requested review from jkleinsc
+2 more reviewers
@itsananderson itsananderson itsananderson approved these changes
@zcbenz zcbenz zcbenz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-review/approved ✅ no-backport semver/major incompatible API changes

Projects

None yet

Milestone

No milestone

7 participants

@codebytere @itsananderson @pushkin- @ckerr @jkleinsc @zcbenz @VerteDinde