Completely rewrite the PpdProvider/PpdCache to use the SCS backend. Along the way, clean it up a l…

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

Index: chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
diff --git a/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc b/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
index 0a1b1abcf30b61135dca8a8ab34c64c36e5b9aa2..7a80da2c1b684ba964bc0c44a9fe5f4757643619 100644
--- a/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
+++ b/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
+#include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
@@ -197,6 +198,7 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
   CHECK(printer_dict->GetString("printerProtocol", &printer_protocol));
   // printerQueue might be null for a printer whose protocol is not 'LPD'.
   printer_dict->GetString("printerQueue", &printer_queue);
+
   // printerPPDPath might be null for an auto-discovered printer.
   printer_dict->GetString("printerPPDPath", &printer_ppd_path);
   std::string printer_uri =
@@ -209,36 +211,46 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
   printer->set_model(printer_model);
   printer->set_uri(printer_uri);
   if (!printer_ppd_path.empty()) {
-    printer->mutable_ppd_reference()->user_supplied_ppd_url = printer_ppd_path;
-    if (!ppd_provider_->CachePpd(printer->ppd_reference(),
-                                 base::FilePath(printer_ppd_path))) {
-      LOG(WARNING) << "PPD could not be stored in the cache";
+    printer->mutable_ppd_reference()->user_supplied_ppd_url =
+        "file:///" + printer_ppd_path;

[michaelpg, 2017/02/03 01:57:00]

Can you use a util function for this, e.g. net::FilePathToFileURL?

[Carlson, 2017/02/03 19:32:19]

Done.

+  } else if (!printer_manufacturer.empty() && !printer_model.empty()) {
+    // Using the manufacturer and model, get a ppd reference.
+    if (!ppd_provider_->GetPpdReference(printer_manufacturer, printer_model,
+                                        printer->mutable_ppd_reference())) {
+      LOG(ERROR) << "Failed to get ppd reference!";

[michaelpg, 2017/02/03 01:57:00]

nit: the exclamation point is a bit dramatic ;-)

[Carlson, 2017/02/03 19:32:19]

Done.

       OnAddPrinterError();
       return;
     }
-  } else if (!printer_manufacturer.empty() && !printer_model.empty()) {
-    Printer::PpdReference* ppd = printer->mutable_ppd_reference();
-    ppd->effective_manufacturer = printer_manufacturer;
-    ppd->effective_model = printer_model;
   }
 
   if (printer->IsIppEverywhere()) {
-    AddPrinterToCups(std::move(printer), base::FilePath(), true);
-    return;
+    std::string printer_id = printer->id();
+    std::string printer_uri = printer->uri();
+
+    chromeos::DebugDaemonClient* client =
+        chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
+
+    client->CupsAddAutoConfiguredPrinter(
+        printer_id, printer_uri,
+        base::Bind(&CupsPrintersHandler::OnAddedPrinter,
+                   weak_factory_.GetWeakPtr(), base::Passed(&printer)),
+        base::Bind(&CupsPrintersHandler::OnAddPrinterError,
+                   weak_factory_.GetWeakPtr()));
+  } else {
+    // We need to save a reference to members of printer since we transfer
+    // ownership in the bind call.
+    const Printer::PpdReference& ppd_reference = printer->ppd_reference();

[michaelpg, 2017/02/03 01:57:00]

is the Printer at |printer| guaranteed to outlive this reference? (even if so,
that's not obvious. I don't see the need to avoid a copy here.)

[Carlson, 2017/02/03 19:32:19]

It should be safe, but sure, done.  The conventions around Passed and Bind in
the Chromium code are all a mess IMHO, and you're right, making a copy makes
this much more obviously safe.

+    ppd_provider_->ResolvePpd(
+        ppd_reference,
+        base::Bind(&CupsPrintersHandler::ResolvePpdDone,
+                   weak_factory_.GetWeakPtr(), base::Passed(&printer)));
   }
