Convert to CupsPrintJobManagerImpl to TaskScheduler.

chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc

Index: chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc
diff --git a/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc b/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc
index d03c07c5d950f5c7565c337f9aa3bab9a8d1487b..085e93bcf741d3d030cf773d9c5fcaa12f09762b 100644
--- a/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc
+++ b/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc
@@ -14,10 +14,12 @@
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/threading/sequenced_task_runner_handle.h"
-#include "base/threading/sequenced_worker_pool.h"
+#include "base/task_runner_util.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/thread_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"
@@ -102,13 +104,6 @@ chromeos::CupsPrintJob::State ConvertState(printing::CupsJob::JobState state) {
   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) {
@@ -172,6 +167,43 @@ bool UpdateCurrentPage(const printing::CupsJob& job,
   return pages_updated;
 }
 
+// Updates the state of a print job based on |printer_status| and |job|.
+// Returns true if observers need to be notified of an update.
+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:
+      if (ContainsReason(printer_status, PrinterReason::CONNECTING_TO_DEVICE)) {
+        if (EnforceTimeout(job, print_job)) {
+          LOG(WARNING) << "Connecting to printer timed out";
+          print_job->set_expired(true);
+        }
+      } else {
+        pages_updated = UpdateCurrentPage(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 {
@@ -182,9 +214,49 @@ QueryResult::QueryResult(const QueryResult& other) = default;
 
 QueryResult::~QueryResult() = default;
 
+CupsWrapper::CupsWrapper()
+    : cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);

[Carlson, 2017/06/26 18:42:01]

I don't understand what this does (which is probably just be being unfamiliar
with this mechanism).  Are constructors for sequence checkers normally
automatically required to be sequenced?  I don't understand why you need this
here (as opposed to just *not* having a DCHECK_CALLED_ON_VALID_SEQUENCE() call).

[skau, 2017/06/26 23:15:16]

This object is actually being created on a different sequence than the one it's
being used on.  This macro indicates that.  Otherwise, it associates with the
sequence it was created on.

+}
+
+CupsWrapper::~CupsWrapper() = default;
+
+void CupsWrapper::QueryCups(const std::vector<std::string>& printer_ids,
+                            QueryResult* result) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  result->success = cups_connection_.GetJobs(printer_ids, &result->queues);
+}
+
+void CupsWrapper::CancelJobImpl(const std::string& printer_id,
+                                const int job_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  std::unique_ptr<::printing::CupsPrinter> printer =
+      cups_connection_.GetPrinter(printer_id);
+  if (!printer) {
+    LOG(WARNING) << "Printer not found: " << printer_id;
+    return;
+  }
+
+  if (!printer->CancelJob(job_id)) {
+    // This is not expected to fail but log it if it does.
+    LOG(WARNING) << "Cancelling job failed.  Job may be stuck in queue.";
+  }
+}
+
 CupsPrintJobManagerImpl::CupsPrintJobManagerImpl(Profile* profile)
     : CupsPrintJobManager(profile),
