Move PDF to PWG conversion into PrivetLocalPrintOperationImpl

Move PDF to PWG conversion into PrivetLocalPrintOperationImpl

Change the format of the data provided to PrivetLocalPrintOperationImpl to
RefCountedBytes. Remove PrivetLocalPrintRequestPDF/PrivetLocalPrintRequestPWG.
Have users provide PDF data consistently, and do conversin in the
PrivetLocalPrintOperation.

BUG=318373
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235820

[Vitaly Buka (NO REVIEWS), 2013-11-15 06:48:38]

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http.h:134: virtual void
SetData(scoped_refptr<base::RefCountedBytes> data) = 0;
just base::RefCountedBytes*

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http.h:149: 
same

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/privet_http_unittest.cc (right):

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:848:
local_print_operation_->SetData(RefCountedBytesFromString(
line break before RefCountedBytesFromString

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:873:
local_print_operation_->SetData(RefCountedBytesFromString(
same

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:899:
local_print_operation_->SetData(RefCountedBytesFromString(
same

[Vitaly Buka (NO REVIEWS), 2013-11-15 06:48:55]

lgtm

BUGID?

[Noam Samuel, 2013-11-15 19:13:46]

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/privet_http.h (right):

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http.h:134: virtual void
SetData(scoped_refptr<base::RefCountedBytes> data) = 0;
On 2013/11/15 06:48:38, Vitaly Buka wrote:
> just base::RefCountedBytes*

Done.

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http.h:149: 
On 2013/11/15 06:48:38, Vitaly Buka wrote:
> same

Using a scoped_ptr denotes the transfer of ownership, so I feel it's
semantically useful. Unless you think pwg_raster_converter should be
ref-counted?

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
File chrome/browser/local_discovery/privet_http_unittest.cc (right):

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:848:
local_print_operation_->SetData(RefCountedBytesFromString(
On 2013/11/15 06:48:38, Vitaly Buka wrote:
> line break before RefCountedBytesFromString

Done.

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:873:
local_print_operation_->SetData(RefCountedBytesFromString(
On 2013/11/15 06:48:38, Vitaly Buka wrote:
> same

Done.

https://codereview.chromium.org/73503002/diff/1/chrome/browser/local_discover...
chrome/browser/local_discovery/privet_http_unittest.cc:899:
local_print_operation_->SetData(RefCountedBytesFromString(
On 2013/11/15 06:48:38, Vitaly Buka wrote:
> same

Done.

[Vitaly Buka (NO REVIEWS), 2013-11-15 20:06:04]

lgtm

[commit-bot: I haz the power, 2013-11-15 20:58:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/73503002/90001

[commit-bot: I haz the power, 2013-11-15 20:59:50]

Failed to apply patch for chrome/browser/local_discovery/privet_http.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/local_discovery/privet_http.h
  Hunk #3 FAILED at 131.
  Hunk #4 succeeded at 147 (offset -1 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/local_discovery/privet_http.h.rej

Patch:       chrome/browser/local_discovery/privet_http.h
Index: chrome/browser/local_discovery/privet_http.h
diff --git a/chrome/browser/local_discovery/privet_http.h
b/chrome/browser/local_discovery/privet_http.h
index
1c38a1724cfe51b1424367ff236956c45b57225b..035e9f184c7624b09c84f12a885211387e4ba4a5
100644
--- a/chrome/browser/local_discovery/privet_http.h
+++ b/chrome/browser/local_discovery/privet_http.h
@@ -9,8 +9,13 @@
 
 #include "base/callback.h"
 #include "chrome/browser/local_discovery/privet_url_fetcher.h"
+#include "chrome/browser/local_discovery/pwg_raster_converter.h"
 #include "net/base/host_port_pair.h"
 
+namespace base {
+class RefCountedBytes;
+}
+
 namespace local_discovery {
 
 class PrivetHTTPClient;
@@ -116,10 +121,6 @@ class PrivetLocalPrintOperation {
   class Delegate {
    public:
     virtual ~Delegate() {}
-    virtual void OnPrivetPrintingRequestPDF(
-        const PrivetLocalPrintOperation* print_operation) = 0;
-    virtual void OnPrivetPrintingRequestPWGRaster(
-        const PrivetLocalPrintOperation* print_operation) = 0;
     virtual void OnPrivetPrintingDone(
         const PrivetLocalPrintOperation* print_operation) = 0;
     virtual void OnPrivetPrintingError(
@@ -130,12 +131,9 @@ class PrivetLocalPrintOperation {
 
   virtual void Start() = 0;
 
-  // One of thsese should be called ONLY after |OnPrivetPrintingRequestPDF| or
-  // |OnPrivetPrintingRequestPWGRaster| are called on the delegate.  Data
should
-  // be in PDF or PWG format depending on what is requested by the local print
-  // operation.
-  virtual void SendData(const std::string& data) = 0;
-  virtual void SendDataFile(const base::FilePath& data) = 0;
+
+  // Required print data. MUST be called before calling |Start()|.
+  virtual void SetData(base::RefCountedBytes* data) = 0;
 
   // Optional attributes for /submitdoc. Call before calling |Start()|
   // |ticket| should be in CJT format.
@@ -147,6 +145,10 @@ class PrivetLocalPrintOperation {
   // to Google Cloud Print.
   virtual void SetOffline(bool offline) = 0;
 
+  // For testing, inject an alternative PWG raster converter.
+  virtual void SetPWGRasterConverterForTesting(
+      scoped_ptr<PWGRasterConverter> pwg_raster_converter) = 0;
+
   virtual PrivetHTTPClient* GetHTTPClient() = 0;
 };

[commit-bot: I haz the power, 2013-11-18 08:35:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/73503002/250001

[commit-bot: I haz the power, 2013-11-18 08:35:15]

Failed to apply patch for chrome/browser/local_discovery/privet_http_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/local_discovery/privet_http_impl.cc
  Hunk #6 FAILED at 582.
  Hunk #7 succeeded at 609 (offset -1 lines).
  Hunk #8 succeeded at 647 (offset -1 lines).
  Hunk #9 FAILED at 673.
  2 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/local_discovery/privet_http_impl.cc.rej

Patch:       chrome/browser/local_discovery/privet_http_impl.cc
Index: chrome/browser/local_discovery/privet_http_impl.cc
diff --git a/chrome/browser/local_discovery/privet_http_impl.cc
b/chrome/browser/local_discovery/privet_http_impl.cc
index
9266bac6eb7a22c89efaa2d35eba7622f0ce7372..55cb05c7ba64608bbd5085e7563172a3f9dbfcf1
100644
--- a/chrome/browser/local_discovery/privet_http_impl.cc
+++ b/chrome/browser/local_discovery/privet_http_impl.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/local_discovery/privet_http_impl.h"
 
 #include <algorithm>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
@@ -368,6 +369,7 @@
PrivetLocalPrintOperationImpl::PrivetLocalPrintOperationImpl(
     : privet_client_(privet_client), delegate_(delegate),
       use_pdf_(false), has_capabilities_(false), has_extended_workflow_(false),
       started_(false), offline_(false), invalid_job_retries_(0),
+      pwg_raster_converter_(PWGRasterConverter::CreateDefault()),
       weak_factory_(this) {
 }
 
@@ -427,7 +429,7 @@ void PrivetLocalPrintOperationImpl::StartInitialRequest() {
     // Since we have no capabiltties, the only reasonable format we can
     // request is PWG Raster.
     use_pdf_ = false;
-    delegate_->OnPrivetPrintingRequestPWGRaster(this);
+    StartConvertToPWG();
   }
 }
 
@@ -489,18 +491,31 @@ void PrivetLocalPrintOperationImpl::DoSubmitdoc() {
   url_fetcher_= privet_client_->CreateURLFetcher(
       url, net::URLFetcher::POST, this);
 
-  std::string content_type =
-      use_pdf_ ? kPrivetContentTypePDF : kPrivetContentTypePWGRaster;
+  if (!use_pdf_) {
+    url_fetcher_->SetUploadFilePath(kPrivetContentTypePWGRaster,
+                                    pwg_file_path_);
+  } else {
+    // TODO(noamsml): Move to file-based upload data?
+    std::string data_str((const char*)data_->front(), data_->size());
+    url_fetcher_->SetUploadData(kPrivetContentTypePDF, data_str);
+  }
 
-  DCHECK(!data_.empty() || !data_file_.empty());
+  url_fetcher_->Start();
+}
 
-  if (!data_file_.empty()) {
-    url_fetcher_->SetUploadFilePath(content_type, data_file_);
+void PrivetLocalPrintOperationImpl::StartPrinting() {
+  if (has_extended_workflow_ && !ticket_.empty() && jobid_.empty()) {
+    DoCreatejob();
   } else {
-    url_fetcher_->SetUploadData(content_type, data_);
+    DoSubmitdoc();
   }
+}
 
-  url_fetcher_->Start();
+void PrivetLocalPrintOperationImpl::StartConvertToPWG() {
+  pwg_raster_converter_->Start(
+      data_,
+      base::Bind(&PrivetLocalPrintOperationImpl::OnPWGRasterConverted,
+                 base::Unretained(this)));
 }
 
 void PrivetLocalPrintOperationImpl::OnCapabilitiesResponse(
@@ -531,10 +546,11 @@ void
PrivetLocalPrintOperationImpl::OnCapabilitiesResponse(
     }
   }
 
-  if (use_pdf_)
-    delegate_->OnPrivetPrintingRequestPDF(this);
-  else
-    delegate_->OnPrivetPrintingRequestPWGRaster(this);
+  if (use_pdf_) {
+    StartPrinting();
+  } else {
+    StartConvertToPWG();
+  }
 }
 
 void PrivetLocalPrintOperationImpl::OnSubmitdocResponse(
@@ -566,7 +582,7 @@ void PrivetLocalPrintOperationImpl::OnSubmitdocResponse(
           base::TimeDelta::FromSeconds(timeout));
     } else if (use_pdf_ && error == kPrivetErrorInvalidDocumentType) {
       use_pdf_ = false;
-      delegate_->OnPrivetPrintingRequestPWGRaster(this);
+      StartConvertToPWG();
     } else {
       delegate_->OnPrivetPrintingError(this, 200);
     }
@@ -594,6 +610,20 @@ void PrivetLocalPrintOperationImpl::OnCreatejobResponse(
   DoSubmitdoc();
 }
 
+void PrivetLocalPrintOperationImpl::OnPWGRasterConverted(
+    bool success,
+    const base::FilePath& pwg_file_path) {
+  if (!success) {
+    delegate_->OnPrivetPrintingError(this, -1);
+    return;
+  }
+
+  DCHECK(!pwg_file_path.empty());
+
+  pwg_file_path_ = pwg_file_path;
+  StartPrinting();
+}
+
 PrivetHTTPClient* PrivetLocalPrintOperationImpl::GetHTTPClient() {
   return privet_client_;
 }
@@ -618,21 +648,9 @@ void PrivetLocalPrintOperationImpl::OnNeedPrivetToken(
   privet_client_->RefreshPrivetToken(callback);
 }
 
-void PrivetLocalPrintOperationImpl::SendData(const std::string& data) {
-  DCHECK(started_);
-  DCHECK(data_file_.empty());
+void PrivetLocalPrintOperationImpl::SetData(base::RefCountedBytes* data) {
+  DCHECK(!started_);
   data_ = data;
-
-  SendDataInternal();
-}
-
-void PrivetLocalPrintOperationImpl::SendDataFile(
-    const base::FilePath& data_file) {
-  DCHECK(started_);
-  DCHECK(data_.empty());
-  data_file_ = data_file;
-
-  SendDataInternal();
 }
 
 void PrivetLocalPrintOperationImpl::SetTicket(const std::string& ticket) {
@@ -655,12 +673,9 @@ void PrivetLocalPrintOperationImpl::SetOffline(bool
offline) {
   offline_ = offline;
 }
 
-void PrivetLocalPrintOperationImpl::SendDataInternal() {
-  if (has_extended_workflow_ && !ticket_.empty() && jobid_.empty()) {
-    DoCreatejob();
-  } else {
-    DoSubmitdoc();
-  }
+void PrivetLocalPrintOperationImpl::SetPWGRasterConverterForTesting(
+    scoped_ptr<PWGRasterConverter> pwg_raster_converter) {
+  pwg_raster_converter_ = pwg_raster_converter.Pass();
 }
 
 PrivetHTTPClientImpl::PrivetHTTPClientImpl(

[commit-bot: I haz the power, 2013-11-18 08:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/73503002/250001

[commit-bot: I haz the power, 2013-11-18 08:40:55]

Failed to apply patch for chrome/browser/local_discovery/privet_http_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/local_discovery/privet_http_impl.cc
  Hunk #6 FAILED at 582.
  Hunk #7 succeeded at 609 (offset -1 lines).
  Hunk #8 succeeded at 647 (offset -1 lines).
  Hunk #9 FAILED at 673.
  2 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/local_discovery/privet_http_impl.cc.rej

Patch:       chrome/browser/local_discovery/privet_http_impl.cc
Index: chrome/browser/local_discovery/privet_http_impl.cc
diff --git a/chrome/browser/local_discovery/privet_http_impl.cc
b/chrome/browser/local_discovery/privet_http_impl.cc
index
9266bac6eb7a22c89efaa2d35eba7622f0ce7372..55cb05c7ba64608bbd5085e7563172a3f9dbfcf1
100644
--- a/chrome/browser/local_discovery/privet_http_impl.cc
+++ b/chrome/browser/local_discovery/privet_http_impl.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/local_discovery/privet_http_impl.h"
 
 #include <algorithm>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
@@ -368,6 +369,7 @@
PrivetLocalPrintOperationImpl::PrivetLocalPrintOperationImpl(
     : privet_client_(privet_client), delegate_(delegate),
       use_pdf_(false), has_capabilities_(false), has_extended_workflow_(false),
       started_(false), offline_(false), invalid_job_retries_(0),
+      pwg_raster_converter_(PWGRasterConverter::CreateDefault()),
       weak_factory_(this) {
 }
 
@@ -427,7 +429,7 @@ void PrivetLocalPrintOperationImpl::StartInitialRequest() {
     // Since we have no capabiltties, the only reasonable format we can
     // request is PWG Raster.
     use_pdf_ = false;
-    delegate_->OnPrivetPrintingRequestPWGRaster(this);
+    StartConvertToPWG();
   }
 }
 
@@ -489,18 +491,31 @@ void PrivetLocalPrintOperationImpl::DoSubmitdoc() {
   url_fetcher_= privet_client_->CreateURLFetcher(
       url, net::URLFetcher::POST, this);
 
-  std::string content_type =
-      use_pdf_ ? kPrivetContentTypePDF : kPrivetContentTypePWGRaster;
+  if (!use_pdf_) {
+    url_fetcher_->SetUploadFilePath(kPrivetContentTypePWGRaster,
+                                    pwg_file_path_);
+  } else {
+    // TODO(noamsml): Move to file-based upload data?
+    std::string data_str((const char*)data_->front(), data_->size());
+    url_fetcher_->SetUploadData(kPrivetContentTypePDF, data_str);
+  }
 
-  DCHECK(!data_.empty() || !data_file_.empty());
+  url_fetcher_->Start();
+}
 
-  if (!data_file_.empty()) {
-    url_fetcher_->SetUploadFilePath(content_type, data_file_);
+void PrivetLocalPrintOperationImpl::StartPrinting() {
+  if (has_extended_workflow_ && !ticket_.empty() && jobid_.empty()) {
+    DoCreatejob();
   } else {
-    url_fetcher_->SetUploadData(content_type, data_);
+    DoSubmitdoc();
   }
+}
 
-  url_fetcher_->Start();
+void PrivetLocalPrintOperationImpl::StartConvertToPWG() {
+  pwg_raster_converter_->Start(
+      data_,
+      base::Bind(&PrivetLocalPrintOperationImpl::OnPWGRasterConverted,
+                 base::Unretained(this)));
 }
 
 void PrivetLocalPrintOperationImpl::OnCapabilitiesResponse(
@@ -531,10 +546,11 @@ void
PrivetLocalPrintOperationImpl::OnCapabilitiesResponse(
     }
   }
 
-  if (use_pdf_)
-    delegate_->OnPrivetPrintingRequestPDF(this);
-  else
-    delegate_->OnPrivetPrintingRequestPWGRaster(this);
+  if (use_pdf_) {
+    StartPrinting();
+  } else {
+    StartConvertToPWG();
+  }
 }
 
 void PrivetLocalPrintOperationImpl::OnSubmitdocResponse(
@@ -566,7 +582,7 @@ void PrivetLocalPrintOperationImpl::OnSubmitdocResponse(
           base::TimeDelta::FromSeconds(timeout));
     } else if (use_pdf_ && error == kPrivetErrorInvalidDocumentType) {
       use_pdf_ = false;
-      delegate_->OnPrivetPrintingRequestPWGRaster(this);
+      StartConvertToPWG();
     } else {
       delegate_->OnPrivetPrintingError(this, 200);
     }
@@ -594,6 +610,20 @@ void PrivetLocalPrintOperationImpl::OnCreatejobResponse(
   DoSubmitdoc();
 }
 
+void PrivetLocalPrintOperationImpl::OnPWGRasterConverted(
+    bool success,
+    const base::FilePath& pwg_file_path) {
+  if (!success) {
+    delegate_->OnPrivetPrintingError(this, -1);
+    return;
+  }
+
+  DCHECK(!pwg_file_path.empty());
+
+  pwg_file_path_ = pwg_file_path;
+  StartPrinting();
+}
+
 PrivetHTTPClient* PrivetLocalPrintOperationImpl::GetHTTPClient() {
   return privet_client_;
 }
@@ -618,21 +648,9 @@ void PrivetLocalPrintOperationImpl::OnNeedPrivetToken(
   privet_client_->RefreshPrivetToken(callback);
 }
 
-void PrivetLocalPrintOperationImpl::SendData(const std::string& data) {
-  DCHECK(started_);
-  DCHECK(data_file_.empty());
+void PrivetLocalPrintOperationImpl::SetData(base::RefCountedBytes* data) {
+  DCHECK(!started_);
   data_ = data;
-
-  SendDataInternal();
-}
-
-void PrivetLocalPrintOperationImpl::SendDataFile(
-    const base::FilePath& data_file) {
-  DCHECK(started_);
-  DCHECK(data_.empty());
-  data_file_ = data_file;
-
-  SendDataInternal();
 }
 
 void PrivetLocalPrintOperationImpl::SetTicket(const std::string& ticket) {
@@ -655,12 +673,9 @@ void PrivetLocalPrintOperationImpl::SetOffline(bool
offline) {
   offline_ = offline;
 }
 
-void PrivetLocalPrintOperationImpl::SendDataInternal() {
-  if (has_extended_workflow_ && !ticket_.empty() && jobid_.empty()) {
-    DoCreatejob();
-  } else {
-    DoSubmitdoc();
-  }
+void PrivetLocalPrintOperationImpl::SetPWGRasterConverterForTesting(
+    scoped_ptr<PWGRasterConverter> pwg_raster_converter) {
+  pwg_raster_converter_ = pwg_raster_converter.Pass();
 }
 
 PrivetHTTPClientImpl::PrivetHTTPClientImpl(

[commit-bot: I haz the power, 2013-11-18 19:37:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noamsml@chromium.org/73503002/250001

[commit-bot: I haz the power, 2013-11-18 21:32:13]

Change committed as 235820