[CUPS] Implement the Print Job notification.

[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing -> Printing in progress -> Printing Done / Failed.
- If the print job is cancelled, the notification is removed from the message center.
- If the notification is closed by the user when showing the printing progress, prevent showing the following printing progress.

BUG=625376, 626752
Committed: https://crrev.com/4639af5eac058bee60267cc3a054b33818b5488c
Cr-Commit-Position: refs/heads/master@{#428798}

[xdai1, 2016-10-22 00:14:12]

Patchset #1 (id:1) has been deleted

[xdai1, 2016-10-22 06:25:24]

Description was changed from

==========
[CUPS] Implement the Print Job notification.

BUG=625376, 626752
==========

to

==========
[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets
updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing
-> Printing in progress -> Printing Done / Failed. 
- If the print job is cancelled, the notification is removed from the message
center.
- If the notification is closed by the user when showing the printing progress,
prevent showing the following printing progress.

BUG=625376, 626752
==========

[xdai1, 2016-10-22 06:26:38]

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

[xdai1, 2016-10-22 06:26:38]

skau@, stevenjb@, could you help review this CL please? Thanks!

skau@: all files
stevenjb@: all files in ui/chromeos/resources

I'll upload a screen recording video next Monday for your reference.

[xdai1, 2016-10-24 23:17:38]

On 2016/10/22 06:26:38, xdai1 wrote:
> skau@, stevenjb@, could you help review this CL please? Thanks!
> 
> skau@: all files
> stevenjb@: all files in ui/chromeos/resources
> 
> I'll upload a screen recording video next Monday for your reference.

FYI: The video was uploaded in
https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ
(titled in "20161024_print_job_notification"). 
Note: The backend is not ready yet, so this CL is only focus on the UI part of
the print job notification.

[xdai1, 2016-10-26 19:36:41]

kindly ping?

[stevenjb, 2016-10-26 19:59:58]

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job.cc (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.cc:18:
total_page_number_(total_page_number) {}
Can we eliminate this and just make callers do the Printer(printer_id) call? In
general we prefer to avoid multiple constructors.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:15: class CUPSPrintJob {
Even though "CUPS" is generally all caps, we tend to use "Cups" for acronyms
that are more than 2 letters, e.g. CupsPrinter in
src/printing/backend/cups_printer.h, so this should be CupsPrintJob.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:17: enum State {
I believe there is a preference now to use 'enum class State'.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:29: enum ErrorCode {
ditto

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:48: std::string GetId() const;
nit: GetUniqueId to make it clear that this is not "printer_id" or "job_id".

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:49: Printer* GetPrinter();
Do we really need this? Passing a non-const pointer to a private member should
generally be avoided.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager.cc (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.cc:16:
AddObserver(notification_manager_.get());
This is a bit backwards. Generally we would pass |this| to the
CupsPrintJobNotificationManager constructor and it would call
print_job_manager_->Add/RemoveObserver.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.h:23: class
CUPSPrintJobManager : public KeyedService {
CupsProntJobManager

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.h:60: virtual void
RemoveObserver(Observer* observer);
Do these need to be virtual?

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
(right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:21: class
CUPSPrintJobNotificationManager : public CUPSPrintJobManager::Observer {
CupsPrintJobNotificationManager

[xdai1, 2016-10-26 22:55:19]

stevenjb@, I've addressed your comments. Please take another look, thanks for
your review!

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job.cc (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.cc:18:
total_page_number_(total_page_number) {}
On 2016/10/26 19:59:57, stevenjb wrote:
> Can we eliminate this and just make callers do the Printer(printer_id) call?
In
> general we prefer to avoid multiple constructors.

Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:15: class CUPSPrintJob {
On 2016/10/26 19:59:57, stevenjb wrote:
> Even though "CUPS" is generally all caps, we tend to use "Cups" for acronyms
> that are more than 2 letters, e.g. CupsPrinter in
> src/printing/backend/cups_printer.h, so this should be CupsPrintJob.

I see. Thanks. Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:17: enum State {
On 2016/10/26 19:59:58, stevenjb wrote:
> I believe there is a preference now to use 'enum class State'.

Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:29: enum ErrorCode {
On 2016/10/26 19:59:58, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:48: std::string GetId() const;
On 2016/10/26 19:59:58, stevenjb wrote:
> nit: GetUniqueId to make it clear that this is not "printer_id" or "job_id".

Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job.h:49: Printer* GetPrinter();
On 2016/10/26 19:59:58, stevenjb wrote:
> Do we really need this? Passing a non-const pointer to a private member should
> generally be avoided.

You're right. It's not necessary. Removed it.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager.cc (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.cc:16:
AddObserver(notification_manager_.get());
On 2016/10/26 19:59:58, stevenjb wrote:
> This is a bit backwards. Generally we would pass |this| to the
> CupsPrintJobNotificationManager constructor and it would call
> print_job_manager_->Add/RemoveObserver.

I see. Modified as you suggested.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.h:23: class
CUPSPrintJobManager : public KeyedService {
On 2016/10/26 19:59:58, stevenjb wrote:
> CupsProntJobManager

Done.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager.h:60: virtual void
RemoveObserver(Observer* observer);
On 2016/10/26 19:59:58, stevenjb wrote:
> Do these need to be virtual?

Not necessary... Removed it. Thanks.

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
(right):

https://codereview.chromium.org/2435303003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:21: class
CUPSPrintJobNotificationManager : public CUPSPrintJobManager::Observer {
On 2016/10/26 19:59:58, stevenjb wrote:
> CupsPrintJobNotificationManager

Done.

[xdai1, 2016-10-26 23:58:00]

Patchset #4 (id:80001) has been deleted

[skau, 2016-10-27 01:43:01]

Generally looks okay.  Some suggestions on the structure.  Feel free to ignore
my suggestions if you like the current structure better.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job.h:32: OUT_OUT_INK,
OUT_OF_INK,

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager.h:55: virtual bool
CancelPrintJob(CupsPrintJob* job) = 0;
Should we change this to CupsPrintJob#UniqueId?

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:127:
DCHECK(print_job_->state() != CupsPrintJob::State::STATE_CANCELLED);
Is this because jobs are canceled by closing the notification?

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:143:
g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
Did you mean to update here?  The comment doesn't seem to match the code.

Also, can you separate the state == STATE_PAGE_DONE case from the others.  You
should note why it's special.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:198: case
CupsPrintJob::State::STATE_SUSPENDED:
Does SUSPENDED belong here?  Do we have no messaging surrounding a paused state?

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:247: for (auto&
iter : *button_commands_.get()) {
You shouldn't need get() here

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:247: for (auto&
iter : *button_commands_.get()) {
iter isn't an iterator, it's a ButtonCommand.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:259:
commands(new std::vector<CupsPrintJobNotification::ButtonCommand>());
MakeUnique is preferred

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:26: if
(notification_map_.find(job) != notification_map_.end())
you can use base::ContainsKey() for this.  It's a bit nicer.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:34:
notification_map_[job]->OnPrintJobStatusUpdated();
If we end up extracting all the information from the job anyway, should we just
have a OnPrintJobStatusChanged() method instead of all the different changes? 
Also, should we pass the job object to the notification here?

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:24:
std::unordered_map<CupsPrintJob*,
Can you use the CupsPrintJob unique id instead of a pointer as the index?

[stevenjb, 2016-10-27 18:52:48]

lgtm

https://codereview.chromium.org/2435303003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2435303003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:16: if
(print_job_manager_)
Given that this class would be pretty pointless without |print_job_manager|,
maybe just DCHECK(print_job_manager) (and remove the test in the destructor
too).

[xdai1, 2016-10-28 23:59:54]

stevenjb@, thanks for the review!
skau@, I've addressed your comments. Please take another look, thanks!

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job.h (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job.h:32: OUT_OUT_INK,
On 2016/10/27 01:43:00, skau wrote:
> OUT_OF_INK,

Done.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager.h:55: virtual bool
CancelPrintJob(CupsPrintJob* job) = 0;
On 2016/10/27 01:43:00, skau wrote:
> Should we change this to CupsPrintJob#UniqueId?

Why? Actually since |job| is owned by CupsPrintJobManager, it should be deleted
in the function CancelPrintJob().

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification.cc (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:127:
DCHECK(print_job_->state() != CupsPrintJob::State::STATE_CANCELLED);
On 2016/10/27 01:43:01, skau wrote:
> Is this because jobs are canceled by closing the notification?

Yes, the notification is gone after the job is cancelled (see line 103 in this
file). And CupsPrintJobNotificationManager doesn't observe the
OnPrintJobCancelled event.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:143:
g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
On 2016/10/27 01:43:00, skau wrote:
> Did you mean to update here?  The comment doesn't seem to match the code.
> 
> Also, can you separate the state == STATE_PAGE_DONE case from the others.  You
> should note why it's special.

Yes, that's the way to implement it. Add an existing visible notification will
just update the notification.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:198: case
CupsPrintJob::State::STATE_SUSPENDED:
On 2016/10/27 01:43:00, skau wrote:
> Does SUSPENDED belong here?  Do we have no messaging surrounding a paused
state?

Right. We don't have special message for the pause state.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:247: for (auto&
iter : *button_commands_.get()) {
On 2016/10/27 01:43:01, skau wrote:
> iter isn't an iterator, it's a ButtonCommand.

Done.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification.cc:247: for (auto&
iter : *button_commands_.get()) {
On 2016/10/27 01:43:01, skau wrote:
> You shouldn't need get() here

Done.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:26: if
(notification_map_.find(job) != notification_map_.end())
On 2016/10/27 01:43:01, skau wrote:
> you can use base::ContainsKey() for this.  It's a bit nicer.

Done.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:34:
notification_map_[job]->OnPrintJobStatusUpdated();
On 2016/10/27 01:43:01, skau wrote:
> If we end up extracting all the information from the job anyway, should we
just
> have a OnPrintJobStatusChanged() method instead of all the different changes? 
> Also, should we pass the job object to the notification here?

I think it's nicer to have different changes other than a single status change
here. We might have different observers in the future so having different
changes also might benefit future cases. I don't see the reason why we should
pass the job object to the notification, could you elaborate?

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:24:
std::unordered_map<CupsPrintJob*,
On 2016/10/27 01:43:01, skau wrote:
> Can you use the CupsPrintJob unique id instead of a pointer as the index?

Why? If we use pointer, we don't need to always look up print jobs table to find
a specific print job for a given id.

https://codereview.chromium.org/2435303003/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2435303003/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:16: if
(print_job_manager_)
On 2016/10/27 18:52:48, stevenjb wrote:
> Given that this class would be pretty pointless without |print_job_manager|,
> maybe just DCHECK(print_job_manager) (and remove the test in the destructor
> too).

Done.

[skau, 2016-10-31 15:39:05]

lgtm

I've left answers to the questions.  No changes necessary.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager.h:55: virtual bool
CancelPrintJob(CupsPrintJob* job) = 0;
On 2016/10/28 23:59:53, xdai1 wrote:
> On 2016/10/27 01:43:00, skau wrote:
> > Should we change this to CupsPrintJob#UniqueId?
> 
> Why? Actually since |job| is owned by CupsPrintJobManager, it should be
deleted
> in the function 
It's not obvious that this method will delete the object that's passed into it. 
You should add the note to the observer that the corresponding print job will be
deleted after cancelled is called so nobody tries to de-reference it.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:34:
notification_map_[job]->OnPrintJobStatusUpdated();
On 2016/10/28 23:59:54, xdai1 wrote:
> On 2016/10/27 01:43:01, skau wrote:
> > If we end up extracting all the information from the job anyway, should we
> just
> > have a OnPrintJobStatusChanged() method instead of all the different
changes? 
> > Also, should we pass the job object to the notification here?
> 
> I think it's nicer to have different changes other than a single status change
> here. We might have different observers in the future so having different
> changes also might benefit future cases. I don't see the reason why we should
> pass the job object to the notification, could you elaborate? 

Okay.  I was wondering if OnPrintJobStatusUpdated() should be passed the pointer
to the job or at least the status.  Otherwise, when reading this code, we're
forced to remember or assume that the notification has a pointer to the correct
print job object.  Semantically, it's the same.

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
(right):

https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:24:
std::unordered_map<CupsPrintJob*,
On 2016/10/28 23:59:54, xdai1 wrote:
> On 2016/10/27 01:43:01, skau wrote:
> > Can you use the CupsPrintJob unique id instead of a pointer as the index?
> 
> Why? If we use pointer, we don't need to always look up print jobs table to
find
> a specific print job for a given id. 

My main concern is that it requires any object that's going to interact with the
manager to hold a raw pointer to a CupsPrintJob that it doesn't own and we know
will be deleted.  If it doesn't cause problems in practice it's fine.  It just
makes me a bit nervous.

[xdai1, 2016-10-31 17:26:28]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 17:26:46]

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

[xdai1, 2016-10-31 17:33:05]

skau@, thanks for the review!

On 2016/10/31 15:39:05, skau wrote:
> lgtm
> 
> I've left answers to the questions.  No changes necessary.
> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/printing/cups_print_job_manager.h (right):
> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/printing/cups_print_job_manager.h:55: virtual bool
> CancelPrintJob(CupsPrintJob* job) = 0;
> On 2016/10/28 23:59:53, xdai1 wrote:
> > On 2016/10/27 01:43:00, skau wrote:
> > > Should we change this to CupsPrintJob#UniqueId?
> > 
> > Why? Actually since |job| is owned by CupsPrintJobManager, it should be
> deleted
> > in the function 
> It's not obvious that this method will delete the object that's passed into
it. 
> You should add the note to the observer that the corresponding print job will
be
> deleted after cancelled is called so nobody tries to de-reference it.

I added the note as you suggested.

> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc
> (right):
> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/printing/cups_print_job_notification_manager.cc:34:
> notification_map_[job]->OnPrintJobStatusUpdated();
> On 2016/10/28 23:59:54, xdai1 wrote:
> > On 2016/10/27 01:43:01, skau wrote:
> > > If we end up extracting all the information from the job anyway, should we
> > just
> > > have a OnPrintJobStatusChanged() method instead of all the different
> changes? 
> > > Also, should we pass the job object to the notification here?
> > 
> > I think it's nicer to have different changes other than a single status
change
> > here. We might have different observers in the future so having different
> > changes also might benefit future cases. I don't see the reason why we
should
> > pass the job object to the notification, could you elaborate? 
> 
> Okay.  I was wondering if OnPrintJobStatusUpdated() should be passed the
pointer
> to the job or at least the status.  Otherwise, when reading this code, we're
> forced to remember or assume that the notification has a pointer to the
correct
> print job object.  Semantically, it's the same.
> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> File chrome/browser/chromeos/printing/cups_print_job_notification_manager.h
> (right):
> 
>
https://codereview.chromium.org/2435303003/diff/100001/chrome/browser/chromeo...
> chrome/browser/chromeos/printing/cups_print_job_notification_manager.h:24:
> std::unordered_map<CupsPrintJob*,
> On 2016/10/28 23:59:54, xdai1 wrote:
> > On 2016/10/27 01:43:01, skau wrote:
> > > Can you use the CupsPrintJob unique id instead of a pointer as the index?
> > 
> > Why? If we use pointer, we don't need to always look up print jobs table to
> find
> > a specific print job for a given id. 
> 
> My main concern is that it requires any object that's going to interact with
the
> manager to hold a raw pointer to a CupsPrintJob that it doesn't own and we
know
> will be deleted.  If it doesn't cause problems in practice it's fine.  It just
> makes me a bit nervous.

I think for now we should be fine. CupsPrintJobNotificationManager doesn't
observe the print job cancellation events, so theoretically it won't interact
with a invalid point.  
It it does cause problems in the real use case in the future, we can always come
back to it and find a better solution.

[commit-bot: I haz the power, 2016-10-31 17:48:29]

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

[commit-bot: I haz the power, 2016-10-31 17:48:30]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[xdai1, 2016-10-31 18:16:21]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 18:16:41]

Dry run: 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, 2016-10-31 18:52:00]

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

[commit-bot: I haz the power, 2016-10-31 18:52:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[xdai1, 2016-10-31 20:29:26]

The CQ bit was checked by xdai@chromium.org

[xdai1, 2016-10-31 20:29:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, skau@chromium.org
Link to the patchset: https://codereview.chromium.org/2435303003/#ps180001
(title: "Fix the compile warning.")

[commit-bot: I haz the power, 2016-10-31 20:29:54]

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, 2016-10-31 20:48:21]

Description was changed from

==========
[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets
updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing
-> Printing in progress -> Printing Done / Failed. 
- If the print job is cancelled, the notification is removed from the message
center.
- If the notification is closed by the user when showing the printing progress,
prevent showing the following printing progress.

BUG=625376, 626752
==========

to

==========
[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets
updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing
-> Printing in progress -> Printing Done / Failed. 
- If the print job is cancelled, the notification is removed from the message
center.
- If the notification is closed by the user when showing the printing progress,
prevent showing the following printing progress.

BUG=625376, 626752
==========

[commit-bot: I haz the power, 2016-10-31 20:48:23]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-10-31 20:50:04]

Description was changed from

==========
[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets
updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing
-> Printing in progress -> Printing Done / Failed. 
- If the print job is cancelled, the notification is removed from the message
center.
- If the notification is closed by the user when showing the printing progress,
prevent showing the following printing progress.

BUG=625376, 626752
==========

to

==========
[CUPS] Implement the Print Job notification.

- One print job is associated with one notification id. The notification gets
updated when the print job status changes.
- There are four kinds of notifications for the print job: Waiting for printing
-> Printing in progress -> Printing Done / Failed.
- If the print job is cancelled, the notification is removed from the message
center.
- If the notification is closed by the user when showing the printing progress,
prevent showing the following printing progress.

BUG=625376, 626752

Committed: https://crrev.com/4639af5eac058bee60267cc3a054b33818b5488c
Cr-Commit-Position: refs/heads/master@{#428798}
==========

[commit-bot: I haz the power, 2016-10-31 20:50:05]

Patchset 8 (id:??) landed as
https://crrev.com/4639af5eac058bee60267cc3a054b33818b5488c
Cr-Commit-Position: refs/heads/master@{#428798}