Print Preview: Change print to cr.sendWithPromise

chrome/browser/ui/webui/print_preview/print_preview_handler.cc

Index: chrome/browser/ui/webui/print_preview/print_preview_handler.cc
diff --git a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
index b4ceb816e49a79177a1e882ae57422e66f952db4..a91c79ccf81918dcf350f8f21155db302609c036 100644
--- a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
+++ b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
@@ -205,12 +205,7 @@ const int kPrivetTimeoutSec = 5;
 // Get the print job settings dictionary from |args|. The caller takes

[Lei Zhang, 2017/06/23 08:30:01]

Still referring to |args|.

[rbpotter, 2017/06/23 17:34:29]

Done.

 // ownership of the returned DictionaryValue. Returns NULL on failure.

[Lei Zhang, 2017/06/23 08:30:01]

BTW, no need to mention ownership since unique_ptr implies it now.

[rbpotter, 2017/06/23 17:34:29]

Done.

 std::unique_ptr<base::DictionaryValue> GetSettingsDictionary(
-    const base::ListValue* args) {
-  std::string json_str;
-  if (!args->GetString(0, &json_str)) {
-    NOTREACHED() << "Could not read JSON argument";
-    return NULL;
-  }
+    std::string json_str) {

[Lei Zhang, 2017/06/23 08:30:01]

Pass by const-ref, yet again. Does "git cl lint" warn about this? My guess is
no.

[rbpotter, 2017/06/23 17:34:29]

Done.

No, git cl lint doesn't warn about this.

   if (json_str.empty()) {
     NOTREACHED() << "Empty print job settings";
     return NULL;
@@ -688,8 +683,8 @@ void PrintPreviewHandler::HandleGetPrivetPrinters(const base::ListValue* args) {
   using local_discovery::ServiceDiscoverySharedClient;
   scoped_refptr<ServiceDiscoverySharedClient> service_discovery =
       ServiceDiscoverySharedClient::GetInstance();
-  DCHECK(privet_callback_id_.empty());
-  privet_callback_id_ = callback_id;
+  DCHECK(privet_search_callback_id_.empty());
+  privet_search_callback_id_ = callback_id;
   StartPrivetLister(service_discovery);
 #endif
 }
@@ -700,8 +695,9 @@ void PrintPreviewHandler::StopPrivetLister() {
   if (PrivetPrintingEnabled() && printer_lister_) {
     printer_lister_->Stop();
   }
-  ResolveJavascriptCallback(base::Value(privet_callback_id_), base::Value());
-  privet_callback_id_ = "";
+  ResolveJavascriptCallback(base::Value(privet_search_callback_id_),
+                            base::Value());
+  privet_search_callback_id_.clear();
 #endif
 }
 
@@ -776,7 +772,11 @@ void PrintPreviewHandler::HandleGetExtensionPrinterCapabilities(
 
 void PrintPreviewHandler::HandleGetPreview(const base::ListValue* args) {
   DCHECK_EQ(2U, args->GetSize());
-  std::unique_ptr<base::DictionaryValue> settings = GetSettingsDictionary(args);
+  std::string json_str;
+  if (!args->GetString(0, &json_str))
+    return;
+  std::unique_ptr<base::DictionaryValue> settings =
+      GetSettingsDictionary(json_str);
   if (!settings)
     return;
   int request_id = -1;
@@ -862,9 +862,18 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
   UMA_HISTOGRAM_COUNTS("PrintPreview.RegeneratePreviewRequest.BeforePrint",
                        regenerate_preview_request_count_);
 
-  std::unique_ptr<base::DictionaryValue> settings = GetSettingsDictionary(args);
+  AllowJavascript();
+
+  std::string callback_id;
+  CHECK(args->GetString(0, &callback_id) && !callback_id.empty());

[Lei Zhang, 2017/06/23 08:30:01]

CHECK() the 2 checks separately, so if this ever fails, we can tell which
CHECK() failed.

[rbpotter, 2017/06/23 17:34:29]

Done.

+  std::string json_str;
+  if (!args->GetString(1, &json_str))
+    RejectJavascriptCallback(base::Value(callback_id), base::Value(-1));
+
+  std::unique_ptr<base::DictionaryValue> settings =
+      GetSettingsDictionary(json_str);
   if (!settings)
-    return;
+    RejectJavascriptCallback(base::Value(callback_id), base::Value(-1));
 
   ReportPrintSettingsStats(*settings);
 
@@ -896,6 +905,8 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
   if (print_to_pdf) {
     UMA_HISTOGRAM_COUNTS("PrintPreview.PageCount.PrintToPDF", page_count);
     ReportUserActionHistogram(PRINT_TO_PDF);
+    DCHECK(pdf_callback_id_.empty());
+    pdf_callback_id_ = callback_id;
     PrintToPdf();
     return;
   }
@@ -917,12 +928,14 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
         !settings->GetInteger(printing::kSettingPageHeight, &height) ||
         width <= 0 || height <= 0) {
       NOTREACHED();
-      FireWebUIListener("print-failed", base::Value(-1));
+      RejectJavascriptCallback(base::Value(callback_id), base::Value(-1));
       return;
     }
 
-    PrintToPrivetPrinter(
-        printer_name, print_ticket, capabilities, gfx::Size(width, height));
+    DCHECK(privet_print_callback_id_.empty());
+    privet_print_callback_id_ = callback_id;
+    PrintToPrivetPrinter(callback_id, printer_name, print_ticket, capabilities,
+                         gfx::Size(width, height));
     return;
   }
 #endif
@@ -944,7 +957,7 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
         !settings->GetInteger(printing::kSettingPageHeight, &height) ||
         width <= 0 || height <= 0) {
       NOTREACHED();
-      OnExtensionPrintResult(false, "FAILED");
+      RejectJavascriptCallback(base::Value(callback_id), base::Value("FAILED"));
       return;
     }
 
@@ -952,7 +965,8 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
     scoped_refptr<base::RefCountedBytes> data;
     if (!GetPreviewDataAndTitle(&data, &title)) {
       LOG(ERROR) << "Nothing to print; no preview available.";
-      OnExtensionPrintResult(false, "NO_DATA");
+      RejectJavascriptCallback(base::Value(callback_id),
+                               base::Value("NO_DATA"));
       return;
     }
 
@@ -961,7 +975,7 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
         destination_id, capabilities, title, print_ticket,
         gfx::Size(width, height), data,
         base::Bind(&PrintPreviewHandler::OnExtensionPrintResult,
-                   weak_factory_.GetWeakPtr()));
+                   weak_factory_.GetWeakPtr(), callback_id));
     return;
   }
 