-      cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false),
+      cups_wrapper_(),
+      cups_wrapper_ptr_factory_(&cups_wrapper_),
+      query_runner_(base::CreateSequencedTaskRunnerWithTraits(
+          base::TaskTraits(base::TaskPriority::BACKGROUND,
+                           base::MayBlock(),
+                           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN))),
+      browser_thread_runner_(content::BrowserThread::GetTaskRunnerForThread(
+          content::BrowserThread::UI)),
       weak_ptr_factory_(this) {
   registrar_.Add(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
                  content::NotificationService::AllSources());
@@ -194,7 +266,7 @@ CupsPrintJobManagerImpl::~CupsPrintJobManagerImpl() {}
 
 // Must be run from the UI thread.
 void CupsPrintJobManagerImpl::CancelPrintJob(CupsPrintJob* job) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
 
   // Copy job_id and printer_id.  |job| is about to be freed.
   const int job_id = job->job_id();
@@ -205,10 +277,10 @@ void CupsPrintJobManagerImpl::CancelPrintJob(CupsPrintJob* job) {
 
   // Be sure to copy out all relevant fields.  |job| may be destroyed after we

[Carlson, 2017/06/26 18:42:01]

Is this comment still relevant?

[skau, 2017/06/26 23:15:16]

It's still true but it duplicates the comment on #271.  Deleted.

   // exit this scope.
-  content::BrowserThread::GetBlockingPool()->PostTask(
+  query_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&CupsPrintJobManagerImpl::CancelJobImpl,
-                 weak_ptr_factory_.GetWeakPtr(), printer_id, job_id));
+      base::Bind(&CupsWrapper::CancelJobImpl,
+                 cups_wrapper_ptr_factory_.GetWeakPtr(), printer_id, job_id));
 }
 
 bool CupsPrintJobManagerImpl::SuspendPrintJob(CupsPrintJob* job) {
@@ -244,6 +316,8 @@ bool CupsPrintJobManagerImpl::CreatePrintJob(const std::string& printer_name,
                                              const std::string& title,
                                              int job_id,
                                              int total_page_number) {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
   auto printer =
       PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinter(
           printer_name);
@@ -281,10 +355,12 @@ void CupsPrintJobManagerImpl::ScheduleQuery() {
 }
 
 void CupsPrintJobManagerImpl::ScheduleQuery(const base::TimeDelta& delay) {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
   if (!in_query_) {
     in_query_ = true;
 
-    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+    browser_thread_runner_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&CupsPrintJobManagerImpl::PostQuery,
                    weak_ptr_factory_.GetWeakPtr()),
@@ -293,6 +369,8 @@ void CupsPrintJobManagerImpl::ScheduleQuery(const base::TimeDelta& delay) {
 }
 
 void CupsPrintJobManagerImpl::PostQuery() {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
   // The set of active printers is expected to be small.
   std::set<std::string> printer_ids;
   for (const auto& entry : jobs_) {
@@ -300,60 +378,30 @@ void CupsPrintJobManagerImpl::PostQuery() {
   }
   std::vector<std::string> ids{printer_ids.begin(), printer_ids.end()};
 
-  content::BrowserThread::PostTaskAndReplyWithResult(
-      content::BrowserThread::FILE_USER_BLOCKING, FROM_HERE,
-      base::Bind(&QueryCups, &cups_connection_, ids),
+  auto result = base::MakeUnique<QueryResult>();
+  QueryResult* result_ptr = result.get();
+  // Runs a query on query_runner_ which will rejoin browser_thread_runner_.
+  query_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&CupsWrapper::QueryCups,
+                 cups_wrapper_ptr_factory_.GetWeakPtr(), ids, result_ptr),
       base::Bind(&CupsPrintJobManagerImpl::UpdateJobs,
-                 weak_ptr_factory_.GetWeakPtr()));
-}
-
-bool CupsPrintJobManagerImpl::UpdatePrintJob(
-    const ::printing::PrinterStatus& printer_status,
-    const ::printing::CupsJob& job,
-    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:
-      if (ContainsReason(printer_status, PrinterReason::CONNECTING_TO_DEVICE)) {
-        if (EnforceTimeout(job, print_job)) {
-          LOG(WARNING) << "Connecting to printer timed out";
-          print_job->set_expired(true);
-        }
-      } else {
-        pages_updated = UpdateCurrentPage(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;
+                 weak_ptr_factory_.GetWeakPtr(), base::Passed(&result)));
 }
 
 // Use job information to update local job states.  Previously completed jobs
-// could be in |jobs| but those are ignored as we will not emit updates for them
-// after they are completed.
-void CupsPrintJobManagerImpl::UpdateJobs(const QueryResult& result) {
-  const std::vector<::printing::QueueStatus>& queues = result.queues;
+// could be in |jobs| but those are ignored as we will not emit updates for
+// them after they are completed.
+void CupsPrintJobManagerImpl::UpdateJobs(std::unique_ptr<QueryResult> result) {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
+  const std::vector<::printing::QueueStatus>& queues = result->queues;
 
   // Query has completed.  Allow more queries.
   in_query_ = false;
 
   // If the query failed, either retry or purge.
-  if (!result.success) {
+  if (!result->success) {
     retry_count_++;
     LOG(WARNING) << "Failed to query CUPS for queue status.  Schedule retry ("
                  << retry_count_ << ")";
@@ -407,14 +455,16 @@ void CupsPrintJobManagerImpl::UpdateJobs(const QueryResult& result) {
     // 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.
+    // 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() {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
   for (const auto& entry : jobs_) {
     // Declare all lost jobs errors.
     RecordJobResult(LOST);
@@ -426,22 +476,9 @@ void CupsPrintJobManagerImpl::PurgeJobs() {
   jobs_.clear();
 }
 
-void CupsPrintJobManagerImpl::CancelJobImpl(const std::string& printer_id,
-                                            const int job_id) {
-  std::unique_ptr<::printing::CupsPrinter> printer =
-      cups_connection_.GetPrinter(printer_id);
-  if (!printer) {
-    LOG(WARNING) << "Printer not found: " << printer_id;
-    return;
-  }
-
-  if (!printer->CancelJob(job_id)) {
-    // This is not expected to fail but log it if it does.
-    LOG(WARNING) << "Cancelling job failed.  Job may be stuck in queue.";
-  }
-}
-
 void CupsPrintJobManagerImpl::NotifyJobStateUpdate(CupsPrintJob* job) {
+  DCHECK(browser_thread_runner_->RunsTasksInCurrentSequence());
+
   switch (job->state()) {
     case State::STATE_NONE:
       // State does not require notification.