CUPS Printing: Encapsulate background tasks logic inside CupsWrapper

* Move CupsWrapper to separate class
* Move heavy tasks to inner CupsWrapper::Backend class

Bug: 996785
Change-Id: I481e0df807017ee7e1221473baacf0d2f1a52d8e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904170
Commit-Queue: Nikita Podguzov <nikitapodguzov@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714459}

chrome/browser/chromeos/BUILD.gn

@@ -2327,6 +2327,8 @@ source_set("chromeos") {
       "printing/cups_proxy_service_manager.h",
       "printing/cups_proxy_service_manager_factory.cc",
       "printing/cups_proxy_service_manager_factory.h",
+      "printing/cups_wrapper.cc",
+      "printing/cups_wrapper.h",
       "printing/printer_info_cups.cc",
     ]
   } else {

chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc

@@ -14,17 +14,16 @@
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
-#include "base/sequenced_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
 #include "base/task_runner_util.h"
-#include "base/threading/scoped_blocking_call.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
 #include "chrome/browser/chromeos/printing/cups_printers_manager.h"
 #include "chrome/browser/chromeos/printing/cups_printers_manager_factory.h"
+#include "chrome/browser/chromeos/printing/cups_wrapper.h"
 #include "chrome/browser/chromeos/printing/history/print_job_info.pb.h"
 #include "chrome/browser/chromeos/printing/history/print_job_info_proto_conversions.h"
 #include "chrome/browser/printing/print_job.h"
@@ -36,7 +35,6 @@
 #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"
 #include "printing/printing_utils.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -68,16 +66,6 @@ enum JobResultForHistogram {
   RESULT_MAX
 };
 
-// Container for results from CUPS queries.
-struct QueryResult {
-  QueryResult() = default;
-  QueryResult(const QueryResult& other) = default;
-  ~QueryResult() = default;
-
-  bool success;
-  std::vector<::printing::QueueStatus> queues;
-};
-
 // Returns the appropriate JobResultForHistogram for a given |state|.  Only
 // FINISHED and PRINTER_CANCEL are derived from CupsPrintJob::State.
 JobResultForHistogram ResultForHistogram(State state) {
@@ -220,63 +208,11 @@ bool UpdatePrintJob(const ::printing::PrinterStatus& printer_status,
 
 namespace chromeos {
 
-// A wrapper around the CUPS connection to ensure that it's always accessed on
-// the same sequence.
-class CupsWrapper {
- public:
-  CupsWrapper() : cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false) {
-    DETACH_FROM_SEQUENCE(sequence_checker_);
-  }
-
-  ~CupsWrapper() = default;
-
-  // Query CUPS for the current jobs for the given |printer_ids|.  Writes result
-  // to |result|.
-  void QueryCups(const std::vector<std::string>& printer_ids,
-                 QueryResult* result) {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    base::ScopedBlockingCall scoped_blocking_call(
-        FROM_HERE, base::BlockingType::MAY_BLOCK);
-
-    result->success = cups_connection_.GetJobs(printer_ids, &result->queues);
-  }
-
-  // Cancel the print job on the blocking thread.
-  void CancelJobImpl(const std::string& printer_id, const int job_id) {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    base::ScopedBlockingCall scoped_blocking_call(
-        FROM_HERE, base::BlockingType::MAY_BLOCK);
-
-    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.";
-    }
-  }
-
- private:
-  ::printing::CupsConnection cups_connection_;
-  SEQUENCE_CHECKER(sequence_checker_);
-
-  DISALLOW_COPY_AND_ASSIGN(CupsWrapper);
-};
-
 class CupsPrintJobManagerImpl : public CupsPrintJobManager,
                                 public content::NotificationObserver {
  public:
   explicit CupsPrintJobManagerImpl(Profile* profile)
       : CupsPrintJobManager(profile),
-        query_runner_(base::CreateSequencedTaskRunner(base::TaskTraits{
-            base::ThreadPool(), base::TaskPriority::BEST_EFFORT,
-            base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
-        cups_wrapper_(new CupsWrapper(),
-                      base::OnTaskRunnerDeleter(query_runner_)),
         weak_ptr_factory_(this) {
     timer_.SetTaskRunner(
         base::CreateSingleThreadTaskRunner({content::BrowserThread::UI}));
@@ -405,10 +341,7 @@ class CupsPrintJobManagerImpl : public CupsPrintJobManager,
     // Stop montioring jobs after we cancel them.  The user no longer cares.
     jobs_.erase(job->GetUniqueId());
 
-    query_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&CupsWrapper::CancelJobImpl,
-                                  base::Unretained(cups_wrapper_.get()),
-                                  printer_id, job_id));
+    cups_wrapper_.CancelJob(printer_id, job_id);
   }
 
   // Schedule a query of CUPS for print job status with a delay of |delay|.
@@ -418,8 +351,6 @@ class CupsPrintJobManagerImpl : public CupsPrintJobManager,
                                      weak_ptr_factory_.GetWeakPtr()));
   }
 
-  // Schedule the CUPS query off the UI thread. Posts results back to UI thread
-  // to UpdateJobs.
   void PostQuery() {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -430,23 +361,16 @@ class CupsPrintJobManagerImpl : public CupsPrintJobManager,
     }
     std::vector<std::string> ids{printer_ids.begin(), printer_ids.end()};
 
