Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: printToPDF should be headless #33654

Merged
merged 30 commits into from May 31, 2022
Merged

refactor: printToPDF should be headless #33654

merged 30 commits into from May 31, 2022

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Apr 7, 2022 •
edited

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 in the last 24 hours 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
Copy link
Contributor

API LGTM

itsananderson
itsananderson approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@itsananderson itsananderson left a comment

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.

docs/api/web-contents.md Outdated Show resolved Hide resolved
@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 in the last 24 hours label Apr 14, 2022
@pushkin-
Copy link

@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
Copy link
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-
Copy link

pushkin- commented May 9, 2022

@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 in the last 24 hours label May 12, 2022
ckerr
ckerr reviewed May 13, 2022
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

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

docs/api/web-contents.md Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours 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
Copy link
Contributor

@jkleinsc jkleinsc left a comment

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
Copy link
Member

@ckerr ckerr left a comment

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
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
Copy link
Member

@VerteDinde VerteDinde left a comment

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
Copy link
Member

@zcbenz zcbenz left a comment

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
Copy link

release-clerk bot commented May 31, 2022

Release Notes Persisted

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

@commit-lint commit-lint bot 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

@itsananderson itsananderson itsananderson approved these changes

@zcbenz zcbenz zcbenz approved these changes

@VerteDinde VerteDinde VerteDinde approved these changes

@ckerr ckerr Awaiting requested review from ckerr

@jkleinsc jkleinsc Awaiting requested review from jkleinsc

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