Change some printing methods to take a PrinterQuery.

The methods being modified are PrintJob::Initialize() and
PrintViewManagerBase::CreateNewPrintJob().

Right now they take PrinterQuery's parent class, PrintJobWorkerOwner.
However, in non-test situations, these methods always take a
PrinterQuery. So just make them to take a PrinterQuery. Once that is
done:

- PrintJobWorkerOwner no longer needs a cookie() method. Move that into
  PrinterQuery.
- A test class used for testing PrintJob now derives from PrinterQuery.
  Update the test class to let it continue working.

Change-Id: I6214fa481ac57a017111aa2a6f3e1b00c22d96c6
Reviewed-on: https://chromium-review.googlesource.com/1000965
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550809}

chrome/browser/printing/print_job.cc

@@ -51,18 +51,18 @@ PrintJob::~PrintJob() {
   DCHECK(RunsTasksInCurrentSequence());
 }
 
-void PrintJob::Initialize(PrintJobWorkerOwner* job,
+void PrintJob::Initialize(PrinterQuery* query,
                           const base::string16& name,
                           int page_count) {
   DCHECK(!worker_);
   DCHECK(!is_job_pending_);
   DCHECK(!is_canceling_);
   DCHECK(!document_);
-  worker_ = job->DetachWorker(this);
-  settings_ = job->settings();
+  worker_ = query->DetachWorker(this);
+  settings_ = query->settings();
 
   auto new_doc =
-      base::MakeRefCounted<PrintedDocument>(settings_, name, job->cookie());
+      base::MakeRefCounted<PrintedDocument>(settings_, name, query->cookie());
   new_doc->set_page_count(page_count);
   UpdatePrintedDocument(new_doc);
 
@@ -122,12 +122,6 @@ const PrintSettings& PrintJob::settings() const {
   return settings_;
 }
 
-int PrintJob::cookie() const {
-  // Use an invalid cookie if |document_| does not exist.
-  // TODO(thestig): Check if this is even reachable.
-  return document_ ? document_->cookie() : 0;
-}
-
 void PrintJob::StartPrinting() {
   DCHECK(RunsTasksInCurrentSequence());
   if (!worker_->IsRunning() || is_job_pending_) {

chrome/browser/printing/print_job.h

@@ -49,9 +49,10 @@ class PrintJob : public PrintJobWorkerOwner,
   // post-constructor initialization must be done with Initialize().
   PrintJob();
 
-  // Grabs the ownership of the PrintJobWorker from another job, which is
-  // usually a PrinterQuery. Set the expected page count of the print job.
-  virtual void Initialize(PrintJobWorkerOwner* job,
+  // Grabs the ownership of the PrintJobWorker from a PrinterQuery along with
+  // the print settings. Sets the expected page count of the print job based on
+  // the settings.
+  virtual void Initialize(PrinterQuery* query,
                           const base::string16& name,
                           int page_count);
 
@@ -75,7 +76,6 @@ class PrintJob : public PrintJobWorkerOwner,
   std::unique_ptr<PrintJobWorker> DetachWorker(
       PrintJobWorkerOwner* new_owner) override;
   const PrintSettings& settings() const override;
-  int cookie() const override;
 
   // Starts the actual printing. Signals the worker that it should begin to
   // spool as soon as data is available.

chrome/browser/printing/print_job_manager.cc

@@ -22,11 +22,11 @@ PrintQueriesQueue::~PrintQueriesQueue() {
   queued_queries_.clear();
 }
 
-void PrintQueriesQueue::QueuePrinterQuery(PrinterQuery* job) {
+void PrintQueriesQueue::QueuePrinterQuery(PrinterQuery* query) {
   base::AutoLock lock(lock_);
-  DCHECK(job);
-  queued_queries_.push_back(base::WrapRefCounted(job));
-  DCHECK(job->is_valid());
+  DCHECK(query);
+  queued_queries_.push_back(base::WrapRefCounted(query));
+  DCHECK(query->is_valid());
 }
 
 scoped_refptr<PrinterQuery> PrintQueriesQueue::PopPrinterQuery(

chrome/browser/printing/print_job_manager.h

@@ -29,7 +29,7 @@ class PrintQueriesQueue : public base::RefCountedThreadSafe<PrintQueriesQueue> {
   // Queues a semi-initialized worker thread. Can be called from any thread.
   // Current use case is queuing from the I/O thread.
   // TODO(maruel):  Have them vanish after a timeout (~5 minutes?)
-  void QueuePrinterQuery(PrinterQuery* job);
+  void QueuePrinterQuery(PrinterQuery* query);
 
   // Pops a queued PrintJobWorkerOwner object that was previously queued or
   // create new one. Can be called from any thread.

chrome/browser/printing/print_job_unittest.cc

@@ -13,6 +13,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/printing/print_job_worker.h"
+#include "chrome/browser/printing/printer_query.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/common/child_process_host.h"
@@ -32,17 +33,24 @@ class TestPrintJobWorker : public PrintJobWorker {
   friend class TestOwner;
 };
 
-class TestOwner : public PrintJobWorkerOwner {
+class TestOwner : public PrinterQuery {
  public:
-  TestOwner() {}
+  TestOwner()
+      : PrinterQuery(content::ChildProcessHost::kInvalidUniqueID,
+                     content::ChildProcessHost::kInvalidUniqueID) {}
 
   void GetSettingsDone(const PrintSettings& new_settings,
                        PrintingContext::Result result) override {
-    EXPECT_FALSE(true);
+    FAIL();
   }
 
   std::unique_ptr<PrintJobWorker> DetachWorker(
       PrintJobWorkerOwner* new_owner) override {
+    {
+      // Do an actual detach to keep the parent class happy.
+      auto real_worker = PrinterQuery::DetachWorker(new_owner);
+    }
+
     // We're screwing up here since we're calling worker from the main thread.
     // That's fine for testing. It is actually simulating PrinterQuery behavior.
     auto worker = std::make_unique<TestPrintJobWorker>(new_owner);
@@ -54,8 +62,6 @@ class TestOwner : public PrintJobWorkerOwner {
 
   const PrintSettings& settings() const override { return settings_; }
 
-  int cookie() const override { return 42; }
-
  private:
   ~TestOwner() override {}
 

chrome/browser/printing/print_job_worker_owner.h

@@ -39,9 +39,6 @@ class PrintJobWorkerOwner
   // Access the current settings.
   virtual const PrintSettings& settings() const = 0;
 
-  // Cookie uniquely identifying the PrintedDocument and/or loaded settings.
-  virtual int cookie() const = 0;
-
   // Returns true if tasks posted to this TaskRunner are sequenced
   // with this call.
   bool RunsTasksInCurrentSequence() const;

chrome/browser/printing/print_view_manager_base.cc

@@ -116,7 +116,7 @@ PrintViewManagerBase::PrintViewManagerBase(content::WebContents* web_contents)
       inside_inner_message_loop_(false),
       queue_(g_browser_process->print_job_manager()->queue()),
       weak_ptr_factory_(this) {
-  DCHECK(queue_.get());
+  DCHECK(queue_);
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   printing_enabled_.Init(
@@ -417,11 +417,11 @@ void PrintViewManagerBase::RenderFrameDeleted(
   PrintManager::PrintingRenderFrameDeleted();
   ReleasePrinterQuery();
 
-  if (!print_job_.get())
+  if (!print_job_)
     return;
 
   scoped_refptr<PrintedDocument> document(print_job_->document());
-  if (document.get()) {
+  if (document) {
     // If IsComplete() returns false, the document isn't completely rendered.
     // Since our renderer is gone, there's nothing to do, cancel it. Otherwise,
     // the print job may finish without problem.
@@ -523,7 +523,7 @@ void PrintViewManagerBase::OnNotifyPrintJobEvent(
 }
 
 bool PrintViewManagerBase::RenderAllMissingPagesNow() {
-  if (!print_job_.get() || !print_job_->is_job_pending())
+  if (!print_job_ || !print_job_->is_job_pending())
     return false;
 
   // Is the document already complete?
@@ -571,8 +571,9 @@ void PrintViewManagerBase::ShouldQuitFromInnerMessageLoop() {
   }
 }
 
-bool PrintViewManagerBase::CreateNewPrintJob(PrintJobWorkerOwner* job) {
+bool PrintViewManagerBase::CreateNewPrintJob(PrinterQuery* query) {
   DCHECK(!inside_inner_message_loop_);
+  DCHECK(query);
 
   // Disconnect the current |print_job_|.
   DisconnectFromCurrentPrintJob();
@@ -583,13 +584,9 @@ bool PrintViewManagerBase::CreateNewPrintJob(PrintJobWorkerOwner* job) {
     return false;
   }
 
-  DCHECK(!print_job_.get());
-  DCHECK(job);
-  if (!job)
-    return false;
-
+  DCHECK(!print_job_);
   print_job_ = base::MakeRefCounted<PrintJob>();
-  print_job_->Initialize(job, RenderSourceName(), number_pages_);
+  print_job_->Initialize(query, RenderSourceName(), number_pages_);
   registrar_.Add(this, chrome::NOTIFICATION_PRINT_JOB_EVENT,
                  content::Source<PrintJob>(print_job_.get()));
   printing_succeeded_ = false;
@@ -602,8 +599,7 @@ void PrintViewManagerBase::DisconnectFromCurrentPrintJob() {
   bool result = RenderAllMissingPagesNow();
 
   // Verify that assertion.
-  if (print_job_.get() &&
-      print_job_->document() &&
+  if (print_job_ && print_job_->document() &&
       !print_job_->document()->IsComplete()) {
     DCHECK(!result);
     // That failed.
@@ -615,7 +611,7 @@ void PrintViewManagerBase::DisconnectFromCurrentPrintJob() {
 }
 
 void PrintViewManagerBase::TerminatePrintJob(bool cancel) {
-  if (!print_job_.get())
+  if (!print_job_)
     return;
 
   if (cancel) {
@@ -638,7 +634,7 @@ void PrintViewManagerBase::ReleasePrintJob() {
   content::RenderFrameHost* rfh = printing_rfh_;
   printing_rfh_ = nullptr;
 
-  if (!print_job_.get())
+  if (!print_job_)
     return;
 
   if (rfh) {
@@ -692,7 +688,7 @@ bool PrintViewManagerBase::RunInnerMessageLoop() {
 }
 
 bool PrintViewManagerBase::OpportunisticallyCreatePrintJob(int cookie) {
-  if (print_job_.get())
+  if (print_job_)
     return true;
 
   if (!cookie) {
@@ -704,7 +700,7 @@ bool PrintViewManagerBase::OpportunisticallyCreatePrintJob(int cookie) {
   // The job was initiated by a script. Time to get the corresponding worker
   // thread.
   scoped_refptr<PrinterQuery> queued_query = queue_->PopPrinterQuery(cookie);
-  if (!queued_query.get()) {
+  if (!queued_query) {
     NOTREACHED();
     return false;
   }
@@ -748,7 +744,7 @@ void PrintViewManagerBase::ReleasePrinterQuery() {
 
   scoped_refptr<PrinterQuery> printer_query;
   printer_query = queue_->PopPrinterQuery(cookie);
-  if (!printer_query.get())
+  if (!printer_query)
     return;
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,

chrome/browser/printing/print_view_manager_base.h

@@ -35,7 +35,6 @@ namespace printing {
 
 class JobEventDetails;
 class PrintJob;
-class PrintJobWorkerOwner;
 class PrintQueriesQueue;
 class PrintedDocument;
 class PrinterQuery;
@@ -94,7 +93,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
   // currently a print job, safely disconnect from it. Returns false if it is
   // impossible to safely disconnect from the current print job or it is
   // impossible to create a new print job.
-  virtual bool CreateNewPrintJob(PrintJobWorkerOwner* job);
+  virtual bool CreateNewPrintJob(PrinterQuery* query);
 
   // Manages the low-level talk to the printer.
   scoped_refptr<PrintJob> print_job_;
@@ -129,12 +128,12 @@ class PrintViewManagerBase : public content::NotificationObserver,
       const scoped_refptr<base::RefCountedMemory>& print_data,
       int page_count,
       PrinterHandler::PrintCallback callback,
-      scoped_refptr<printing::PrinterQuery> printer_query);
+      scoped_refptr<PrinterQuery> printer_query);
 
   void StartLocalPrintJob(
       const scoped_refptr<base::RefCountedMemory>& print_data,
       int page_count,
-      scoped_refptr<printing::PrinterQuery> printer_query,
+      scoped_refptr<PrinterQuery> printer_query,
       PrinterHandler::PrintCallback callback);
 #endif  // BUILDFLAG(ENABLE_PRINT_PREVIEW)
 
@@ -211,7 +210,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
   // Whether printing is enabled.
   BooleanPrefMember printing_enabled_;
 
-  scoped_refptr<printing::PrintQueriesQueue> queue_;
+  scoped_refptr<PrintQueriesQueue> queue_;
 
   base::WeakPtrFactory<PrintViewManagerBase> weak_ptr_factory_;
 

chrome/browser/printing/print_view_manager_unittest.cc

@@ -71,9 +71,9 @@ class TestPrintViewManager : public PrintViewManagerBase {
 
  protected:
   // Override to create a TestPrintJob instead of a real one.
-  bool CreateNewPrintJob(PrintJobWorkerOwner* job) override {
+  bool CreateNewPrintJob(PrinterQuery* query) override {
     print_job_ = base::MakeRefCounted<TestPrintJob>();
-    print_job_->Initialize(job, RenderSourceName(), number_pages_);
+    print_job_->Initialize(query, RenderSourceName(), number_pages_);
     return true;
   }
 

chrome/browser/printing/printer_query.h

@@ -37,7 +37,6 @@ class PrinterQuery : public PrintJobWorkerOwner {
   std::unique_ptr<PrintJobWorker> DetachWorker(
       PrintJobWorkerOwner* new_owner) override;
   const PrintSettings& settings() const override;
-  int cookie() const override;
 
   // Initializes the printing context. It is fine to call this function multiple
   // times to reinitialize the settings. |web_contents_observer| can be queried
@@ -67,12 +66,16 @@ class PrinterQuery : public PrintJobWorkerOwner {
   // Returns true if a GetSettings() call is pending completion.
   bool is_callback_pending() const;
 
+  int cookie() const;
   PrintingContext::Result last_status() const { return last_status_; }
 
   // Returns if a worker thread is still associated to this instance.
   bool is_valid() const;
 
  protected:
+  // Refcounted class.
+  ~PrinterQuery() override;
+
   // For unit tests to manually set the print callback.
   void set_callback(base::OnceClosure callback);
 
@@ -81,9 +84,6 @@ class PrinterQuery : public PrintJobWorkerOwner {
   // worker thread per print job.
   std::unique_ptr<PrintJobWorker> worker_;
 
-  // Refcounted class.
-  ~PrinterQuery() override;
-
  private:
   // Lazy create the worker thread. There is one worker thread per print job.
   void StartWorker(base::OnceClosure callback);

chrome/browser/printing/test_print_job.cc

@@ -9,23 +9,23 @@
 #include "base/memory/ref_counted_memory.h"
 #include "build/build_config.h"
 #include "chrome/browser/printing/print_job_worker.h"
-#include "chrome/browser/printing/print_job_worker_owner.h"
+#include "chrome/browser/printing/printer_query.h"
 #include "chrome/browser/printing/test_print_job.h"
 #include "printing/printed_document.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace printing {
 
-void TestPrintJob::Initialize(PrintJobWorkerOwner* job,
+void TestPrintJob::Initialize(PrinterQuery* query,
                               const base::string16& name,
                               int page_count) {
   // Since we do not actually print in these tests, just let this get destroyed
   // when this function exits.
-  std::unique_ptr<PrintJobWorker> worker = job->DetachWorker(this);
-  set_settings(job->settings());
+  std::unique_ptr<PrintJobWorker> worker = query->DetachWorker(this);
+  set_settings(query->settings());
 
   scoped_refptr<PrintedDocument> new_doc =
-      base::MakeRefCounted<PrintedDocument>(settings(), name, job->cookie());
+      base::MakeRefCounted<PrintedDocument>(settings(), name, query->cookie());
 
   new_doc->set_page_count(page_count);
   UpdatePrintedDocument(new_doc.get());

chrome/browser/printing/test_print_job.h

@@ -11,11 +11,12 @@
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/printing/print_job.h"
-#include "chrome/browser/printing/print_job_worker_owner.h"
 #include "printing/print_settings.h"
 
 namespace printing {
 
+class PrinterQuery;
+
 class TestPrintJob : public PrintJob {
  public:
   // Create a empty PrintJob. When initializing with this constructor,
@@ -36,7 +37,7 @@ class TestPrintJob : public PrintJob {
                const content::NotificationDetails& details) override {}
 
   // All remaining functions are PrintJob implementation.
-  void Initialize(PrintJobWorkerOwner* job,
+  void Initialize(PrinterQuery* query,
                   const base::string16& name,
                   int page_count) override;