Cleanup jobs which have failed.

Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

TEST=Start job to unreachable printer, let it time out.  Start job to valid printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
BUG=684853
Review-Url: https://codereview.chromium.org/2748253005
Cr-Commit-Position: refs/heads/master@{#457627}
Committed: https://chromium.googlesource.com/chromium/src/+/def538a8cd9e4d982d150d4308e44a2aa4c504ff

[skau, 2017-03-16 01:25:59]

Description was changed from

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

BUG=
==========

to

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

BUG=
TEST=Start job to unreachable printer, let it time out.  Start job to valid
printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
==========

[skau, 2017-03-16 01:25:59]

skau@chromium.org changed reviewers:
+ justincarlson@chromium.org, xdai@chromium.org

[skau, 2017-03-16 01:26:31]

Description was changed from

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

BUG=
TEST=Start job to unreachable printer, let it time out.  Start job to valid
printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
==========

to

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

TEST=Start job to unreachable printer, let it time out.  Start job to valid
printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
BUG=684853
==========

[skau, 2017-03-16 18:28:37]

This should be the last of the fixes for print queue monitoring.  PTAL.

[Carlson, 2017-03-16 18:36:48]

Seems pretty straightforward.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:159: // Must be
run from the UI thread or jobs_ could become inconsistent.
the "or jobs_ could become inconsistent" suffix here probably is something most
casual users won't care about.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:286: break;
Nit:  I think it would be slightly more natural to put the UpdateCurrentPage in
an else block instead of breaking at the end of the if to avoid executing it.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:391:
LOG(WARNING) << "Printer not found";
Would it be useful to add printer_id and/or job_id to this log message and the
one below to help track down what happened?

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:121: //
print_job_ may be deleted by print_job_manager after cancellation.
I don't understand what this comment means.  That it might already be deleted?

[xdai1, 2017-03-16 19:56:17]

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job.h:79: bool expired_ = false;
Maybe we can merge this variable to ErrorCode?

[skau, 2017-03-16 22:14:36]

Thanks for the reviews.  PTAL.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job.h:79: bool expired_ = false;
On 2017/03/16 19:56:16, xdai1 wrote:
> Maybe we can merge this variable to ErrorCode?  

We could.  But I think it'll be easier to maintain with separate fields. 
ErrorCode indicates what the problem is.  expired indicates that the job needs
to be removed from the print queue.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:159: // Must be
run from the UI thread or jobs_ could become inconsistent.
On 2017/03/16 18:36:48, Carlson wrote:
> the "or jobs_ could become inconsistent" suffix here probably is something
most
> casual users won't care about.

Done.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:286: break;
On 2017/03/16 18:36:48, Carlson wrote:
> Nit:  I think it would be slightly more natural to put the UpdateCurrentPage
in
> an else block instead of breaking at the end of the if to avoid executing it.

Done.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:391:
LOG(WARNING) << "Printer not found";
On 2017/03/16 18:36:48, Carlson wrote:
> Would it be useful to add printer_id and/or job_id to this log message and the
> one below to help track down what happened?

I'll log the printer_id.  I don't think the job_id will have much meaning in the
logs.

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:121: //
print_job_ may be deleted by print_job_manager after cancellation.
On 2017/03/16 18:36:48, Carlson wrote:
> I don't understand what this comment means.  That it might already be deleted?

It was supposed to mean that print_job_ is deleted sometime after CancelPrintJob
returned but we didn't guarantee when.  However, it's deleted in CancelPrintJob
now and the comment has been updated.

[Carlson, 2017-03-16 22:22:31]

lgtm

[xdai1, 2017-03-16 22:56:14]

lgtm

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2748253005/diff/1/chrome/browser/chromeos/pri...
chrome/browser/chromeos/printing/cups_print_job.h:79: bool expired_ = false;
On 2017/03/16 22:14:36, skau wrote:
> On 2017/03/16 19:56:16, xdai1 wrote:
> > Maybe we can merge this variable to ErrorCode?  
> 
> We could.  But I think it'll be easier to maintain with separate fields. 
> ErrorCode indicates what the problem is.  expired indicates that the job needs
> to be removed from the print queue.

Acknowledged.

[skau, 2017-03-16 23:19:47]

The CQ bit was checked by skau@chromium.org

[commit-bot: I haz the power, 2017-03-16 23:20:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 23:46:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-16 23:46:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[skau, 2017-03-16 23:58:06]

The CQ bit was checked by skau@chromium.org

[skau, 2017-03-16 23:58:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincarlson@chromium.org, xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2748253005/#ps40001
(title: "fix fake")

[commit-bot: I haz the power, 2017-03-16 23:58:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 00:37:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489708686042160,
"parent_rev": "8e07fc9bf21eaec35f94de57f9ba20cc5a9b009e", "commit_rev":
"def538a8cd9e4d982d150d4308e44a2aa4c504ff"}

[commit-bot: I haz the power, 2017-03-17 00:38:16]

Description was changed from

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

TEST=Start job to unreachable printer, let it time out.  Start job to valid
printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
BUG=684853
==========

to

==========
Cleanup jobs which have failed.

We're stricter about declaring failed jobs than CUPS.  Be sure to remove
the jobs from the print queue when we've given up on them.  Also, ensure
that we don't notify observers after cancellation has been requested.

TEST=Start job to unreachable printer, let it time out.  Start job to valid
printer, click cancel.  Run 'lpstat -t', verify that no print jobs are pending.
BUG=684853

Review-Url: https://codereview.chromium.org/2748253005
Cr-Commit-Position: refs/heads/master@{#457627}
Committed:
https://chromium.googlesource.com/chromium/src/+/def538a8cd9e4d982d150d4308e4...
==========

[commit-bot: I haz the power, 2017-03-17 00:38:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/def538a8cd9e4d982d150d4308e4...