Refine notifications for print jobs in progress.

chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/printing/cups_print_job_manager_impl.h"
 
 #include <cups/cups.h>
+#include <algorithm>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
 #include "chrome/browser/chromeos/printing/printers_manager.h"
 #include "chrome/browser/chromeos/printing/printers_manager_factory.h"
 #include "chrome/browser/printing/print_job.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "printing/backend/cups_connection.h"
 #include "printing/printed_document.h"
 
 namespace {
 
 // The rate in milliseconds at which we will poll CUPS for print job updates.
 const int kPollRate = 1000;
 
+// How long we'll wait to connect to a printer before declaring an error.
+const int kConnectingTimeout = 20;
+
 // Threshold for giving up on communicating with CUPS.
 const int kRetryMax = 6;
 
+using State = chromeos::CupsPrintJob::State;
+using ErrorCode = chromeos::CupsPrintJob::ErrorCode;
+
+using PrinterReason = printing::PrinterStatus::PrinterReason;
+
 // Returns the equivalient CupsPrintJob#State from a CupsJob#JobState.
 chromeos::CupsPrintJob::State ConvertState(printing::CupsJob::JobState state) {
-  using cpj = chromeos::CupsPrintJob::State;
-
   switch (state) {
     case printing::CupsJob::PENDING:
-      return cpj::STATE_WAITING;
+      return State::STATE_WAITING;
     case printing::CupsJob::HELD:
-      return cpj::STATE_SUSPENDED;
+      return State::STATE_SUSPENDED;
     case printing::CupsJob::PROCESSING:
-      return cpj::STATE_STARTED;
+      return State::STATE_STARTED;
     case printing::CupsJob::CANCELED:
-      return cpj::STATE_CANCELLED;
+      return State::STATE_CANCELLED;
     case printing::CupsJob::COMPLETED:
-      return cpj::STATE_DOCUMENT_DONE;
+      return State::STATE_DOCUMENT_DONE;
     case printing::CupsJob::STOPPED:
-      return cpj::STATE_SUSPENDED;
+      return State::STATE_SUSPENDED;
     case printing::CupsJob::ABORTED:
-      return cpj::STATE_ERROR;
+      return State::STATE_ERROR;
     case printing::CupsJob::UNKNOWN:
       break;
   }
 
   NOTREACHED();
 
-  return cpj::STATE_NONE;
+  return State::STATE_NONE;
 }
 
 chromeos::QueryResult QueryCups(::printing::CupsConnection* connection,
                                 const std::vector<std::string>& printer_ids) {
   chromeos::QueryResult result;
   result.success = connection->GetJobs(printer_ids, &result.queues);
   return result;
 }
 
+// Returns true if |printer_status|.reasons contains |reason|.
+bool ContainsReason(const printing::PrinterStatus printer_status,
+                    PrinterReason::Reason reason) {
+  return std::find_if(
+             printer_status.reasons.begin(), printer_status.reasons.end(),
+             [reason](const PrinterReason& r) { return r.reason == reason; }) !=

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

I think this can be [&reason], but I don't know if that's a big enough structure
to care.

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

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

+         printer_status.reasons.end();
+}
+
+// Returns true if the reason is paper related.
+bool IsPaperReason(const PrinterReason& reason) {
+  static std::set<PrinterReason::Reason> paper_reasons = {
+      PrinterReason::MEDIA_JAM, PrinterReason::MEDIA_EMPTY,
+      PrinterReason::MEDIA_NEEDED, PrinterReason::MEDIA_LOW};
+
+  return paper_reasons.find(reason.reason) != paper_reasons.end();
+}
+
+// Returns true if the reason is ink related.
+bool IsInkReason(const PrinterReason& reason) {
+  return reason.reason == PrinterReason::TONER_EMPTY ||
+         reason.reason == PrinterReason::TONER_LOW;
+}
+
+// Extracts an ErrorCode from PrinterStatus#reasons.  Returns NO_ERROR if there
+// are no reasons which indicate an error.
+chromeos::CupsPrintJob::ErrorCode ErrorCodeFromReasons(
+    const printing::PrinterStatus& printer_status) {
+  const auto begin = printer_status.reasons.begin();
+  const auto end = printer_status.reasons.end();
+
+  if (std::find_if(begin, end, &IsPaperReason) != end) {

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

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.

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

Switched to a switch.

+    return chromeos::CupsPrintJob::ErrorCode::PAPER_JAM;
+  } else if (std::find_if(begin, end, &IsInkReason) != end) {
+    return chromeos::CupsPrintJob::ErrorCode::OUT_OF_INK;
+  }
+
+  return chromeos::CupsPrintJob::ErrorCode::NO_ERROR;
+}
+
+// Update job in the processing state.  Returns true if the number of pages
+// changed.
+bool UpdateForProcessing(const printing::PrinterStatus& printer_status,
+                         const printing::CupsJob& job,
+                         chromeos::CupsPrintJob* print_job) {
+  if (ContainsReason(printer_status, PrinterReason::CONNECTING_TO_DEVICE)) {
+    // Check to see if we should time out.
+    base::TimeDelta time_waiting =
+        base::Time::Now() - base::Time::FromTimeT(job.processing_started);
+    if (time_waiting > base::TimeDelta::FromSeconds(kConnectingTimeout)) {
+      print_job->set_state(chromeos::CupsPrintJob::State::STATE_ERROR);
+      print_job->set_error_code(
+          chromeos::CupsPrintJob::ErrorCode::PRINTER_UNREACHABLE);
+      LOG(WARNING) << "Connecting to printer timed out";
+    }
+
+    // Always return false from the CONNECTING_TO_DEVICE state since pages
+    // are never updated if we can't talk to the printer.
+    return false;
+  }
+
+  bool pages_updated = false;
+  if (job.current_pages <= 0) {
+    print_job->set_printed_page_number(0);
+    print_job->set_state(State::STATE_STARTED);
+  } else {
+    pages_updated = job.current_pages != print_job->printed_page_number();
+    print_job->set_printed_page_number(job.current_pages);
+    print_job->set_state(State::STATE_PAGE_DONE);
+  }
+
+  return pages_updated;
+}
+
+// Returns true if the state of |print_job| changed.  Updates |print_job| with

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

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]

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.  

