Skip to content

/electron

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.

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

refactor: printing #15143

Merged
merged 3 commits into from Nov 9, 2018
+780 −1,073

Conversation

Reviewers

@brenca brenca

@jkleinsc jkleinsc

@MarshallOfSound MarshallOfSound

@ckerr ckerr

@nornagon nornagon

@alexeykuzmin alexeykuzmin

@zcbenz zcbenz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@deepak1556 @popod @bengotow @jkleinsc @brenca @MarshallOfSound
@deepak1556
Copy link
Member

commented Oct 13, 2018 •
edited by MarshallOfSound

Description of Change

Followup for #15023

  • Registers pdf compositor service for generating pdf with OOPIF
  • Cleans up printing sources and dependencies

Fixes #14260
Fixes #9537
Fixes #15188

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines

Release Notes

Notes: no-notes

@deepak1556 deepak1556 requested a review from Oct 13, 2018

@deepak1556 deepak1556 requested a review from as a code owner Oct 13, 2018

@jkleinsc jkleinsc referenced this pull request Nov 1, 2018

Closed

Printing causes the process to crash #15188

@deepak1556 deepak1556 force-pushed the printing_cleanup branch 3 times, most recently from 1677054 to b9ce69b Nov 2, 2018

@deepak1556 deepak1556 changed the title [WIP] refactor: printing refactor: printing Nov 4, 2018

@deepak1556 deepak1556 requested review from nornagon, zcbenz, alexeykuzmin, brenca and ckerr Nov 4, 2018

@deepak1556 deepak1556 added the target/4-0-x label Nov 4, 2018

@deepak1556 deepak1556 force-pushed the printing_cleanup branch from b9ce69b to a57ac9f Nov 4, 2018

@deepak1556 deepak1556 referenced this pull request Nov 6, 2018

Open

Only first page prints under Electron 3 using webContents.print() #14260

@deepak1556 deepak1556 force-pushed the printing_cleanup branch from a57ac9f to f87cf1a Nov 7, 2018

@jkleinsc

jkleinsc approved these changes Nov 8, 2018
View changes

Copy link
Contributor

left a comment

LGTM
@brenca

brenca reviewed Nov 8, 2018
View changes

Copy link
Member

left a comment •
edited

I see that you added isPrintingEnabled to put the specs behind a flag, but I can't see any changes to the specs themselves. Was that intentional? Otherwise LGTM!

deepak1556 added some commits Oct 12, 2018

@deepak1556
refactor: basic printing 
* move build files to chromium_src/BUILD.gn
* remove dependency on chrome prerender sources
3ff90d1
@deepak1556
spec: move printing specs behind feature flag
39fe851
@deepak1556
build: register pdf compositor service
Loading status checks…
734ab4b

@deepak1556 deepak1556 force-pushed the printing_cleanup branch from f87cf1a to 734ab4b Nov 8, 2018

@deepak1556

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 8, 2018

but I can't see any changes to the specs themselves. Was that intentional?

@brenca missed isolating the specs, have fixed them now. Thanks!
@MarshallOfSound

MarshallOfSound approved these changes Nov 9, 2018
View changes

Copy link
Member

left a comment

LGTM

@MarshallOfSound MarshallOfSound merged commit 8232296 into master Nov 9, 2018
21 checks passed

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Sign in to view
Copy link

commented Nov 9, 2018

No Release Notes

@MarshallOfSound MarshallOfSound deleted the printing_cleanup branch Nov 9, 2018

@trop

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 9, 2018

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.
@popod

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 13, 2018

Hi @deepak1556 ! Where can I follow the achievement of backporting this PR for 3-x-x as said in some of the related issues ? Thanks
@bengotow

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 26, 2018 •
edited

Hey folks—based on the bot's comment above, I guess this wasn't included in the latest 3.x or 4.x release? Would be super great to get this into a release branch! (cc @MarshallOfSound , @deepak1556 )

bengotow added a commit to bengotow/electron that referenced this pull request Nov 29, 2018

@bengotow
refactor: printing implementation (electron#15143) 
* refactor: basic printing

* move build files to chromium_src/BUILD.gn
* remove dependency on chrome prerender sources

* spec: move printing specs behind feature flag

* build: register pdf compositor service
912e39f
@popod

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 30, 2018 •
edited

Hi @deepak1556 @brenca @MarshallOfSound !

Sorry for spamming you..

.. but it will be nice to give us a little "feedback" about all that.. Let me explain myself: about a month ago, you wrote that #14260 resolved in this PR will be available soon on 3-x-x. But you give us no more informations after that.. Is something blocking you? Are you waiting for some more informations?

My desire is to resolve the #14260 bug (only the first page of a document is print on macOS, but works on Windows) on Electron 3. I don't know if all this PR changes are needed for that, but I think that this will be nice to resolve this bug which dates from August.

As I see, you have done a lot of works to improve and fixe some bug on the printing part of Electron and this will be very nice to let us enjoy it :)

Thank you for your fantastic job!
One more time, sorry about spamming and thank you in advance for your reply

bengotow added a commit to bengotow/electron that referenced this pull request Dec 7, 2018

@deepak1556 @bengotow
refactor: printing implementation (electron#15143) 
* refactor: basic printing

* move build files to chromium_src/BUILD.gn
* remove dependency on chrome prerender sources

* spec: move printing specs behind feature flag

* build: register pdf compositor service
05a5910

@bengotow bengotow referenced this pull request Dec 7, 2018

Merged

chore: backport printing changes (#15023, #15143) to 4-0-x #15976

4 of 5 tasks complete

deepak1556 added a commit that referenced this pull request Dec 14, 2018

@deepak1556
refactor: printing implementation (#15143) 
* refactor: basic printing

* move build files to chromium_src/BUILD.gn
* remove dependency on chrome prerender sources

* spec: move printing specs behind feature flag

* build: register pdf compositor service
a19ba5c

ckerr added a commit that referenced this pull request Dec 17, 2018

@bengotow @ckerr
chore: backport printing changes (#15023, #15143) to 4-0-x (#15976) 
* refactor: move printing out of chromium_src (#15023)

* remove printing related things from chromium_src

* chore: add printing build flag and patch

* fix: include PrintingService on other platforms too

* fix: printing_handler is only needed on Windows

* fix: format BUILD.gn properly

* fix: rename printing build flag to avoid conflict with chromium

* fix: place previously missed printing calls behind build flag

* fix: accidentally renamed flag in patch file

* fix: don't include all printing strings

* fix: allow ShowItemInFolder and OpenItem to block, fixing a DCHECK crash

* fix: make things compile, some changes got lost while rebasing

* fix: remove rogue line from BUILD.gn

* chore: update patch description

* style: lint fix

* chore: use chromium printing buildflag, move node related stuff out of patch

* revert: remove ScopedAllowBlockingForTesting call

* fix: fix my rebase blooper

* fix: re-add header lost during rebase, update patch

* fix: add <map> include, tweak the patch a bit

* revert: remove rogue diff from patch

* fix: clean up after rebase

* refactor: printing implementation (#15143)

* refactor: basic printing

* move build files to chromium_src/BUILD.gn
* remove dependency on chrome prerender sources

* spec: move printing specs behind feature flag

* build: register pdf compositor service

* chore: cleanup api usage from //chrome
Loading status checks…
f6fa88a

@javan javan referenced this pull request Jan 1, 2019

Closed

Electron 4.0.0: webContents.print() must be called twice or with an empty object #16219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.