-
-  // We need to save a reference to members of printer since we transfer
-  // ownership in the bind call.
-  const Printer::PpdReference& ppd_reference = printer->ppd_reference();
-  ppd_provider_->Resolve(
-      ppd_reference,
-      base::Bind(&CupsPrintersHandler::OnPPDResolved,
-                 weak_factory_.GetWeakPtr(), base::Passed(&printer)));
 }
 
 void CupsPrintersHandler::OnAddedPrinter(std::unique_ptr<Printer> printer,
-                                         bool success) {
+                                         int32_t result_code) {
   std::string printer_name = printer->display_name();
+  bool success = (result_code == 0);
   if (success) {
     PrinterPrefManagerFactory::GetForBrowserContext(profile_)->RegisterPrinter(
         std::move(printer));
@@ -256,44 +268,47 @@ void CupsPrintersHandler::OnAddPrinterError() {
 
 void CupsPrintersHandler::HandleGetCupsPrinterManufacturers(
     const base::ListValue* args) {
+  AllowJavascript();
   std::string js_callback;
   CHECK_EQ(1U, args->GetSize());
   CHECK(args->GetString(0, &js_callback));
-  ppd_provider_->QueryAvailable(
-      base::Bind(&CupsPrintersHandler::QueryAvailableManufacturersDone,
-                 weak_factory_.GetWeakPtr(),
-                 js_callback));
+  ppd_provider_->ResolveManufacturers(
+      base::Bind(&CupsPrintersHandler::ResolveManufacturersDone,
+                 weak_factory_.GetWeakPtr(), js_callback));
 }
 
 void CupsPrintersHandler::HandleGetCupsPrinterModels(
     const base::ListValue* args) {
+  AllowJavascript();
   std::string js_callback;
   std::string manufacturer;
   CHECK_EQ(2U, args->GetSize());
   CHECK(args->GetString(0, &js_callback));
   CHECK(args->GetString(1, &manufacturer));
-  // Special case the "asked with no manufacturer case" since the UI sometimes
-  // triggers this and it should yield a trivial (empty) result
+
+  // Empty manufacturer queries may be triggered as a part of the ui
+  // initialization, and should just return empty results.
   if (manufacturer.empty()) {
-    base::SequencedTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&CupsPrintersHandler::QueryAvailableModelsDone,
-                   weak_factory_.GetWeakPtr(), js_callback, manufacturer,
-                   chromeos::printing::PpdProvider::SUCCESS,
-                   chromeos::printing::PpdProvider::AvailablePrintersMap()));
-  } else {
-    ppd_provider_->QueryAvailable(
-        base::Bind(&CupsPrintersHandler::QueryAvailableModelsDone,
-                   weak_factory_.GetWeakPtr(), js_callback, manufacturer));
+    base::DictionaryValue response;
+    response.SetBoolean("success", true);
+    response.Set("models", base::MakeUnique<base::ListValue>());
+    ResolveJavascriptCallback(base::StringValue(js_callback), response);
+    return;
   }
+
+  ppd_provider_->ResolvePrinters(
+      manufacturer, base::Bind(&CupsPrintersHandler::ResolvePrintersDone,
+                               weak_factory_.GetWeakPtr(), js_callback));
 }
 
 void CupsPrintersHandler::HandleSelectPPDFile(const base::ListValue* args) {
   CHECK_EQ(1U, args->GetSize());
   CHECK(args->GetString(0, &webui_callback_id_));
 
-  base::FilePath downloads_path = DownloadPrefs::FromDownloadManager(
-      content::BrowserContext::GetDownloadManager(profile_))->DownloadPath();
+  base::FilePath downloads_path =
+      DownloadPrefs::FromDownloadManager(
+          content::BrowserContext::GetDownloadManager(profile_))
+          ->DownloadPath();
 
   select_file_dialog_ = ui::SelectFileDialog::Create(
       this, new ChromeSelectFilePolicy(web_ui()->GetWebContents()));
@@ -306,40 +321,33 @@ void CupsPrintersHandler::HandleSelectPPDFile(const base::ListValue* args) {
       nullptr, 0, FILE_PATH_LITERAL(""), owning_window, nullptr);
 }
 
-void CupsPrintersHandler::QueryAvailableManufacturersDone(
+void CupsPrintersHandler::ResolveManufacturersDone(
     const std::string& js_callback,
     chromeos::printing::PpdProvider::CallbackResultCode result_code,
-    const chromeos::printing::PpdProvider::AvailablePrintersMap& available) {
-  auto manufacturers = base::MakeUnique<base::ListValue>();
+    const std::vector<std::string>& manufacturers) {
+  auto manufacturers_value = base::MakeUnique<base::ListValue>();
   if (result_code == chromeos::printing::PpdProvider::SUCCESS) {

[michaelpg, 2017/02/03 01:57:00]

nit: remove {} for single-line ifs

[Carlson, 2017/02/03 19:32:19]

As I read style guide, this is left to the author, and I'm in the "always do
braces because someday you will add something to the if and forget to add braces
and be confused" camp.

[michaelpg, 2017/02/03 22:02:53]

whoops, I was thinking of the Chromium JS style guide.

-    for (const auto& entry : available) {
-      manufacturers->AppendString(entry.first);
-    }
+    manufacturers_value->AppendStrings(manufacturers);
   }
   base::DictionaryValue response;
   response.SetBoolean("success",
                       result_code == chromeos::printing::PpdProvider::SUCCESS);
-  response.Set("manufacturers", std::move(manufacturers));
+  response.Set("manufacturers", std::move(manufacturers_value));
   ResolveJavascriptCallback(base::StringValue(js_callback), response);
 }
 
-void CupsPrintersHandler::QueryAvailableModelsDone(
+void CupsPrintersHandler::ResolvePrintersDone(
     const std::string& js_callback,
-    const std::string& manufacturer,
     chromeos::printing::PpdProvider::CallbackResultCode result_code,
-    const chromeos::printing::PpdProvider::AvailablePrintersMap& available) {
-  auto models = base::MakeUnique<base::ListValue>();
+    const std::vector<std::string>& printers) {
+  auto printers_value = base::MakeUnique<base::ListValue>();
   if (result_code == chromeos::printing::PpdProvider::SUCCESS) {

[michaelpg, 2017/02/03 01:57:00]

ditto

[Carlson, 2017/02/03 19:32:19]

Acknowledged.

-    const auto it = available.find(manufacturer);
-    if (it != available.end()) {
-      // Everything looks good.
-      models->AppendStrings(it->second);
-    }
+    printers_value->AppendStrings(printers);
   }
   base::DictionaryValue response;
   response.SetBoolean("success",
-                       result_code == chromeos::printing::PpdProvider::SUCCESS);
-  response.Set("models", std::move(models));
+                      result_code == chromeos::printing::PpdProvider::SUCCESS);
+  response.Set("models", std::move(printers_value));
   ResolveJavascriptCallback(base::StringValue(js_callback), response);
 }
 
@@ -392,34 +400,30 @@ void CupsPrintersHandler::OnDiscoveryDone() {
                          base::StringValue("on-printer-discovery-done"));
 }
 
-void CupsPrintersHandler::AddPrinterToCups(std::unique_ptr<Printer> printer,
-                                           const base::FilePath& ppd_path,
-                                           bool ipp_everywhere) {
+void CupsPrintersHandler::ResolvePpdDone(
+    std::unique_ptr<Printer> printer,
+    printing::PpdProvider::CallbackResultCode result,
+    const std::string& ppd_contents) {
+  if (result != printing::PpdProvider::SUCCESS) {
+    // TODO(skau): Add appropriate failure modes crbug.com/670068.
+    LOG(ERROR) << "Error resolving";
+    OnAddPrinterError();
+    return;
+  }
+
   std::string printer_id = printer->id();
   std::string printer_uri = printer->uri();
 
   chromeos::DebugDaemonClient* client =
       chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
-  client->CupsAddPrinter(
-      printer_id, printer_uri, ppd_path.value(), ipp_everywhere,
+
+  client->CupsAddManuallyConfiguredPrinter(
+      printer_id, printer_uri, ppd_contents,
       base::Bind(&CupsPrintersHandler::OnAddedPrinter,
                  weak_factory_.GetWeakPtr(), base::Passed(&printer)),
       base::Bind(&CupsPrintersHandler::OnAddPrinterError,
                  weak_factory_.GetWeakPtr()));
 }
 
-void CupsPrintersHandler::OnPPDResolved(
-    std::unique_ptr<Printer> printer,
-    printing::PpdProvider::CallbackResultCode result,
-    base::FilePath path) {
-  if (result != printing::PpdProvider::SUCCESS) {
-    // TODO(skau): Add appropriate failure modes crbug.com/670068.
-    OnAddPrinterError();
-    return;
-  }
-
-  AddPrinterToCups(std::move(printer), path, false /* never ipp everywhere */);
-}
-
 }  // namespace settings
 }  // namespace chromeos