+// the data in |printer_status| and |job|.
+bool UpdatePrintJob(const printing::PrinterStatus& printer_status,
+                    const printing::CupsJob& job,
+                    chromeos::CupsPrintJob* print_job) {
+  DCHECK_EQ(job.id, print_job->job_id());
+
+  State old_state = print_job->state();
+
+  bool pages_updated = false;
+  switch (job.state) {
+    case printing::CupsJob::PROCESSING:
+      pages_updated = UpdateForProcessing(printer_status, job, print_job);
+      break;
+    case printing::CupsJob::COMPLETED:
+      DCHECK_GE(job.current_pages, print_job->total_page_number());
+      print_job->set_state(State::STATE_DOCUMENT_DONE);
+      break;
+    case printing::CupsJob::ABORTED:
+    case printing::CupsJob::CANCELED:
+      print_job->set_error_code(ErrorCodeFromReasons(printer_status));
+    // fall through
+    default:
+      print_job->set_state(ConvertState(job.state));
+      break;
+  }
+
+  return print_job->state() != old_state || pages_updated;
+}
+
 }  // namespace
 
 namespace chromeos {
 
 CupsPrintJobManagerImpl::CupsPrintJobManagerImpl(Profile* profile)
     : CupsPrintJobManager(profile),
       cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false),
       weak_ptr_factory_(this) {
   registrar_.Add(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
                  content::NotificationService::AllSources());
@@ skipping 136 matching lines @@
   std::vector<std::string> active_jobs;
   for (const auto& queue : queues) {
     for (auto& job : queue.jobs) {
       std::string key = CupsPrintJob::GetUniqueId(job.printer_id, job.id);
       const auto& entry = jobs_.find(key);
       if (entry == jobs_.end())
         continue;
 
       CupsPrintJob* print_job = entry->second.get();
 
-      // Update a job we're tracking.
-      JobStateUpdated(print_job, ConvertState(job.state));
+      if (UpdatePrintJob(queue.printer_status, job, print_job)) {
+        // The state of the job changed, notify observers.
+        NotifyJobStateUpdate(print_job);
+      }
 
       // Cleanup completed jobs.
       if (print_job->IsJobFinished()) {
         jobs_.erase(entry);
       } else {
         active_jobs.push_back(key);
       }
     }
   }
 
   // Keep polling until all jobs complete or error.
   if (!active_jobs.empty()) {
     // During normal operations, we poll at the default rate.
     ScheduleQuery();
   } else if (!jobs_.empty()) {
     // We're tracking jobs that we didn't receive an update for.  Something bad
     // has happened.
     LOG(ERROR) << "Lost track of (" << jobs_.size() << ") jobs";
     PurgeJobs();
   }
 }
 
 void CupsPrintJobManagerImpl::PurgeJobs() {
   for (const auto& entry : jobs_) {
     // Declare all lost jobs errors.
-    JobStateUpdated(entry.second.get(), CupsPrintJob::State::STATE_ERROR);
+    CupsPrintJob* job = entry.second.get();
+    job->set_state(CupsPrintJob::State::STATE_ERROR);
+    NotifyJobStateUpdate(job);
   }
 
   jobs_.clear();
 }
 
