Print Preview: Change getPrinterCapabilities 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 e0a11e50259acc571042f4c1fd52dff7783983fe..610301b3e721519cf512772772e73684f1d28f1f 100644
--- a/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
+++ b/chrome/browser/ui/webui/print_preview/print_preview_handler.cc
@@ -706,15 +706,25 @@ void PrintPreviewHandler::StopPrivetLister() {
 
 void PrintPreviewHandler::HandleGetPrivetPrinterCapabilities(
     const base::ListValue* args) {
-#if BUILDFLAG(ENABLE_SERVICE_DISCOVERY)
-  std::string name;
-  bool success = args->GetString(0, &name);
-  DCHECK(success);
+  AllowJavascript();
 
-  CreatePrivetHTTP(
-      name, base::Bind(&PrintPreviewHandler::PrivetCapabilitiesUpdateClient,
-                       weak_factory_.GetWeakPtr()));
+  std::string callback_id;
+  std::string printer_name;
+  if (!args->GetString(0, &callback_id) || !args->GetString(1, &printer_name) ||
+      callback_id.empty() || printer_name.empty()) {
+    RejectJavascriptCallback(base::Value(callback_id),
+                             base::Value(printer_name));
+    return;
+  }
+#if BUILDFLAG(ENABLE_SERVICE_DISCOVERY)
+  if (CreatePrivetHTTP(
+          printer_name,
+          base::Bind(&PrintPreviewHandler::PrivetCapabilitiesUpdateClient,
+                     weak_factory_.GetWeakPtr(), callback_id))) {
+    return;
+  }
 #endif
+  RejectJavascriptCallback(base::Value(callback_id), base::Value(printer_name));
 }
 
 void PrintPreviewHandler::HandleGetExtensionPrinters(
@@ -747,15 +757,22 @@ void PrintPreviewHandler::HandleGrantExtensionPrinterAccess(
 
 void PrintPreviewHandler::HandleGetExtensionPrinterCapabilities(
     const base::ListValue* args) {
-  std::string printer_id;
-  bool ok = args->GetString(0, &printer_id);
-  DCHECK(ok);
+  AllowJavascript();
+
+  std::string callback_id;
+  std::string printer_name;
+  if (!args->GetString(0, &callback_id) || !args->GetString(1, &printer_name) ||
+      callback_id.empty() || printer_name.empty()) {
+    RejectJavascriptCallback(base::Value(callback_id),
+                             base::Value(printer_name));
+    return;
+  }
 
   EnsureExtensionPrinterHandlerSet();
   extension_printer_handler_->StartGetCapability(
-      printer_id,
+      printer_name,
       base::Bind(&PrintPreviewHandler::OnGotExtensionPrinterCapabilities,
-                 weak_factory_.GetWeakPtr()));
+                 weak_factory_.GetWeakPtr(), callback_id));
 }
 
 void PrintPreviewHandler::HandleGetPreview(const base::ListValue* args) {
@@ -1071,10 +1088,16 @@ void PrintPreviewHandler::HandleSaveAppState(const base::ListValue* args) {
 
 void PrintPreviewHandler::HandleGetPrinterCapabilities(
     const base::ListValue* args) {
+  AllowJavascript();
+
+  std::string callback_id;
   std::string printer_name;
-  bool ret = args->GetString(0, &printer_name);
-  if (!ret || printer_name.empty())
+  if (!args->GetString(0, &callback_id) || !args->GetString(1, &printer_name) ||
+      callback_id.empty() || printer_name.empty()) {
+    RejectJavascriptCallback(base::Value(callback_id),
+                             base::Value(printer_name));
     return;
+  }
 
   if (printer_name == kLocalPdfPrinterId) {
     auto printer_info = base::MakeUnique<base::DictionaryValue>();
@@ -1082,13 +1105,13 @@ void PrintPreviewHandler::HandleGetPrinterCapabilities(
     printer_info->Set(
         printing::kPrinterCapabilities,
         GetPdfCapabilities(g_browser_process->GetApplicationLocale()));
-    SendPrinterCapabilities(printer_name, std::move(printer_info));
+    SendPrinterCapabilities(callback_id, printer_name, std::move(printer_info));
     return;
   }
 
   printing::PrinterSetupCallback cb =
       base::Bind(&PrintPreviewHandler::SendPrinterCapabilities,
-                 weak_factory_.GetWeakPtr(), printer_name);
+                 weak_factory_.GetWeakPtr(), callback_id, printer_name);
 
   printer_backend_proxy()->ConfigurePrinterAndFetchCapabilities(printer_name,
                                                                 cb);
@@ -1307,19 +1330,18 @@ void PrintPreviewHandler::SendAccessToken(const std::string& type,
 }
 
 void PrintPreviewHandler::SendPrinterCapabilities(
+    const std::string& callback_id,
     const std::string& printer_name,
     std::unique_ptr<base::DictionaryValue> settings_info) {
   // Check that |settings_info| is valid.
   if (settings_info && settings_info->Get("capabilities", nullptr)) {
     VLOG(1) << "Get printer capabilities finished";
-    web_ui()->CallJavascriptFunctionUnsafe("updateWithPrinterCapabilities",
-                                           *settings_info);
+    ResolveJavascriptCallback(base::Value(callback_id), *settings_info);
     return;
   }
 
   VLOG(1) << "Get printer capabilities failed";
-  web_ui()->CallJavascriptFunctionUnsafe("failedToGetPrinterCapabilities",
-                                         base::Value(printer_name));
+  RejectJavascriptCallback(base::Value(callback_id), base::Value(printer_name));
 }
 
 void PrintPreviewHandler::SendPrinterSetup(
@@ -1570,21 +1592,31 @@ void PrintPreviewHandler::LocalPrinterCacheFlushed() {
 }
 
 void PrintPreviewHandler::PrivetCapabilitiesUpdateClient(
+    const std::string& callback_id,
     std::unique_ptr<cloud_print::PrivetHTTPClient> http_client) {
-  if (!PrivetUpdateClient(std::move(http_client)))
+  if (!PrivetUpdateClient(callback_id, std::move(http_client)))
     return;
 
   privet_capabilities_operation_ =
       privet_http_client_->CreateCapabilitiesOperation(
           base::Bind(&PrintPreviewHandler::OnPrivetCapabilities,
-                     weak_factory_.GetWeakPtr()));
+                     weak_factory_.GetWeakPtr(), callback_id));
   privet_capabilities_operation_->Start();
 }
 
 bool PrintPreviewHandler::PrivetUpdateClient(
+    const std::string& callback_id,
     std::unique_ptr<cloud_print::PrivetHTTPClient> http_client) {
   if (!http_client) {
-    SendPrivetCapabilitiesError(privet_http_resolution_->GetName());
+    if (callback_id.empty()) {

[rbpotter, 2017/06/09 10:24:23]

Here and in one other location the capabilities error was being
triggered despite the fact that we reach this point only via a
printing call. It seems like it makes more sense to call a
privet printing fail at that point rather than a capabilities
error, which just checks if the destination was auto-filled and
then picks another one but does nothing to indicate or recover
from a printing fail.

If we want to keep the original behavior, could add a listener
for this privet printing failed event in the destination store and
handle it in the same way as the capabilities fetch failure.

[dpapad, 2017/06/09 17:59:11]

Hm, I don't fully understand the implications here. At first glance, why do we
ever get an empty callback_id here? I see that we are passing "" at line 1639,
but is this temporary (until we convert more stuff to sendWithPromise)?

In theory all async requests from JS to C++ are backed by a Promise, and we
reject that promise, eliminating the need for "onPrivetPrintFailed" global JS
function.

[rbpotter, 2017/06/14 03:55:08]

Was originally thinking it might be temporary and we could change
the print message to cr.sendWithPromise, then reject that promise
here if it failed. However I looked into this more and found that local
printing never actually sends a message back to JS except for in the
case of Extension/Privet print failures, which both call the global
"onPrivetPrintFailed" function. This is because the dialog gets closed
if the print job is successful. So I am not sure if it makes sense to
change print to sendWithPromise, since the dialog would be closed
either before the promise resolves or at around the same time,
depending on the type of printer.

[dpapad, 2017/06/14 17:19:36]

Ok. Can we then eliminate (in a follow up CL) the global onPrivetPrintFailed
function using FireWebUIListener() as needed?

[rbpotter, 2017/06/14 18:28:25]

Ok, will do.

[rbpotter, 2017/06/14 19:19:55]

See followup here: https://codereview.chromium.org/2935303003/

+      // This was an attempt to print to a privet printer and has failed.
+      base::Value http_code_value(-1);
+      web_ui()->CallJavascriptFunctionUnsafe("onPrivetPrintFailed",
+                                             http_code_value);
+    } else {  // Capabilities update failed
+      RejectJavascriptCallback(base::Value(callback_id),
+                               base::Value(privet_http_resolution_->GetName()));
+    }
     privet_http_resolution_.reset();
     return false;
   }
@@ -1604,7 +1636,7 @@ void PrintPreviewHandler::PrivetLocalPrintUpdateClient(
     std::string capabilities,
     gfx::Size page_size,
     std::unique_ptr<cloud_print::PrivetHTTPClient> http_client) {
-  if (!PrivetUpdateClient(std::move(http_client)))
+  if (!PrivetUpdateClient("", std::move(http_client)))
     return;
 
   StartPrivetLocalPrint(print_ticket, capabilities, page_size);
@@ -1645,14 +1677,14 @@ void PrintPreviewHandler::StartPrivetLocalPrint(const std::string& print_ticket,
   privet_local_print_operation_->Start();
 }
 
-
 void PrintPreviewHandler::OnPrivetCapabilities(
+    const std::string& callback_id,
     const base::DictionaryValue* capabilities) {
   std::string name = privet_capabilities_operation_->GetHTTPClient()->GetName();
 
   if (!capabilities || capabilities->HasKey(cloud_print::kPrivetKeyError) ||
       !printer_lister_) {
-    SendPrivetCapabilitiesError(name);
+    RejectJavascriptCallback(base::Value(callback_id), base::Value(name));
     return;
   }
 
@@ -1661,33 +1693,36 @@ void PrintPreviewHandler::OnPrivetCapabilities(
       printer_lister_->GetDeviceDescription(name);
 
   if (!description) {
-    SendPrivetCapabilitiesError(name);
+    RejectJavascriptCallback(base::Value(callback_id), base::Value(name));
     return;
   }
 
   FillPrinterDescription(name, *description, true, &printer_info);
 
-  web_ui()->CallJavascriptFunctionUnsafe("onPrivetCapabilitiesSet",
-                                         printer_info, *capabilities);
+  base::DictionaryValue printer_info_and_caps;
+  printer_info_and_caps.SetDictionary("printer",
+                                      base::WrapUnique(&printer_info));
+  base::DictionaryValue capabilities_copy = *capabilities;
+  printer_info_and_caps.SetDictionary("capabilities",
+                                      base::WrapUnique(&capabilities_copy));
+  ResolveJavascriptCallback(base::Value(callback_id), printer_info_and_caps);
 
   privet_capabilities_operation_.reset();
 }
 
-void PrintPreviewHandler::SendPrivetCapabilitiesError(
-    const std::string& device_name) {
-  base::Value name_value(device_name);
-  web_ui()->CallJavascriptFunctionUnsafe("failedToGetPrivetPrinterCapabilities",
-                                         name_value);
-}
-
 void PrintPreviewHandler::PrintToPrivetPrinter(const std::string& device_name,
                                                const std::string& ticket,
                                                const std::string& capabilities,
                                                const gfx::Size& page_size) {
-  CreatePrivetHTTP(
-      device_name,
-      base::Bind(&PrintPreviewHandler::PrivetLocalPrintUpdateClient,
-                 weak_factory_.GetWeakPtr(), ticket, capabilities, page_size));
+  if (!CreatePrivetHTTP(
+          device_name,
+          base::Bind(&PrintPreviewHandler::PrivetLocalPrintUpdateClient,
+                     weak_factory_.GetWeakPtr(), ticket, capabilities,
+                     page_size))) {
+    base::Value http_code_value(-1);
+    web_ui()->CallJavascriptFunctionUnsafe("onPrivetPrintFailed",
+                                           http_code_value);
+  }
 }
 
 bool PrintPreviewHandler::CreatePrivetHTTP(
@@ -1697,10 +1732,8 @@ bool PrintPreviewHandler::CreatePrivetHTTP(
   const cloud_print::DeviceDescription* device_description =
       printer_lister_ ? printer_lister_->GetDeviceDescription(name) : NULL;
 
-  if (!device_description) {
-    SendPrivetCapabilitiesError(name);
+  if (!device_description)
     return false;
-  }
 
   privet_http_factory_ =
       cloud_print::PrivetHTTPAsynchronousFactory::CreateInstance(
@@ -1776,16 +1809,20 @@ void PrintPreviewHandler::OnGotExtensionPrinterInfo(
 }
 
 void PrintPreviewHandler::OnGotExtensionPrinterCapabilities(
+    const std::string& callback_id,
     const std::string& printer_id,
     const base::DictionaryValue& capabilities) {
   if (capabilities.empty()) {
-    web_ui()->CallJavascriptFunctionUnsafe(
-        "failedToGetExtensionPrinterCapabilities", base::Value(printer_id));
+    RejectJavascriptCallback(base::Value(callback_id), base::Value(printer_id));

[dpapad, 2017/06/09 17:59:11]

Does it still make sense to pass the printer_id in the response? I was thinking
maybe not, since there is a direct association of request and response via the
Promise anyway.

[rbpotter, 2017/06/14 03:55:08]

Done.

     return;
   }
-
-  web_ui()->CallJavascriptFunctionUnsafe("onExtensionCapabilitiesSet",
-                                         base::Value(printer_id), capabilities);
+  base::DictionaryValue printer_id_and_capabilities;
+  printer_id_and_capabilities.SetString("printerId", printer_id);
+  base::DictionaryValue capabilities_copy = capabilities;
+  printer_id_and_capabilities.SetDictionary(
+      "capabilities", base::WrapUnique(&capabilities_copy));
+  ResolveJavascriptCallback(base::Value(callback_id),
+                            printer_id_and_capabilities);
 }
 
 void PrintPreviewHandler::OnExtensionPrintResult(bool success,