-    auto result = std::make_unique<QueryResult>();
-    QueryResult* result_ptr = result.get();
-    // Runs a query on |query_runner_| which will rejoin this sequnece on
-    // completion.
-    query_runner_->PostTaskAndReply(
-        FROM_HERE,
-        base::BindOnce(&CupsWrapper::QueryCups,
-                       base::Unretained(cups_wrapper_.get()), ids, result_ptr),
-        base::BindOnce(&CupsPrintJobManagerImpl::UpdateJobs,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(result)));
+    cups_wrapper_.QueryCups(ids,
+                            base::BindOnce(&CupsPrintJobManagerImpl::UpdateJobs,
+                                           weak_ptr_factory_.GetWeakPtr()));
   }
 
   // Process jobs from CUPS and perform notifications.
   // 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 UpdateJobs(std::unique_ptr<QueryResult> result) {
+  void UpdateJobs(std::unique_ptr<CupsWrapper::QueryResult> result) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
     // If the query failed, either retry or purge.
@@ -580,9 +504,7 @@ class CupsPrintJobManagerImpl : public CupsPrintJobManager,
 
   base::RepeatingTimer timer_;
   content::NotificationRegistrar registrar_;
-  // Task runner for queries to CUPS.
-  scoped_refptr<base::SequencedTaskRunner> query_runner_;
-  std::unique_ptr<CupsWrapper, base::OnTaskRunnerDeleter> cups_wrapper_;
+  CupsWrapper cups_wrapper_;
   base::WeakPtrFactory<CupsPrintJobManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CupsPrintJobManagerImpl);

chrome/browser/chromeos/printing/cups_wrapper.cc

+// Copyright 2019 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_wrapper.h"
+
+#include <cups/cups.h>
+
+#include "base/callback.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
+#include "base/task_runner_util.h"
+#include "base/threading/scoped_blocking_call.h"
+#include "printing/backend/cups_printer.h"
+#include "url/gurl.h"
+
+namespace chromeos {
+
+CupsWrapper::QueryResult::QueryResult() = default;
+
+CupsWrapper::QueryResult::~QueryResult() = default;
+
+class CupsWrapper::Backend {
+ public:
+  Backend();
+  Backend(const Backend&) = delete;
+  Backend& operator=(const Backend&) = delete;
+  ~Backend();
+
+  std::unique_ptr<QueryResult> QueryCups(
+      const std::vector<std::string>& printer_ids);
+
+  void CancelJob(const std::string& printer_id, int job_id);
+
+ private:
+  ::printing::CupsConnection cups_connection_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+};
+
+CupsWrapper::Backend::Backend()
+    : cups_connection_(GURL(), HTTP_ENCRYPT_NEVER, false) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
+
+CupsWrapper::Backend::~Backend() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
+std::unique_ptr<CupsWrapper::QueryResult> CupsWrapper::Backend::QueryCups(
+    const std::vector<std::string>& printer_ids) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  auto result = std::make_unique<CupsWrapper::QueryResult>();
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+  result->success = cups_connection_.GetJobs(printer_ids, &result->queues);
+  return result;
+}
+
+void CupsWrapper::Backend::CancelJob(const std::string& printer_id,
+                                     int job_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+
+  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.";
+  }
+}
+
+CupsWrapper::CupsWrapper()
+    : backend_(std::make_unique<Backend>()),
+      backend_task_runner_(base::CreateSequencedTaskRunner(
+          {base::ThreadPool(), base::MayBlock(),
+           base::TaskPriority::BEST_EFFORT,
+           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {}
+
+CupsWrapper::~CupsWrapper() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  backend_task_runner_->DeleteSoon(FROM_HERE, backend_.release());
+}
+
+void CupsWrapper::QueryCups(
+    const std::vector<std::string>& printer_ids,
+    base::OnceCallback<void(std::unique_ptr<CupsWrapper::QueryResult>)>
+        callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // It's safe to pass unretained pointer here because we delete |backend_| on
+  // the same task runner.
+  base::PostTaskAndReplyWithResult(
+      backend_task_runner_.get(), FROM_HERE,
+      base::BindOnce(&Backend::QueryCups, base::Unretained(backend_.get()),
+                     printer_ids),
+      std::move(callback));
+}
+
+void CupsWrapper::CancelJob(const std::string& printer_id, int job_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // It's safe to pass unretained pointer here because we delete |backend_| on
+  // the same task runner.
+  backend_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&Backend::CancelJob, base::Unretained(backend_.get()),
+                     printer_id, job_id));
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/printing/cups_wrapper.h

+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_WRAPPER_H_
+#define CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_WRAPPER_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
+#include "printing/backend/cups_connection.h"
+
+namespace chromeos {
+
+// A wrapper around the CUPS connection to ensure that it's always accessed on
+// the same sequence and run in the appropriate sequence off of the calling
+// sequence.
+class CupsWrapper {
+ public:
+  // Container for results from CUPS queries.
+  struct QueryResult {
+    QueryResult();
+    QueryResult(const QueryResult& other) = delete;
+    QueryResult& operator=(const QueryResult& other) = delete;
+    ~QueryResult();
+
+    bool success;
+    std::vector<::printing::QueueStatus> queues;
+  };
+
+  CupsWrapper();
+  CupsWrapper(const CupsWrapper&) = delete;
+  CupsWrapper& operator=(const CupsWrapper&) = delete;
+  ~CupsWrapper();
+
+  // Queries CUPS for the current jobs for the given |printer_ids|. Passes
+  // the result to |callback|.
+  void QueryCups(
+      const std::vector<std::string>& printer_ids,
+      base::OnceCallback<void(std::unique_ptr<QueryResult>)> callback);
+
+  // Cancels the print job on the blocking thread.
+  void CancelJob(const std::string& printer_id, int job_id);
+
+ private:
+  class Backend;
+  // The |backend_| handles all communication with CUPS.
+  // It is instantiated on the thread |this| runs on but after that,
+  // must only be accessed and eventually destroyed via the
+  // |backend_task_runner_|.
+  std::unique_ptr<Backend> backend_;
+
+  scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_WRAPPER_H_