8000 fix: callback values for printing cancellation and success by codebytere · Pull Request #17400 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: callback values for printing cancellation and success #17400

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
Merged
merged 5 commits into from
Jun 12, 2019
Merged

fix: callback values for printing cancellation and success #17400

merged 5 commits into from
Jun 12, 2019

Conversation

codebytere
Copy link
Member
@codebytere codebytere commented Mar 15, 2019 •
edited
Loading

Description of Change

Resolves #17265.
Resolves #16085.

Callback false when disconnecting from the print job if the current printing job has not completed, and true if printing is triggered successfully.

cc @deepak1556 @jkleinsc

Checklist

Release Notes

Notes: Fixed webContents.print() callback not returning boolean correctly in all cases.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Mar 15, 2019
@codebytere codebytere requested a review from deepak1556 March 15, 2019 18:26
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Mar 16, 2019
@codebytere codebytere marked this pull request as ready for review April 22, 2019 18:01
@codebytere codebytere requested a review from a team as a code owner April 22, 2019 18:01
@codebytere codebytere changed the title [wip] fix: callback false for printing cancellation fix: callback false for printing cancellation Apr 22, 2019
@popod
Copy link
Contributor
popod commented May 3, 2019

Hi,

Is this PR ready for merge or should you do some more works ? Can I help ?

p.s. I think your should remove the "wip" label.

jkleinsc
jkleinsc requested changes May 3, 2019
View reviewed changes
Copy link
Member
@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.

Looks good to me though I think @deepak1556 should weigh in on this one.

Also, a rebase should resolve the CircleCI issue on the Linux tests.

@codebytere codebytere removed the wip ⚒ label May 3, 2019
@popod
Copy link
Contributor
popod commented May 22, 2019 •
edited
Loading

Hi @deepak1556

Do you have some time to review this small PR ? Huge thanks :)

And is they any backport planned for electron 4.x.x ?

deepak1556
deepak1556 requested changes May 22, 2019
View reviewed changes
Copy link
Member
@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Tested this, although the callback now gets triggered, it doesn't get invoked until app quits or page navigates. Needs further investigation.
8000

@codebytere codebytere force-pushed the printing branch 2 times, most recently from b752a08 to e732655 Compare May 30, 2019 16:15
codebytere
codebytere commented May 30, 2019
View reviewed changes
@codebytere codebytere changed the title fix: callback false for printing cancellation [wip] fix: callback false for printing cancellation May 30, 2019
@codebytere codebytere requested a review from deepak1556 May 30, 2019 17:43
@codebytere codebytere changed the title [wip] fix: callback false for printing cancellation fix: callback false for printing cancellation May 30, 2019
codebytere
codebytere commented May 30, 2019
View reviewed changes
patches/common/chromium/printing.patch Outdated
DCHECK(query);

- // Disconnect the current |print_job_|.
- DisconnectFromCurrentPrintJob();
Copy link
Member Author
@codebytere codebytere May 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we don't want to call this twice, since it prevents printing_succeeded_ from ever being set to true and thus causes #17265

@codebytere codebytere requested a review from jkleinsc May 30, 2019 17:50
@codebytere codebytere changed the title fix: callback false for printing cancellation fix: callback values for printing cancellation and success May 30, 2019
jkleinsc
jkleinsc approved these changes May 30, 2019
View reviewed changes
Copy link
Member
@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.

LGTM

@inukshuk inukshuk mentioned this pull request Jun 3, 2019
Merged
15 tasks
@popod
Copy link
Contributor
popod commented Jun 4, 2019

@codebytere thanks for your works ! And I hope that we could use this fix soon ;) Is there any backport to 4.x.x planned ?

deepak1556
deepak1556 requested changes Jun 7, 2019
View reviewed changes
Copy link
Member
@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM, with some changes.

patches/chromium/printing.patch
- case JobEventDetails::USER_INIT_DONE:
- case JobEventDetails::DEFAULT_INIT_DONE:
case JobEventDetails::USER_INIT_CANCELED: {
+ ReleasePrintJob();
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't it be better to call TerminatePrintJob ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Both should work! either way the callback is called with false :)

patches/chromium/printing.patch
DCHECK(query);

- // Disconnect the current |print_job_|.
- DisconnectFromCurrentPrintJob();
Copy link
Member

Choose a reason for hiding this comment

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

whats the reason behind this change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

#17400 (review)

patches/chromium/printing.patch
DCHECK(!print_job_);
print_job_ = base::MakeRefCounted<PrintJob>();
print_job_->Initialize(query, RenderSourceName(), number_pages_);
- registrar_.Add(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
Copy link
Member

Choose a reason for hiding this comment

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

why change the registrar source ? The notification from PrintJobWorker posts in on print_job_ , so it will be received by PrintViewManagerBase who is listening on it.

deepak1556
deepak1556 approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member
@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

While I am against the idea of using chrome notification observer for all sources on an object that may never be created, its not straightforward to get the response from acquiring print settings back to the user. This needs a refactor which reuses the print preview code to handle printing, which is better to be done as a follow up.

< 9E88 div class="pr-review-reactions">
@codebytere codebytere merged commit ec10fd3 into master Jun 12, 2019
@release-clerk
Copy link
release-clerk bot commented Jun 12, 2019

Release Notes Persisted

Fixed webContents.print() callback not returning boolean correctly in all cases.

@trop
Copy link
Contributor
trop bot commented Jun 12, 2019

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

@trop trop bot removed the target/6-0-x label Jun 12, 2019
@trop
Copy link
Contributor
trop bot commented Jun 12, 2019

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

@codebytere codebytere deleted the printing branch June 12, 2019 19:36
This was referenced Jun 14, 2019
Merged
Merged
@popod popod mentioned this pull request Jun 25, 2019
Closed
3 tasks
@PalmerAL PalmerAL mentioned this pull request Jul 31, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

Assignees
No one assigned
Labels
component/printing semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

webContents.print() callback always return false Fix callback for webContents.print() cancel event
4 participants
@codebytere @popod @jkleinsc @deepak1556
0