-void CupsPrintJobManagerImpl::JobStateUpdated(CupsPrintJob* job,
-                                              CupsPrintJob::State new_state) {
-  if (job->state() == new_state)
-    return;
-
-  // We don't track state transitions because some of them might be missed due
-  // to how we query jobs.
-  job->set_state(new_state);
-  switch (new_state) {
-    case CupsPrintJob::State::STATE_NONE:
+void CupsPrintJobManagerImpl::NotifyJobStateUpdate(CupsPrintJob* job) {
+  switch (job->state()) {
+    case State::STATE_NONE:
       // State does not require notification.
       break;
-    case CupsPrintJob::State::STATE_WAITING:
+    case State::STATE_WAITING:
       NotifyJobUpdated(job);
       break;
-    case CupsPrintJob::State::STATE_STARTED:
+    case State::STATE_STARTED:
       NotifyJobStarted(job);
       break;
-    case CupsPrintJob::State::STATE_RESUMED:
+    case State::STATE_PAGE_DONE:
+      NotifyJobUpdated(job);
+      break;
+    case State::STATE_RESUMED:
       NotifyJobResumed(job);
       break;
-    case CupsPrintJob::State::STATE_SUSPENDED:
+    case State::STATE_SUSPENDED:
       NotifyJobSuspended(job);
       break;
-    case CupsPrintJob::State::STATE_CANCELLED:
+    case State::STATE_CANCELLED:
       NotifyJobCanceled(job);
       break;
-    case CupsPrintJob::State::STATE_ERROR:
+    case State::STATE_ERROR:
       NotifyJobError(job);
       break;
-    case CupsPrintJob::State::STATE_PAGE_DONE:
-      NOTREACHED() << "CUPS does not surface this state so it's not expected";
-      break;
-    case CupsPrintJob::State::STATE_DOCUMENT_DONE:
+    case State::STATE_DOCUMENT_DONE:
       NotifyJobDone(job);
       break;
   }
 }
 
 // static
 CupsPrintJobManager* CupsPrintJobManager::CreateInstance(Profile* profile) {
   return new CupsPrintJobManagerImpl(profile);
 }
 
 }  // namespace chromeos