Report print jobs that are aborted by CUPS as errors.

Report print jobs that are cancelled by CUPS as errors.

If a print job encounters a problem, it can sometimes be cancelled by
CUPS or the printer.  We will now notify the user of an error in this 
situation.  If a user cancels a print job, the behavior remains the same.

BUG=682853, 684853
Review-Url: https://codereview.chromium.org/2708233006
Cr-Commit-Position: refs/heads/master@{#453025}
Committed: https://chromium.googlesource.com/chromium/src/+/b5d49127cc30716341b904fd9cfc6e26e68b6d5d

[skau, 2017-02-24 00:45:00]

Description was changed from

==========
Report print jobs that are aborted by CUPS as errors.

If a print job encounters a problem, it can sometimes be aborted by
CUPS or the printer.  This situation is currently unhandled.  If a user
aborts a print job, the behavior remains the same.

BUG=682853,684853
==========

to

==========
Report print jobs that are cancelled by CUPS as errors.

If a print job encounters a problem, it can sometimes be cancelled by
CUPS or the printer.  We will now notify the user of an error in this 
situation.  If a user cancels a print job, the behavior remains the same.

BUG=682853,684853
==========

[skau, 2017-02-24 00:45:00]

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

[skau, 2017-02-24 00:46:12]

xdai@: It turns out, jobs can be cancelled without clicking the button.  Who
knew?

[xdai1, 2017-02-24 19:17:01]

Yes I think that's explains the occasional crash. Thanks for digging it out!

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:90: 
I think we're not supposed to get any updates after the print job is cancelled
by user?

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:64: auto
notification = notification_map_.find(job);
Why not use the same format in the other functions? e.g.:
DCHECK(base::ContainsKey(notification_map_, job));
notification_map_[job]->OnPrintJobStatusUpdated();

[skau, 2017-02-24 23:09:34]

PTAL

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:90: 
On 2017/02/24 19:17:00, xdai1 wrote:
> I think we're not supposed to get any updates after the print job is cancelled
> by user?

The previous behavior was that we never received any events that indicated
cancellation.  I could suppress the CANCELLED events but it would require
deleting the CupsPrintJob object at an unpredictable time.  This is safer.

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2708233006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:64: auto
notification = notification_map_.find(job);
On 2017/02/24 19:17:00, xdai1 wrote:
> Why not use the same format in the other functions? e.g.:
> DCHECK(base::ContainsKey(notification_map_, job));
> notification_map_[job]->OnPrintJobStatusUpdated();

I've changed it.  I had thought notifications were removed from the map after
they're dismissed but it seems like this never happens.

[xdai1, 2017-02-24 23:31:52]

lgtm with questions

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification.h (right):

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.h:10: #include
<vector>
Just curious: Normally I don't proactively add more head files if the compiler
doesn't complain. Any guide rules about when should add more head files?

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.h:74:
DISALLOW_COPY_AND_ASSIGN(CupsPrintJobNotification);
Do you also need include "base/macros.h" since you're adding included header
files?

[skau, 2017-02-25 00:16:06]

Thanks for the review.  Questions answered.

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification.h (right):

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.h:10: #include
<vector>
On 2017/02/24 23:31:52, xdai1 wrote:
> Just curious: Normally I don't proactively add more head files if the compiler
> doesn't complain. Any guide rules about when should add more head files? 

It shows up as a linter warning when I run 'git cl lint'.  It also improves
build stability in the case where you're getting <vector> from  a different
header file then that header removes it.  The build will break in an unexpected
way.

https://codereview.chromium.org/2708233006/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification.h:74:
DISALLOW_COPY_AND_ASSIGN(CupsPrintJobNotification);
On 2017/02/24 23:31:52, xdai1 wrote:
> Do you also need include "base/macros.h" since you're adding included header
> files?

Done.

[skau, 2017-02-25 00:16:11]

The CQ bit was checked by skau@chromium.org

[skau, 2017-02-25 00:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2708233006/#ps60001
(title: "address comments from xdai")

[commit-bot: I haz the power, 2017-02-25 00:16:47]

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-02-25 00:48:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487981771707120,
"parent_rev": "f788c53efa1ace1ef43714293ee37b844fc9e67a", "commit_rev":
"b5d49127cc30716341b904fd9cfc6e26e68b6d5d"}

[commit-bot: I haz the power, 2017-02-25 00:49:03]

Description was changed from

==========
Report print jobs that are cancelled by CUPS as errors.

If a print job encounters a problem, it can sometimes be cancelled by
CUPS or the printer.  We will now notify the user of an error in this 
situation.  If a user cancels a print job, the behavior remains the same.

BUG=682853,684853
==========

to

==========
Report print jobs that are cancelled by CUPS as errors.

If a print job encounters a problem, it can sometimes be cancelled by
CUPS or the printer.  We will now notify the user of an error in this 
situation.  If a user cancels a print job, the behavior remains the same.

BUG=682853,684853

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

[commit-bot: I haz the power, 2017-02-25 00:49:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b5d49127cc30716341b904fd9cfc...