Refine notifications for print jobs in progress.

Refine notifications for print jobs in progress.

This CL adds page updates and network timeout notficiations for print job
notifications.  There is also some cleanup of the class.

BUG=695669, 684853
Review-Url: https://codereview.chromium.org/2705333007
Cr-Commit-Position: refs/heads/master@{#457188}
Committed: https://chromium.googlesource.com/chromium/src/+/bd6fdcf63999268426e5e80802b5ab261fb2e1a2

[skau, 2017-03-10 01:08:41]

Description was changed from

==========
Refine notifications for print jobs in progress.

This CL adds page updates and network timeout notficiations for print job
notifications.  There is also some cleanup of the class.

BUG=695669,684853
==========

to

==========
Refine notifications for print jobs in progress.

This CL adds page updates and network timeout notficiations for print job
notifications.  There is also some cleanup of the class.

BUG=695669,684853
==========

[skau, 2017-03-10 01:08:41]

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

[skau, 2017-03-10 01:09:22]

Please take a look.  I've made some improvements to how we discover errors and
progress for notifications.

[xdai1, 2017-03-13 18:48:37]

lgtm with nits

https://codereview.chromium.org/2705333007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right):

https://codereview.chromium.org/2705333007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:137: return
false;
return in line 134 instead of here?

[skau, 2017-03-14 02:31:29]

https://codereview.chromium.org/2705333007/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc (right):

https://codereview.chromium.org/2705333007/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:137: return
false;
On 2017/03/13 18:48:37, xdai1 wrote:
> return in line 134 instead of here?

The intention is that we always return false if where in the
CONNECTING_TO_DEVICE state since we know that pages are not being printed if we
can't communicate with the printer.  An appropriate comment has been added.

[Carlson, 2017-03-14 17:10:36]

Couple nits.

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

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:86:
[reason](const PrinterReason& r) { return r.reason == reason; }) !=
I think this can be [&reason], but I don't know if that's a big enough structure
to care.

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:112: if
(std::find_if(begin, end, &IsPaperReason) != end) {
This seems odd to me.  

Do you really want the hierarchical "if both paper jam and out of ink reasons
exist, make sure you return paper jam" semantics?

If not, it seems like it would be simpler to do something like

for (const auto& reason: reasons) {
  switch (reason) {
    case MEDIA_JAM:
    case MEDIA_EMPTY:
    ...
      return PAPER_JAM;
    case TONER_EMPTY:
    ...
      return OUT_OF_INK;
  }
}
return NO_ERROR;


If you *do* need the "JAM overrides OUT_OF_INK errors if they both exist"
semantics, then please make sure it's documented why this is needed.

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:155: // Returns
true if the state of |print_job| changed.  Updates |print_job| with
I think the docstring about the return value is not quite accurate.  It looks
like this returns true if the function itself changes the state *or* the number
of pages has changed.

[skau, 2017-03-14 21:32:07]

Thanks for the review.  I tweaked the structure to, hopefully, make it a bit
clearer.

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

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:86:
[reason](const PrinterReason& r) { return r.reason == reason; }) !=
On 2017/03/14 17:10:36, Carlson wrote:
> I think this can be [&reason], but I don't know if that's a big enough
structure
> to care.

Both parts of your statement are true.  I changed it anyway.

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:112: if
(std::find_if(begin, end, &IsPaperReason) != end) {
On 2017/03/14 17:10:36, Carlson wrote:
> This seems odd to me.  
> 
> Do you really want the hierarchical "if both paper jam and out of ink reasons
> exist, make sure you return paper jam" semantics?
> 
> If not, it seems like it would be simpler to do something like
> 
> for (const auto& reason: reasons) {
>   switch (reason) {
>     case MEDIA_JAM:
>     case MEDIA_EMPTY:
>     ...
>       return PAPER_JAM;
>     case TONER_EMPTY:
>     ...
>       return OUT_OF_INK;
>   }
> }
> return NO_ERROR;
> 
> 
> If you *do* need the "JAM overrides OUT_OF_INK errors if they both exist"
> semantics, then please make sure it's documented why this is needed.

Switched to a switch.

https://codereview.chromium.org/2705333007/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:155: // Returns
true if the state of |print_job| changed.  Updates |print_job| with
On 2017/03/14 17:10:36, Carlson wrote:
> I think the docstring about the return value is not quite accurate.  It looks
> like this returns true if the function itself changes the state *or* the
number
> of pages has changed.

The boolean represents if we should notify observers.  The criteria happens to
be if the pages changed or the state changed.  It's been updated accordingly.

[Carlson, 2017-03-14 21:54:05]

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

https://codereview.chromium.org/2705333007/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:158:
EnforceTimeout(job, print_job);
I suspect you meant to use the return value for EnforceTimeout().

[skau, 2017-03-15 01:29:09]

ptal

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

https://codereview.chromium.org/2705333007/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc:158:
EnforceTimeout(job, print_job);
On 2017/03/14 21:54:05, Carlson wrote:
> I suspect you meant to use the return value for EnforceTimeout().

I did.  But I actually need it later since the change is going to be a little
tricky.

[Carlson, 2017-03-15 19:36:50]

lgtm

[skau, 2017-03-15 20:06:02]

The CQ bit was checked by skau@chromium.org

[skau, 2017-03-15 20:06:03]

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/2705333007/#ps160001
(title: "use enforce boolean")

[commit-bot: I haz the power, 2017-03-15 20:06:49]

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-15 20:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489608362966570,
"parent_rev": "d0deb19af7f210edc9d644cb1240f742f755d5c2", "commit_rev":
"bd6fdcf63999268426e5e80802b5ab261fb2e1a2"}

[commit-bot: I haz the power, 2017-03-15 20:28:43]

Description was changed from

==========
Refine notifications for print jobs in progress.

This CL adds page updates and network timeout notficiations for print job
notifications.  There is also some cleanup of the class.

BUG=695669,684853
==========

to

==========
Refine notifications for print jobs in progress.

This CL adds page updates and network timeout notficiations for print job
notifications.  There is also some cleanup of the class.

BUG=695669,684853

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

[commit-bot: I haz the power, 2017-03-15 20:28:44]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/bd6fdcf63999268426e5e80802b5...