Print Preview: Change getPrinterCapabilities to cr.sendWithPromise

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

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/webui/print_preview/print_preview_handler.h"
 
 #include <ctype.h>
 #include <stddef.h>
 
 #include <map>
@@ skipping 688 matching lines @@
   privet_lister_timer_.reset();
   if (PrivetPrintingEnabled() && printer_lister_) {
     printer_lister_->Stop();
   }
   ResolveJavascriptCallback(base::Value(privet_callback_id_), base::Value());
 #endif
 }
 
 void PrintPreviewHandler::HandleGetPrivetPrinterCapabilities(
     const base::ListValue* args) {
+  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;
+  }
 #if BUILDFLAG(ENABLE_SERVICE_DISCOVERY)
-  std::string name;
-  bool success = args->GetString(0, &name);
-  DCHECK(success);
-
-  CreatePrivetHTTP(
-      name, base::Bind(&PrintPreviewHandler::PrivetCapabilitiesUpdateClient,
-                       weak_factory_.GetWeakPtr()));
+  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(
     const base::ListValue* args) {
   std::string callback_id;
   CHECK(args->GetString(0, &callback_id));
   CHECK(!callback_id.empty());
 
   AllowJavascript();
   EnsureExtensionPrinterHandlerSet();
@@ skipping 12 matching lines @@
   DCHECK(ok);
 
   EnsureExtensionPrinterHandlerSet();
   extension_printer_handler_->StartGrantPrinterAccess(
       printer_id, base::Bind(&PrintPreviewHandler::OnGotExtensionPrinterInfo,
                              weak_factory_.GetWeakPtr(), printer_id));
 }
 
 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) {
   DCHECK_EQ(2U, args->GetSize());
   std::unique_ptr<base::DictionaryValue> settings = GetSettingsDictionary(args);
   if (!settings)
     return;
   int request_id = -1;
   if (!settings->GetInteger(printing::kPreviewRequestID, &request_id))
     return;
@@ skipping 295 matching lines @@
   std::string data_to_save;
   printing::StickySettings* sticky_settings = GetStickySettings();
   if (args->GetString(0, &data_to_save) && !data_to_save.empty())
     sticky_settings->StoreAppState(data_to_save);
   sticky_settings->SaveInPrefs(Profile::FromBrowserContext(
       preview_web_contents()->GetBrowserContext())->GetPrefs());
 }
 
 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>();
     printer_info->SetString(printing::kPrinterId, printer_name);
     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);
 }
 
 // |args| is expected to contain a string with representing the callback id
 // followed by a list of arguments the first of which should be the printer id.
 void PrintPreviewHandler::HandlePrinterSetup(const base::ListValue* args) {
   AllowJavascript();
 
@@ skipping 198 matching lines @@
 }
 
 void PrintPreviewHandler::SendAccessToken(const std::string& type,
                                           const std::string& access_token) {
   VLOG(1) << "Get getAccessToken finished";
   web_ui()->CallJavascriptFunctionUnsafe(
       "onDidGetAccessToken", base::Value(type), base::Value(access_token));
 }
 
 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(
     const std::string& callback_id,
     const std::string& printer_name,
     std::unique_ptr<base::DictionaryValue> destination_info) {
   auto response = base::MakeUnique<base::DictionaryValue>();
   bool success = true;
   auto caps_value = base::MakeUnique<base::Value>();
   auto caps = base::MakeUnique<base::DictionaryValue>();
@@ skipping 230 matching lines @@
   }
 }
 
 void PrintPreviewHandler::LocalPrinterRemoved(const std::string& name) {
 }
 
 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;
   }
 
   privet_local_print_operation_.reset();
   privet_capabilities_operation_.reset();
   privet_http_client_ = cloud_print::PrivetV1HTTPClient::CreateDefault(
       std::move(http_client));
 
   privet_http_resolution_.reset();
 
   return true;
 }
 
 void PrintPreviewHandler::PrivetLocalPrintUpdateClient(
     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("", std::move(http_client)))
     return;
 
   StartPrivetLocalPrint(print_ticket, capabilities, page_size);
 }
 
 void PrintPreviewHandler::StartPrivetLocalPrint(const std::string& print_ticket,
                                                 const std::string& capabilities,
                                                 const gfx::Size& page_size) {
   privet_local_print_operation_ =
       privet_http_client_->CreateLocalPrintOperation(this);
@@ skipping 20 matching lines @@
       SigninManagerFactory::GetForProfileIfExists(profile);
 
   if (signin_manager) {
     privet_local_print_operation_->SetUsername(
         signin_manager->GetAuthenticatedAccountInfo().email);
   }
 
   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;
   }
 
   base::DictionaryValue printer_info;
   const cloud_print::DeviceDescription* description =
       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(
     const std::string& name,
     const cloud_print::PrivetHTTPAsynchronousFactory::ResultCallback&
         callback) {
   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(
           Profile::FromWebUI(web_ui())->GetRequestContext());
   privet_http_resolution_ = privet_http_factory_->CreatePrivetHTTP(name);
   privet_http_resolution_->Start(device_description->address, callback);
 
   return true;
 }
 
@@ skipping 55 matching lines @@
     web_ui()->CallJavascriptFunctionUnsafe("failedToResolveProvisionalPrinter",
                                            base::Value(printer_id));
     return;
   }
 
   web_ui()->CallJavascriptFunctionUnsafe("onProvisionalPrinterResolved",
                                          base::Value(printer_id), printer_info);
 }
 
 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,
                                                  const std::string& status) {
   if (success) {
     ClosePreviewDialog();
     return;
   }
 
   // TODO(tbarzic): This function works for extension printers case too, but it
@@ skipping 16 matching lines @@
 
 void PrintPreviewHandler::UnregisterForGaiaCookieChanges() {
   if (gaia_cookie_manager_service_)
     gaia_cookie_manager_service_->RemoveObserver(this);
 }
 
 void PrintPreviewHandler::SetPdfSavedClosureForTesting(
     const base::Closure& closure) {
   pdf_file_saved_closure_ = closure;
 }