Change PpdProvider API to make it discovery friendly.

This changes two main things.  First, and most significantly, the
way we resolve a PpdReference is unified -- instead of trying
a make-and-model string or trying usb ids, now we have a single
function to which you supply everything you know about the printer
and it gives you back a PpdReference or a failure.

Second, the way we support UI make, model listings changes.  When
the user asks for a list of supported printers from a manufacturer,
instead of getting back just the names, now the api returns the
names paired with the corresponding PpdReference.  Before, the
user had to call back into the ppd provider to get the reference, which
meant we had a weird "have to call this before that" sort of constraint
which this removes.

All callsites and tests updated as well.

Bug: 744996
Change-Id: Ied385c087e4c3d6c1b21b924fcea772c786074c5
Reviewed-on: https://chromium-review.googlesource.com/580007
Commit-Queue: Justin Carlson <justincarlson@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488716}

chrome/browser/chromeos/printing/usb_printer_detector.cc

@@ -210,8 +210,11 @@ class UsbPrinterDetectorImpl : public UsbPrinterDetector,
 
     // Look for an exact match based on USB ids.
     scoped_refptr<PpdProvider> ppd_provider = CreatePpdProvider(profile_);
-    ppd_provider->ResolveUsbIds(
-        device->vendor_id(), device->product_id(),
+    PpdProvider::PrinterSearchData printer_search_data;
+    printer_search_data.usb_vendor_id = device->vendor_id();
+    printer_search_data.usb_product_id = device->product_id();
+    ppd_provider->ResolvePpdReference(
+        printer_search_data,
         base::Bind(&UsbPrinterDetectorImpl::ResolveUsbIdsDone,
                    weak_ptr_factory_.GetWeakPtr(), ppd_provider,
                    base::Passed(std::move(data))));
@@ -235,12 +238,11 @@ class UsbPrinterDetectorImpl : public UsbPrinterDetector,
   void ResolveUsbIdsDone(scoped_refptr<PpdProvider> provider,
                          std::unique_ptr<SetUpPrinterData> data,
                          PpdProvider::CallbackResultCode result,
-                         const std::string& effective_make_and_model) {
+                         const Printer::PpdReference& ppd_reference) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     if (result == PpdProvider::SUCCESS) {
       // Got something based on usb ids.  Go with it.
-      data->printer->mutable_ppd_reference()->effective_make_and_model =
-          effective_make_and_model;
+      *data->printer->mutable_ppd_reference() = ppd_reference;
     } else {
       // Couldn't figure this printer out based on usb ids, fall back to
       // guessing the make/model string from what the USB system reports.

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

@@ -384,9 +384,17 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
     printer.mutable_ppd_reference()->user_supplied_ppd_url = tmp.spec();
   } else if (!ppd_manufacturer.empty() && !ppd_model.empty()) {
     RecordPpdSource(kScs);
-    // Using the manufacturer and model, get a ppd reference.
-    if (!ppd_provider_->GetPpdReference(ppd_manufacturer, ppd_model,
-                                        printer.mutable_ppd_reference())) {
+    // Pull out the ppd reference associated with the selected manufacturer and
+    // model.
+    bool found = false;
+    for (const auto& resolved_printer : resolved_printers_[ppd_manufacturer]) {
+      if (resolved_printer.first == ppd_model) {
+        *printer.mutable_ppd_reference() = resolved_printer.second;
+        found = true;
+        break;
+      }
+    }
+    if (!found) {
       LOG(ERROR) << "Failed to get ppd reference";
       OnAddPrinterError();
       return;
@@ -490,8 +498,9 @@ void CupsPrintersHandler::HandleGetCupsPrinterModels(
   }
 
   ppd_provider_->ResolvePrinters(
-      manufacturer, base::Bind(&CupsPrintersHandler::ResolvePrintersDone,
-                               weak_factory_.GetWeakPtr(), js_callback));
+      manufacturer,
+      base::Bind(&CupsPrintersHandler::ResolvePrintersDone,
+                 weak_factory_.GetWeakPtr(), manufacturer, js_callback));
 }
 
 void CupsPrintersHandler::HandleSelectPPDFile(const base::ListValue* args) {
@@ -529,12 +538,16 @@ void CupsPrintersHandler::ResolveManufacturersDone(
 }
 
 void CupsPrintersHandler::ResolvePrintersDone(
+    const std::string& manufacturer,
     const std::string& js_callback,
     PpdProvider::CallbackResultCode result_code,
-    const std::vector<std::string>& printers) {
+    const PpdProvider::ResolvedPrintersList& printers) {
   auto printers_value = base::MakeUnique<base::ListValue>();
   if (result_code == PpdProvider::SUCCESS) {
-    printers_value->AppendStrings(printers);
+    resolved_printers_[manufacturer] = printers;
+    for (const auto& printer : printers) {
+      printers_value->AppendString(printer.first);
+    }
   }
   base::DictionaryValue response;
   response.SetBoolean("success", result_code == PpdProvider::SUCCESS);

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

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_CUPS_PRINTERS_HANDLER_H_
 #define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_CUPS_PRINTERS_HANDLER_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -90,9 +91,10 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
   void ResolveManufacturersDone(const std::string& js_callback,
                                 PpdProvider::CallbackResultCode result_code,
                                 const std::vector<std::string>& available);
-  void ResolvePrintersDone(const std::string& js_callback,
+  void ResolvePrintersDone(const std::string& manufacturer,
+                           const std::string& js_callback,
                            PpdProvider::CallbackResultCode result_code,
-                           const std::vector<std::string>& available);
+                           const PpdProvider::ResolvedPrintersList& printers);
 
   // ui::SelectFileDialog::Listener override:
   void FileSelected(const base::FilePath& path,
@@ -119,6 +121,10 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
   scoped_refptr<PpdProvider> ppd_provider_;
   std::unique_ptr<PrinterConfigurer> printer_configurer_;
 
+  // Cached list of {printer name, PpdReference} pairs for each manufacturer
+  // that has been resolved in the lifetime of this object.
+  std::map<std::string, PpdProvider::ResolvedPrintersList> resolved_printers_;
+
   Profile* profile_;
   scoped_refptr<ui::SelectFileDialog> select_file_dialog_;
   std::string webui_callback_id_;

chromeos/printing/ppd_provider.h

@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/callback.h"
@@ -55,6 +56,25 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
     std::string ppd_server_root = "https://www.gstatic.com/chromeos_printing";
   };
 
+  // Everything we might know about a printer when looking for a
+  // driver for it.  All of the default values for fields in this struct
+  // mean we *don't* have that piece of information.
+  //
+  // Fields are listed in search order preference -- we use earlier
+  // fields first to attempt to find a match.
+  struct PrinterSearchData {
+    PrinterSearchData();
+    PrinterSearchData(const PrinterSearchData& other);
+    ~PrinterSearchData();
+
+    // Make-and-model string guesses.
+    std::vector<std::string> make_and_model;
+
+    // 16-bit usb identifiers.
+    int usb_vendor_id = 0;
+    int usb_product_id = 0;
+  };
+
   // Result of a ResolvePpd() call.
   // If the result code is SUCCESS, then:
   //    string holds the contents of a PPD (that may or may not be gzipped).
@@ -71,17 +91,25 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
   using ResolveManufacturersCallback =
       base::Callback<void(CallbackResultCode, const std::vector<std::string>&)>;
 
+  // A list of printer names paired with the PpdReference that should be used
+  // for that printer.
+  using ResolvedPrintersList =
+      std::vector<std::pair<std::string, Printer::PpdReference>>;
+
   // Result of a ResolvePrinters() call.  If the result code is SUCCESS, then
-  // the vector contains a sorted list of all printer models from the given
-  // manufacturer for which we have a driver.
+  // the vector contains a sorted list <model_name, PpdReference> tuples of all
+  // printer models from the given manufacturer for which we have a driver,
+  // sorted by model_name.
   using ResolvePrintersCallback =
-      base::Callback<void(CallbackResultCode, const std::vector<std::string>&)>;
+      base::Callback<void(CallbackResultCode, const ResolvedPrintersList&)>;
 
-  // Result of a ResolveUsbIds call.  If the result code is SUCCESS, then the
-  // second argument contains the effective make and model of the printer.
-  // NOT_FOUND means we don't know about this Usb device.
-  using ResolveUsbIdsCallback =
-      base::Callback<void(CallbackResultCode, const std::string&)>;
+  // Result of a ResolvePpdReference call.  If the result code is
+  // SUCCESS, then the second argument contains the a PpdReference
+  // that we have high confidence can be used to obtain a driver for
+  // the printer.  NOT_FOUND means we couldn't confidently figure out
+  // a driver for the printer.
+  using ResolvePpdReferenceCallback =
+      base::Callback<void(CallbackResultCode, const Printer::PpdReference&)>;
 
   // Create and return a new PpdProvider with the given cache and options.
   // A references to |url_context_getter| is taken.
@@ -97,33 +125,20 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
   // |cb| will be called on the invoking thread, and will be sequenced.
   virtual void ResolveManufacturers(const ResolveManufacturersCallback& cb) = 0;
 
-  // Get all models from a given manufacturer, localized in the default browser
-  // locale or the closest available fallback.  |manufacturer| must be a value
-  // returned from a successful ResolveManufacturers() call performed from this
-  // PpdProvider instance.
+  // Get all models from a given manufacturer, localized in the
+  // default browser locale or the closest available fallback.
+  // |manufacturer| must be a value returned from a successful
+  // ResolveManufacturers() call performed from this PpdProvider
+  // instance.
   //
   // |cb| will be called on the invoking thread, and will be sequenced.
   virtual void ResolvePrinters(const std::string& manufacturer,
                                const ResolvePrintersCallback& cb) = 0;
 
-  // Given a usb vendor/device id, attempt to get an effective make and model
-  // string for the given printer.
-  virtual void ResolveUsbIds(int vendor_id,
-                             int device_id,
-                             const ResolveUsbIdsCallback& cb) = 0;
-
-  // Given a |manufacturer| from ResolveManufacturers() and a |printer| from
-  // a ResolvePrinters() call for that manufacturer, fill in |reference|
-  // with the information needed to resolve the Ppd for this printer.  Returns
-  // true on success and overwrites the contents of |reference|.  On failure,
-  // |reference| is unchanged.
-  //
-  // Note that, unlike the other functions in this class, |reference| can be
-  // saved and given to ResolvePpd() in a different PpdProvider instance without
-  // first resolving manufactuerers or printers.
-  virtual bool GetPpdReference(const std::string& manufacturer,
-                               const std::string& printer,
-                               Printer::PpdReference* reference) const = 0;
+  // Attempt to find a PpdReference for the given printer.  You should supply
+  // as much information in search_data as you can.
+  virtual void ResolvePpdReference(const PrinterSearchData& search_data,
+                                   const ResolvePpdReferenceCallback& cb) = 0;
 
   // Given a PpdReference, attempt to get the PPD for printing.
   //

chromeos/printing/ppd_provider_unittest.cc

@@ -21,6 +21,7 @@
 #include "chromeos/chromeos_paths.h"
 #include "chromeos/printing/ppd_cache.h"
 #include "chromeos/printing/ppd_provider.h"
+#include "chromeos/printing/printer_configuration.h"
 #include "net/url_request/test_url_request_interceptor.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -167,7 +168,7 @@ class PpdProviderTest : public ::testing::Test {
 
   // Capture the result of a ResolvePrinters() call.
   void CaptureResolvePrinters(PpdProvider::CallbackResultCode code,
-                              const std::vector<std::string>& data) {
+                              const PpdProvider::ResolvedPrintersList& data) {
     captured_resolve_printers_.push_back({code, data});
   }
 
@@ -183,9 +184,9 @@ class PpdProviderTest : public ::testing::Test {
   }
 
   // Capture the result of a ResolveUsbIds() call.
-  void CaptureResolveUsbIds(PpdProvider::CallbackResultCode code,
-                            const std::string& contents) {
-    captured_resolve_usb_ids_.push_back({code, contents});
+  void CaptureResolvePpdReference(PpdProvider::CallbackResultCode code,
+                                  const Printer::PpdReference& ref) {
+    captured_resolve_ppd_references_.push_back({code, ref});
   }
 
   // Discard the result of a ResolvePpd() call.
@@ -222,8 +223,8 @@ class PpdProviderTest : public ::testing::Test {
       std::pair<PpdProvider::CallbackResultCode, std::vector<std::string>>>
       captured_resolve_manufacturers_;
 
-  std::vector<
-      std::pair<PpdProvider::CallbackResultCode, std::vector<std::string>>>
+  std::vector<std::pair<PpdProvider::CallbackResultCode,
+                        PpdProvider::ResolvedPrintersList>>
       captured_resolve_printers_;
 
   struct CapturedResolvePpdResults {
@@ -233,8 +234,8 @@ class PpdProviderTest : public ::testing::Test {
   };
   std::vector<CapturedResolvePpdResults> captured_resolve_ppd_;
 
-  std::vector<std::pair<PpdProvider::CallbackResultCode, std::string>>
-      captured_resolve_usb_ids_;
+  std::vector<std::pair<PpdProvider::CallbackResultCode, Printer::PpdReference>>
+      captured_resolve_ppd_references_;
 
   std::unique_ptr<net::TestURLRequestInterceptor> interceptor_;
 
@@ -303,39 +304,49 @@ TEST_F(PpdProviderTest, UsbResolution) {
   StartFakePpdServer();
   auto provider = CreateProvider("en");
 
+  PpdProvider::PrinterSearchData search_data;
+
   // Should get back "Some canonical reference"
-  provider->ResolveUsbIds(0x031f, 1592,
-                          base::Bind(&PpdProviderTest::CaptureResolveUsbIds,
-                                     base::Unretained(this)));
+  search_data.usb_vendor_id = 0x031f;
+  search_data.usb_product_id = 1592;
+  provider->ResolvePpdReference(
+      search_data, base::Bind(&PpdProviderTest::CaptureResolvePpdReference,
+                              base::Unretained(this)));
   // Should get back "Some other canonical reference"
-  provider->ResolveUsbIds(0x031f, 6535,
-                          base::Bind(&PpdProviderTest::CaptureResolveUsbIds,
-                                     base::Unretained(this)));
+  search_data.usb_vendor_id = 0x031f;
+  search_data.usb_product_id = 6535;
+  provider->ResolvePpdReference(
+      search_data, base::Bind(&PpdProviderTest::CaptureResolvePpdReference,
+                              base::Unretained(this)));
 
   // Extant vendor id, nonexistant device id, should get a NOT_FOUND
-  provider->ResolveUsbIds(0x031f, 8162,
-                          base::Bind(&PpdProviderTest::CaptureResolveUsbIds,
-                                     base::Unretained(this)));
+  search_data.usb_vendor_id = 0x031f;
+  search_data.usb_product_id = 8162;
+  provider->ResolvePpdReference(
+      search_data, base::Bind(&PpdProviderTest::CaptureResolvePpdReference,
+                              base::Unretained(this)));
 
   // Nonexistant vendor id, should get a NOT_FOUND in the real world, but
   // the URL interceptor we're using considers all nonexistant files to
   // be effectively CONNECTION REFUSED, so we just check for non-success
   // on this one.
-  provider->ResolveUsbIds(1234, 1782,
-                          base::Bind(&PpdProviderTest::CaptureResolveUsbIds,
-                                     base::Unretained(this)));
+  search_data.usb_vendor_id = 1234;
+  search_data.usb_product_id = 1782;
+  provider->ResolvePpdReference(
+      search_data, base::Bind(&PpdProviderTest::CaptureResolvePpdReference,
+                              base::Unretained(this)));
   scoped_task_environment_.RunUntilIdle();
 
-  ASSERT_EQ(captured_resolve_usb_ids_.size(), static_cast<size_t>(4));
-  EXPECT_EQ(captured_resolve_usb_ids_[0].first, PpdProvider::SUCCESS);
-  EXPECT_EQ(captured_resolve_usb_ids_[0].second, "Some canonical reference");
-  EXPECT_EQ(captured_resolve_usb_ids_[1].first, PpdProvider::SUCCESS);
-  EXPECT_EQ(captured_resolve_usb_ids_[1].second,
+  ASSERT_EQ(captured_resolve_ppd_references_.size(), static_cast<size_t>(4));
+  EXPECT_EQ(captured_resolve_ppd_references_[0].first, PpdProvider::SUCCESS);
+  EXPECT_EQ(captured_resolve_ppd_references_[0].second.effective_make_and_model,
+            "Some canonical reference");
+  EXPECT_EQ(captured_resolve_ppd_references_[1].first, PpdProvider::SUCCESS);
+  EXPECT_EQ(captured_resolve_ppd_references_[1].second.effective_make_and_model,
             "Some other canonical reference");
-  EXPECT_EQ(captured_resolve_usb_ids_[2].first, PpdProvider::NOT_FOUND);
-  EXPECT_EQ(captured_resolve_usb_ids_[2].second, "");
-  EXPECT_FALSE(captured_resolve_usb_ids_[3].first == PpdProvider::SUCCESS);
-  EXPECT_EQ(captured_resolve_usb_ids_[3].second, "");
+  EXPECT_EQ(captured_resolve_ppd_references_[2].first, PpdProvider::NOT_FOUND);
+  EXPECT_FALSE(captured_resolve_ppd_references_[3].first ==
+               PpdProvider::SUCCESS);
 }
 
 // For convenience a null ResolveManufacturers callback target.
@@ -365,16 +376,23 @@ TEST_F(PpdProviderTest, ResolvePrinters) {
   EXPECT_EQ(PpdProvider::SUCCESS, captured_resolve_printers_[0].first);
   EXPECT_EQ(PpdProvider::SUCCESS, captured_resolve_printers_[1].first);
   EXPECT_EQ(2UL, captured_resolve_printers_[0].second.size());
-  EXPECT_EQ(std::vector<std::string>({"printer_a", "printer_b"}),
-            captured_resolve_printers_[0].second);
-  EXPECT_EQ(std::vector<std::string>({"printer_c"}),
-            captured_resolve_printers_[1].second);
 
-  // We have manufacturers and models, we should be able to get a ppd out of
-  // this.
-  Printer::PpdReference ref;
-  ASSERT_TRUE(
-      provider->GetPpdReference("manufacturer_a_en", "printer_b", &ref));
+  // First capture should get back printer_a, and printer_b, with ppd
+  // reference effective make and models of printer_a_ref and printer_b_ref.
+  const auto& capture0 = captured_resolve_printers_[0].second;
+  ASSERT_EQ(2UL, capture0.size());
+  EXPECT_EQ("printer_a", capture0[0].first);
+  EXPECT_EQ("printer_a_ref", capture0[0].second.effective_make_and_model);
+
+  EXPECT_EQ("printer_b", capture0[1].first);
+  EXPECT_EQ("printer_b_ref", capture0[1].second.effective_make_and_model);
+
+  // Second capture should get back printer_c with effective make and model of
+  // printer_c_ref
+  const auto& capture1 = captured_resolve_printers_[1].second;
+  ASSERT_EQ(1UL, capture1.size());
+  EXPECT_EQ("printer_c", capture1[0].first);
+  EXPECT_EQ("printer_c_ref", capture1[0].second.effective_make_and_model);
 }
 
 // Test that if we give a bad reference to ResolvePrinters(), we get an