@@ -969,14 +983,14 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
   base::string16 title;
   if (!GetPreviewDataAndTitle(&data, &title)) {
     // Nothing to print, no preview available.
-    return;
+    RejectJavascriptCallback(base::Value(callback_id), base::Value());

[Lei Zhang, 2017/06/23 08:30:01]

Is this missing a return?

[rbpotter, 2017/06/23 17:34:29]

Done.

   }
 
   if (is_cloud_printer) {
     UMA_HISTOGRAM_COUNTS("PrintPreview.PageCount.PrintToCloudPrint",
                          page_count);
     ReportUserActionHistogram(PRINT_WITH_CLOUD_PRINT);
-    SendCloudPrintJob(data.get());
+    SendCloudPrintJob(callback_id, data.get());
     return;
   }
 
@@ -993,7 +1007,7 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
 
   // This tries to activate the initiator as well, so do not clear the

[Lei Zhang, 2017/06/23 08:30:01]

Is this comment still valid?

[rbpotter, 2017/06/23 17:34:29]

Yes, because OnHidePreviewDialog will get called after JS calls
chrome.send("hidePreview") - see HandleHidePreview(). Previously
it was actually getting called twice, since JS did this anyway...
To be safer with the ClearInitiatorDetails() below, I am moving
that into the HandleHidePreview function.

   // association with the initiator yet.
-  print_preview_ui()->OnHidePreviewDialog();
+  ResolveJavascriptCallback(base::Value(callback_id), base::Value());
 
   // Grab the current initiator before calling ClearInitiatorDetails() below.
   // Otherwise calling GetInitiator() later will return the wrong WebContents.
