Convert to CupsPrintJobManagerImpl to TaskScheduler.

chrome/browser/chromeos/printing/cups_print_job_manager_impl.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_PRINT_JOB_MANAGER_IMPL_H_
 #define CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_PRINT_JOB_MANAGER_IMPL_H_
 
 #include <map>
 #include <memory>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
+#include "base/single_thread_task_runner.h"
 #include "chrome/browser/chromeos/printing/cups_print_job.h"
 #include "chrome/browser/chromeos/printing/cups_print_job_manager.h"
 #include "chrome/browser/chromeos/printing/printers_manager.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "printing/backend/cups_connection.h"
 
 class Profile;
 
 namespace chromeos {
 
 struct QueryResult {
   QueryResult();
   QueryResult(const QueryResult& other);
   ~QueryResult();
 
   bool success;
   std::vector<::printing::QueueStatus> queues;
 };
 
+class CupsWrapper {

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

Class comment, esp why this class exists in the first place.

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

Done.

+ public:
+  CupsWrapper();
+  ~CupsWrapper();
+
+  // 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);
+
+  // Cancel the print job on the blocking thread.
+  void CancelJobImpl(const std::string& printer_id, const int job_id);
+
+ private:
+  // Access only on query_runner_.

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

It's not obvious here what query_runner_ you're talking about.

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

I think the presence of the sequence checker is sufficient to make the comment
redundant.  It's gone now.

+  ::printing::CupsConnection cups_connection_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+};

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

I think this is still unsafe to me, w.r.t destruction.  You're using weak ptr to
ensure that we don't *start* any callbacks after the Wrapper is destroyed, but
as I understand it, that mechanism can't be used outside of an entirely
sequenced context -- e.g., the weak ptr guarantee requires that the destructor
be sequenced with respect to the callbacks it's cancelling.  

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

That is correct.  I'm digging through the documentation to see if I can figure
out how this is supposed to be done.  AFAICT the task runner would need to be
flushed to make this guarantee but I think the intended approach is something
else.

+
 class CupsPrintJobManagerImpl : public CupsPrintJobManager,
                                 public content::NotificationObserver {
  public:
   explicit CupsPrintJobManagerImpl(Profile* profile);
   ~CupsPrintJobManagerImpl() override;
 
   // CupsPrintJobManager overrides:
   void CancelPrintJob(CupsPrintJob* job) override;
   bool SuspendPrintJob(CupsPrintJob* job) override;
   bool ResumePrintJob(CupsPrintJob* job) override;
@@ skipping 13 matching lines @@
 
   // Schedule a query of CUPS for print job status with the default delay.
   void ScheduleQuery();
   // Schedule a query of CUPS for print job status with a delay of |delay|.
   void ScheduleQuery(const base::TimeDelta& delay);
 
   // Schedule the CUPS query off the UI thread. Posts results back to UI thread
   // to UpdateJobs.
   void PostQuery();
 
-  // 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,
-                      CupsPrintJob* print_job);
-
   // Process jobs from CUPS and perform notifications.
-  void UpdateJobs(const QueryResult& results);
+  void UpdateJobs(std::unique_ptr<QueryResult> results);
 
   // Mark remaining jobs as errors and remove active jobs.
   void PurgeJobs();
 
-  // Cancel the print job on the blocking thread.
-  void CancelJobImpl(const std::string& printer_id, const int job_id);
-
   // Notify observers that a state update has occured for |job|.
   void NotifyJobStateUpdate(CupsPrintJob* job);
 
   // Ongoing print jobs.
   std::map<std::string, std::unique_ptr<CupsPrintJob>> jobs_;
 
   // Prevents multiple queries from being scheduled simultaneously.
   bool in_query_ = false;
 
   // Records the number of consecutive times the GetJobs query has failed.
   int retry_count_ = 0;
 
-  ::printing::CupsConnection cups_connection_;
+  // Container for the cups connection to ensure that accesses are sequenced.
+  CupsWrapper cups_wrapper_;
+  base::WeakPtrFactory<CupsWrapper> cups_wrapper_ptr_factory_;

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

I don't understand why you aren't putting the weak ptr factory into CupsWrapper
directly?  I think what you've done here probably works, but it certainly breaks
the normal WeakPtrFactory pattern; I'd be inclined to put the WeakPtrFactory
into the wrapper and expose it as a member function just to preserve the pattern
and make this easier for people to understand.

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

It's SupportsWeakPtr now.

+
   content::NotificationRegistrar registrar_;
+
+  // Task runner for queries to CUPS.
+  scoped_refptr<base::SequencedTaskRunner> query_runner_;
+
+  // Task runner to for the Browser UI thread.
+  scoped_refptr<base::SingleThreadTaskRunner> browser_thread_runner_;
+
   base::WeakPtrFactory<CupsPrintJobManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CupsPrintJobManagerImpl);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_PRINTING_CUPS_PRINT_JOB_MANAGER_IMPL_H_