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.

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

fix: crash in window.print() #19690

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@codebytere
Copy link
Member

commented Aug 8, 2019

Description of Change

Closes #19687.

window.print() also calls InitPrintSettings and would not receive the settings received when we call webContents.print() so we need to send PrintHostMsg_GetDefaultPrintSettings over IPC in that case.

cc @jkleinsc @deepak1556

Checklist

Release Notes

Notes: Fixed a crash in window.print().

@codebytere codebytere requested a review from electron/wg-upgrades as a code owner Aug 8, 2019

@codebytere codebytere requested review from jkleinsc and deepak1556 Aug 8, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Aug 9, 2019

@codebytere codebytere requested a review from brenca Aug 9, 2019

@brenca

brenca approved these changes Aug 9, 2019

Copy link
Member

left a comment

LGTM!

@deepak1556
Copy link
Member

left a comment

Can you add a test for this, thanks! it can only be a basic api test but better to avoid situations where code path is accidentally removed.

@codebytere codebytere force-pushed the window-print branch 2 times, most recently from 55c8cf2 to d341b18 Aug 10, 2019

@codebytere

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@deepak1556 the test itself passes but the native printer dialog pops when window.print() gets called and blocks so the tests time out every time. How would you suggest we proceed here?

The webContents.print() tests don't block since they don't invoke the dialog and can stay, but we may not be able to programmatically test window.print() itself.

@deepak1556

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Can you test with silent code path for window.print ?

@codebytere codebytere force-pushed the window-print branch from d341b18 to 919137d Aug 12, 2019

@codebytere codebytere requested a review from deepak1556 Aug 12, 2019

@deepak1556
Copy link
Member

left a comment

LGTM since we can't UI test this without introducing testing apis in printing code.

@codebytere codebytere merged commit e8fa248 into master Aug 13, 2019

13 of 14 checks passed

Artifact Comparison Changes Detected
Details
Backportable? - 7-0-x Clean Backport
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190812.34 succeeded
Details
electron-arm64-testing Build #20190812.32 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Aug 13, 2019

Release Notes Persisted

Fixed a crash in window.print().

@codebytere codebytere deleted the window-print branch Aug 13, 2019

@trop trop bot referenced this pull request Aug 13, 2019
@trop

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

I have automatically backported this PR to "7-0-x", please check out #19728

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