@@ -1036,6 +1050,8 @@ void PrintPreviewHandler::HandlePrint(const base::ListValue* args) {
 void PrintPreviewHandler::PrintToPdf() {
   if (!print_to_pdf_path_.empty()) {
     // User has already selected a path, no need to show the dialog again.
+    ResolveJavascriptCallback(base::Value(pdf_callback_id_), base::Value());
+    pdf_callback_id_.clear();
     PostPrintToPdfTask();
   } else if (!select_file_dialog_.get() ||
              !select_file_dialog_->IsRunning(platform_util::GetTopLevel(
@@ -1394,15 +1410,15 @@ void PrintPreviewHandler::SendCloudPrintEnabled() {
   }
 }
 
-void PrintPreviewHandler::SendCloudPrintJob(const base::RefCountedBytes* data) {
+void PrintPreviewHandler::SendCloudPrintJob(const std::string& callback_id,
+                                            const base::RefCountedBytes* data) {
   // BASE64 encode the job data.
   const base::StringPiece raw_data(reinterpret_cast<const char*>(data->front()),
                                    data->size());
   std::string base64_data;
   base::Base64Encode(raw_data, &base64_data);
-  base::Value data_value(base64_data);
 
-  web_ui()->CallJavascriptFunctionUnsafe("printToCloud", data_value);
+  ResolveJavascriptCallback(base::Value(callback_id), base::Value(base64_data));
 }
 
 WebContents* PrintPreviewHandler::GetInitiator() const {
@@ -1425,7 +1441,8 @@ void PrintPreviewHandler::SelectFile(const base::FilePath& default_filename,
     ChromeSelectFilePolicy policy(GetInitiator());
     if (!policy.CanOpenSelectFileDialog()) {
       policy.SelectFileDenied();
-      return ClosePreviewDialog();
+      RejectJavascriptCallback(base::Value(pdf_callback_id_), base::Value());
+      pdf_callback_id_.clear();

[Lei Zhang, 2017/06/23 08:30:01]

Is this suppose to return?

[rbpotter, 2017/06/23 17:34:30]

Return type is void. ClosePreviewDialog() does not return anything either,
so I think previously it was just using that as a return.

[Lei Zhang, 2017/06/23 20:39:57]

But now execution continues onwards, whereas before it didn't.

     }
   }
 
@@ -1494,7 +1511,8 @@ void PrintPreviewHandler::FileSelected(const base::FilePath& path,
   sticky_settings->SaveInPrefs(
       Profile::FromBrowserContext(preview_web_contents()->GetBrowserContext())
           ->GetPrefs());
-  web_ui()->CallJavascriptFunctionUnsafe("fileSelectionCompleted");
+  ResolveJavascriptCallback(base::Value(pdf_callback_id_), base::Value());
+  pdf_callback_id_.clear();
   print_to_pdf_path_ = path;
   PostPrintToPdfTask();
 }
@@ -1516,7 +1534,8 @@ void PrintPreviewHandler::PostPrintToPdfTask() {
 }
 
 void PrintPreviewHandler::FileSelectionCanceled(void* params) {
-  print_preview_ui()->OnFileSelectionCancelled();
+  RejectJavascriptCallback(base::Value(pdf_callback_id_), base::Value());
+  pdf_callback_id_.clear();
 }
 
 void PrintPreviewHandler::ClearInitiatorDetails() {
@@ -1604,13 +1623,10 @@ bool PrintPreviewHandler::PrivetUpdateClient(
     const std::string& callback_id,
     std::unique_ptr<cloud_print::PrivetHTTPClient> http_client) {
   if (!http_client) {
-    if (callback_id.empty()) {
-      // This was an attempt to print to a privet printer and has failed.
-      FireWebUIListener("print-failed", base::Value(-1));
-    } else {  // Capabilities update failed
-      RejectJavascriptCallback(base::Value(callback_id), base::Value());
-    }
+    RejectJavascriptCallback(base::Value(callback_id), base::Value());
     privet_http_resolution_.reset();
+    if (callback_id == privet_print_callback_id_)
+      privet_print_callback_id_.clear();
     return false;
   }
 
@@ -1625,11 +1641,12 @@ bool PrintPreviewHandler::PrivetUpdateClient(
 }
 
 void PrintPreviewHandler::PrivetLocalPrintUpdateClient(
+    const std::string& callback_id,
     std::string print_ticket,
     std::string capabilities,
     gfx::Size page_size,
     std::unique_ptr<cloud_print::PrivetHTTPClient> http_client) {
-  if (!PrivetUpdateClient("", std::move(http_client)))
+  if (!PrivetUpdateClient(callback_id, std::move(http_client)))
     return;
 
   StartPrivetLocalPrint(print_ticket, capabilities, page_size);
@@ -1648,7 +1665,9 @@ void PrintPreviewHandler::StartPrivetLocalPrint(const std::string& print_ticket,
   base::string16 title;
 
   if (!GetPreviewDataAndTitle(&data, &title)) {
-    FireWebUIListener("print-failed", base::Value(-1));
+    RejectJavascriptCallback(base::Value(privet_print_callback_id_),
+                             base::Value(-1));
+    privet_print_callback_id_.clear();
     return;
   }
 
@@ -1701,16 +1720,19 @@ void PrintPreviewHandler::OnPrivetCapabilities(
   privet_capabilities_operation_.reset();
 }
 
-void PrintPreviewHandler::PrintToPrivetPrinter(const std::string& device_name,
+void PrintPreviewHandler::PrintToPrivetPrinter(const std::string& callback_id,
+                                               const std::string& device_name,
                                                const std::string& ticket,
                                                const std::string& capabilities,
                                                const gfx::Size& page_size) {
   if (!CreatePrivetHTTP(
           device_name,
           base::Bind(&PrintPreviewHandler::PrivetLocalPrintUpdateClient,
-                     weak_factory_.GetWeakPtr(), ticket, capabilities,
-                     page_size))) {
-    FireWebUIListener("print-failed", base::Value(-1));
+                     weak_factory_.GetWeakPtr(), callback_id, ticket,
+                     capabilities, page_size))) {
+    RejectJavascriptCallback(base::Value(privet_print_callback_id_),
+                             base::Value(-1));
+    privet_print_callback_id_.clear();
   }
 }
 
@@ -1735,13 +1757,17 @@ bool PrintPreviewHandler::CreatePrivetHTTP(
 
 void PrintPreviewHandler::OnPrivetPrintingDone(
     const cloud_print::PrivetLocalPrintOperation* print_operation) {
-  ClosePreviewDialog();
+  ResolveJavascriptCallback(base::Value(privet_print_callback_id_),
+                            base::Value());
+  privet_print_callback_id_.clear();
 }
 
 void PrintPreviewHandler::OnPrivetPrintingError(
     const cloud_print::PrivetLocalPrintOperation* print_operation,
     int http_code) {
-  FireWebUIListener("print-failed", base::Value(http_code));
+  RejectJavascriptCallback(base::Value(privet_print_callback_id_),
+                           base::Value(http_code));
+  privet_print_callback_id_.clear();
 }
 
 void PrintPreviewHandler::FillPrinterDescription(
@@ -1805,13 +1831,14 @@ void PrintPreviewHandler::OnGotExtensionPrinterCapabilities(
   ResolveJavascriptCallback(base::Value(callback_id), capabilities);
 }
 
-void PrintPreviewHandler::OnExtensionPrintResult(bool success,
+void PrintPreviewHandler::OnExtensionPrintResult(const std::string& callback_id,
+                                                 bool success,
                                                  const std::string& status) {
   if (success) {
-    ClosePreviewDialog();
+    ResolveJavascriptCallback(base::Value(callback_id), base::Value());

[Lei Zhang, 2017/06/23 08:30:01]

We are calling: ResolveJavascriptCallback(base::Value(callback_id), ...) either
way. Merge into 1 call?

[rbpotter, 2017/06/23 17:34:29]

Below is RejectJavascriptCallback, not Resolve, so they need to be
separate calls.

     return;
   }
-  FireWebUIListener("print-failed", base::Value(status));
+  RejectJavascriptCallback(base::Value(callback_id), base::Value(status));
 }
 
 void PrintPreviewHandler::RegisterForGaiaCookieChanges() {