Closed Bug 1409971 Opened 7 years ago Closed 7 years ago

Cancelling the print job doesn't work on GTK / Windows.

Categories

(Core :: Printing: Setup, defect, P3)

Product:
Core
Component:
Printing: Setup
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(3 files)

Bug 1409971 - Part 1. Add cancelling print job interface to PPrintProgressDialog.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1409971 - Part 2. Notify cancelling print job when cancel button pushed.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1409971 - Part 3. Set nsIPrintSettings::IsCancelled to true in order to cancel print job.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
Current print progress dialog of GTK is XUL dialog. When user push the cancel button of this dialog, gecko will mark print job settings to cancel.[1]
However this mark doesn't pass to the parent process on e10s environment, as result, we can't cancel the print job.

This phenomenon will occur on e10s only.

Note:
 * macOS doesn't have this problem due to doesn't have this progress dialog.
 * windows doesn't set this parameter, so I think windows has this problem before e10s. (Maybe bug 424965)

[1] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/toolkit/components/printingui/unixshared/nsPrintProgress.cpp#136

STR:
 1) Open long page(e.g. DisplayList.cpp of dxr).
 2) Click the Print or Print Preview
 3) Click cancel button of progress dialog

Actually Result:
 We can't cancel print job.

Expected Result:
 Pushing the cancel button affect canceling the print job.
Do you know if this has always been broken with e10s, vs. if this is a more recent regression?

(Regardless, calling this wontfix for 57 since the risk/reward tolerance for changes there is pretty strict at this point. Though if this happens to be a regression *in 57*, that might merit reconsideration.)
status-firefox57: --- → wontfix
Flags: needinfo?(mantaroh)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Do you know if this has always been broken with e10s, vs. if this is a more
> recent regression?
> 
> (Regardless, calling this wontfix for 57 since the risk/reward tolerance for
> changes there is pretty strict at this point. Though if this happens to be a
> regression *in 57*, that might merit reconsideration.)

As far as I can see, this phenomenon happened after bug 1090448 in my linux environment.
So I think this is not recent regression but bug of enabling e10s printing.
Flags: needinfo?(mantaroh)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> (In reply to Daniel Holbert [:dholbert] from comment #1)
> > Do you know if this has always been broken with e10s, vs. if this is a more
> > recent regression?
> > 
> > (Regardless, calling this wontfix for 57 since the risk/reward tolerance for
> > changes there is pretty strict at this point. Though if this happens to be a
> > regression *in 57*, that might merit reconsideration.)
> 
> As far as I can see, this phenomenon happened after bug 1090448 in my linux
> environment.
> So I think this is not recent regression but bug of enabling e10s printing.

I guess we need to send notification of cancelling the print job via IPC.

This is WIP patch:
https://hg.mozilla.org/try/rev/bb708e83e902f3fe7008f9c4338a631ac94ab634
Assignee: nobody → mantaroh
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #4)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #2)
> > (In reply to Daniel Holbert [:dholbert] from comment #1)
> > > Do you know if this has always been broken with e10s, vs. if this is a more
> > > recent regression?
> > > 
> > > (Regardless, calling this wontfix for 57 since the risk/reward tolerance for
> > > changes there is pretty strict at this point. Though if this happens to be a
> > > regression *in 57*, that might merit reconsideration.)
> > 
> > As far as I can see, this phenomenon happened after bug 1090448 in my linux
> > environment.
> > So I think this is not recent regression but bug of enabling e10s printing.
> 
> I guess we need to send notification of cancelling the print job via IPC.
> 
> This is WIP patch:
> https://hg.mozilla.org/try/rev/bb708e83e902f3fe7008f9c4338a631ac94ab634

We need to following changes :
 * Open the traffic of cancelling job via ipc.
 * Have a member which reference to ipc parent.
 * Call this new ipc interface from nsPrintProgress::SetProcessCanceledByUser().
 * Receive notifying cancel job on child process, and should nsPrintSettings::isCancelled to true.
 * These change work only e10s environment.

Note:
 * nsPrintProgress() created by nsPrintingPromptService, so we can't pass ipc parent(like PrintingParent) pointer directly. So we need to pass XPCOM interface like nsIObserver. Actually PrintProgressDialog uses nsIObserver.[1]
 * We can use this nsIObserver(i.e. PrintProgressDialogParent).

[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/toolkit/components/printingui/ipc/PrintProgressDialogParent.cpp#96
Comment on attachment 8928424 [details]
Bug 1409971 - Part 1. Add cancelling print job interface to PPrintProgressDialog.

https://reviewboard.mozilla.org/r/199654/#review205628

Thanks!
Attachment #8928424 - Flags: review?(mconley) → review+
Comment on attachment 8928425 [details]
Bug 1409971 - Part 2. Notify cancelling print job when cancel button pushed.

https://reviewboard.mozilla.org/r/199656/#review205630
Attachment #8928425 - Flags: review?(mconley) → review+
Comment on attachment 8928426 [details]
Bug 1409971 - Part 3. Set nsIPrintSettings::IsCancelled to true in order to cancel print job.

https://reviewboard.mozilla.org/r/199658/#review205632

A very straight-forward set of patches. Thank you!
Attachment #8928426 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #11)
> Comment on attachment 8928426 [details]
> Bug 1409971 - Part 3. Set nsIPrintSettings::IsCancelled to true in order to
> cancel print job.
> 
> https://reviewboard.mozilla.org/r/199658/#review205632
> 
> A very straight-forward set of patches. Thank you!

Thanks!
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/983d715192fe
Part 1. Add cancelling print job interface to PPrintProgressDialog. r=mconley
https://hg.mozilla.org/integration/autoland/rev/154ec3c19c88
Part 2. Notify cancelling print job when cancel button pushed. r=mconley
https://hg.mozilla.org/integration/autoland/rev/b78cea997533
Part 3. Set nsIPrintSettings::IsCancelled to true in order to cancel print job. r=mconley
https://hg.mozilla.org/mozilla-central/rev/983d715192fe
https://hg.mozilla.org/mozilla-central/rev/154ec3c19c88
https://hg.mozilla.org/mozilla-central/rev/b78cea997533
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1419775
Depends on: 1441598
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: