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: default printer if none is provided #21956
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor change
5049e64
to
ac52be5
Compare
ac52be5
to
e43d53f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux crash seems legit, lets investigate before merging
e43d53f
to
312dcbe
Compare
312dcbe
to
d8c0f69
Compare
d8c0f69
to
06c2968
Compare
No Release Notes |
I was unable to backport this PR to "7-1-x" cleanly; |
I was unable to backport this PR to "8-x-y" cleanly; |
@codebytere has manually backported this PR to "8-x-y", please check out #22011 |
print_settings.SetStringKey(printing::kSettingDeviceName, printer_name); | ||
|
||
auto* print_view_manager = | ||
printing::PrintViewManagerBasic::FromWebContents(web_contents()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing discussion about #21986 (comment) , the crash is caused by this line.
This function is registered as a weak callback to the task runner below, so that its not invoked when the api::WebContents
is not available. Based on the crash it seems content::WebContents
is not available in the callback, @zcbenz is it possible for api::WebContents
to be alive while the content::WebContents
is not ?
Description of Change
Related to #21946, but distinct enough a unit of work that i've chosen to make this its own PR. This removes a section of
printing.patch
by choosing the system default if no printer is passed by the user. This is what happened previously at the chromium level, but as a result of not setting thekSettingDeviceName
key; we can instead take control of this ourselves.cc @deepak1556 @ckerr
Checklist
npm test
passesRelease Notes
Notes: Use system default printer if none is provided.