Rewrite the PpdProvider/PpdCache to use the SCS backend.

Along the way, clean it up a lot based on lessons learned from my first chromium code.

    Note this changes PpdReference to use just a single "effective_make_and_model"
    field to resolve ppds from the ppd server.  This closely mirrors what's usually available from IPP and other sources, so seems to be the most sensible key for doing ppd lookups.

    This also updates all callsites and cascading changes to make the provider changes work.

BUG=617253
R=skau
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2613683004
Cr-Commit-Position: refs/heads/master@{#448100}

chrome/browser/chromeos/printing/ppd_provider_factory.cc

@@ -18,15 +18,20 @@
 namespace chromeos {
 namespace printing {
 
-std::unique_ptr<PpdProvider> CreateProvider(Profile* profile) {
+scoped_refptr<PpdProvider> CreateProvider(Profile* profile) {
   base::FilePath ppd_cache_path =
       profile->GetPath().Append(FILE_PATH_LITERAL("PPDCache"));
 
-  return PpdProvider::Create(google_apis::GetAPIKey(),
-                             g_browser_process->system_request_context(),
+  auto cache = PpdCache::Create(ppd_cache_path,
+                                content::BrowserThread::GetTaskRunnerForThread(
+                                    content::BrowserThread::CACHE)
+                                    .get());
+
+  return PpdProvider::Create(g_browser_process->GetApplicationLocale(),
+                             g_browser_process->system_request_context(), cache,
                              content::BrowserThread::GetTaskRunnerForThread(
-                                 content::BrowserThread::FILE),
-                             PpdCache::Create(ppd_cache_path));
+                                 content::BrowserThread::FILE)
+                                 .get());
 }
 
 }  // namespace printing

chrome/browser/chromeos/printing/ppd_provider_factory.h

@@ -5,7 +5,7 @@
 #ifndef CHROME_BROWSER_CHROMEOS_PRINTING_PPD_PROVIDER_FACTORY_H_
 #define CHROME_BROWSER_CHROMEOS_PRINTING_PPD_PROVIDER_FACTORY_H_
 
-#include <memory>
+#include "base/memory/ref_counted.h"
 
 class Profile;
 
@@ -14,7 +14,7 @@ namespace printing {
 
 class PpdProvider;
 
-std::unique_ptr<PpdProvider> CreateProvider(Profile* profile);
+scoped_refptr<PpdProvider> CreateProvider(Profile* profile);
 
 }  // namespace printing
 }  // namsepace chromeos

chrome/browser/chromeos/printing/specifics_translation.cc

@@ -24,11 +24,8 @@ Printer::PpdReference SpecificsToPpd(
     ref.user_supplied_ppd_url = specifics.user_supplied_ppd_url();
   }
 
-  if (specifics.has_effective_model()) {
-    ref.effective_model = specifics.effective_model();
-
-    if (specifics.has_effective_manufacturer())
-      ref.effective_manufacturer = specifics.effective_manufacturer();
+  if (specifics.has_effective_make_and_model()) {
+    ref.effective_make_and_model = specifics.effective_make_and_model();
   }
 
   return ref;
@@ -41,11 +38,8 @@ sync_pb::PrinterPPDReference ReferenceToSpecifics(
     specifics.set_user_supplied_ppd_url(ref.user_supplied_ppd_url);
   }
 
-  if (!ref.effective_model.empty()) {
-    specifics.set_effective_model(ref.effective_model);
-
-    if (!ref.effective_manufacturer.empty())
-      specifics.set_effective_manufacturer(ref.effective_manufacturer);
+  if (!ref.effective_make_and_model.empty()) {
+    specifics.set_effective_make_and_model(ref.effective_make_and_model);
   }
 
   return specifics;

chrome/browser/chromeos/printing/specifics_translation_unittest.cc

@@ -20,7 +20,7 @@ const char model[] = "MODEL";
 const char uri[] = "ipps://notaprinter.chromium.org/ipp/print";
 const char uuid[] = "UUIDUUIDUUID";
 
-const char effective_model[] = "Manufacturer Model T1000";
+const char effective_make_and_model[] = "Manufacturer Model T1000";
 
 }  // namespace
 
@@ -38,7 +38,7 @@ TEST(SpecificsTranslationTest, SpecificsToPrinter) {
   specifics.set_uuid(uuid);
 
   sync_pb::PrinterPPDReference ppd;
-  ppd.set_effective_model(effective_model);
+  ppd.set_effective_make_and_model(effective_make_and_model);
   *specifics.mutable_ppd_reference() = ppd;
 
   std::unique_ptr<Printer> result = SpecificsToPrinter(specifics);
@@ -50,7 +50,8 @@ TEST(SpecificsTranslationTest, SpecificsToPrinter) {
   EXPECT_EQ(uri, result->uri());
   EXPECT_EQ(uuid, result->uuid());
 
-  EXPECT_EQ(effective_model, result->ppd_reference().effective_model);
+  EXPECT_EQ(effective_make_and_model,
+            result->ppd_reference().effective_make_and_model);
 }
 
 TEST(SpecificsTranslationTest, PrinterToSpecifics) {
@@ -64,7 +65,7 @@ TEST(SpecificsTranslationTest, PrinterToSpecifics) {
   printer.set_uuid(uuid);
 
   Printer::PpdReference ppd;
-  ppd.effective_model = effective_model;
+  ppd.effective_make_and_model = effective_make_and_model;
   *printer.mutable_ppd_reference() = ppd;
 
   std::unique_ptr<sync_pb::PrinterSpecifics> result =
@@ -77,7 +78,8 @@ TEST(SpecificsTranslationTest, PrinterToSpecifics) {
   EXPECT_EQ(uri, result->uri());
   EXPECT_EQ(uuid, result->uuid());
 
-  EXPECT_EQ(effective_model, result->ppd_reference().effective_model());
+  EXPECT_EQ(effective_make_and_model,
+            result->ppd_reference().effective_make_and_model());
 }
 
 TEST(SpecificsTranslationTest, SpecificsToPrinterRoundTrip) {
@@ -91,7 +93,7 @@ TEST(SpecificsTranslationTest, SpecificsToPrinterRoundTrip) {
   printer.set_uuid(uuid);
 
   Printer::PpdReference ppd;
-  ppd.effective_model = effective_model;
+  ppd.effective_make_and_model = effective_make_and_model;
   *printer.mutable_ppd_reference() = ppd;
 
   std::unique_ptr<sync_pb::PrinterSpecifics> temp = PrinterToSpecifics(printer);
@@ -105,7 +107,8 @@ TEST(SpecificsTranslationTest, SpecificsToPrinterRoundTrip) {
   EXPECT_EQ(uri, result->uri());
   EXPECT_EQ(uuid, result->uuid());
 
-  EXPECT_EQ(effective_model, result->ppd_reference().effective_model);
+  EXPECT_EQ(effective_make_and_model,
+            result->ppd_reference().effective_make_and_model);
 }
 
 }  // namespace printing

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

@@ -96,11 +96,12 @@ void HandlePrinterSetup(std::unique_ptr<chromeos::Printer> printer,
 
 void OnPrinterAddResult(std::unique_ptr<chromeos::Printer> printer,
                         const PrinterSetupCallback& cb,
-                        bool success) {
+                        int32_t result_code) {
   // It's expected that debug daemon posts callbacks on the UI thread.
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  HandlePrinterSetup(std::move(printer), success ? SUCCESS : FAILURE, cb);
+  HandlePrinterSetup(std::move(printer), (result_code == 0) ? SUCCESS : FAILURE,
+                     cb);
 }
 
 void OnPrinterAddError(const PrinterSetupCallback& cb) {
@@ -112,28 +113,39 @@ void OnPrinterAddError(const PrinterSetupCallback& cb) {
 }
 
 void AddPrinter(std::unique_ptr<chromeos::Printer> printer,
-                const std::string& ppd_path,
-                bool ipp_everywhere,
+                const std::string& ppd_contents,
                 const PrinterSetupCallback& cb) {
   // Always push configuration to CUPS.  It may need an update.
   const std::string printer_name = printer->id();
   const std::string printer_uri = printer->uri();
 
-  chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->CupsAddPrinter(
-      printer_name, printer_uri, ppd_path, ipp_everywhere,
-      base::Bind(&OnPrinterAddResult, base::Passed(&printer), cb),
-      base::Bind(&OnPrinterAddError, cb));
+  if (!ppd_contents.empty()) {
+    chromeos::DBusThreadManager::Get()
+        ->GetDebugDaemonClient()
+        ->CupsAddManuallyConfiguredPrinter(
+            printer_name, printer_uri, ppd_contents,
+            base::Bind(&OnPrinterAddResult, base::Passed(&printer), cb),
+            base::Bind(&OnPrinterAddError, cb));
+  } else {
+    // If we weren't given a ppd to use, we'd better be auto-configurable.
+    DCHECK(printer->IsIppEverywhere());
+    chromeos::DBusThreadManager::Get()
+        ->GetDebugDaemonClient()
+        ->CupsAddAutoConfiguredPrinter(
+            printer_name, printer_uri,
+            base::Bind(&OnPrinterAddResult, base::Passed(&printer), cb),
+            base::Bind(&OnPrinterAddError, cb));
+  }
 }
 
 void PPDResolve(std::unique_ptr<chromeos::Printer> printer,
                 const PrinterSetupCallback& cb,
                 chromeos::printing::PpdProvider::CallbackResultCode result,
-                base::FilePath path) {
+                const std::string& ppd_contents) {
   switch (result) {
     case chromeos::printing::PpdProvider::SUCCESS: {
-      DCHECK(!path.empty());
-      AddPrinter(std::move(printer), path.value() /* ppd path */,
-                 false /* non-ipp-everywhere */, cb);
+      DCHECK(!ppd_contents.empty());
+      AddPrinter(std::move(printer), ppd_contents, cb);
       break;
     }
     case chromeos::printing::PpdProvider::NOT_FOUND:
@@ -200,20 +212,18 @@ class PrinterBackendProxyChromeos : public PrinterBackendProxy {
 
     if (printer->IsIppEverywhere()) {
       // ChromeOS registers printer by GUID rather than display name.
-      AddPrinter(std::move(printer), "" /* empty ppd path */,
-                 true /* ipp everywhere */, cb);
-      return;
+      AddPrinter(std::move(printer), "", cb);
+    } else {
+      // Ref taken because printer is moved.
+      const chromeos::Printer::PpdReference& ppd_ref = printer->ppd_reference();
+      ppd_provider_->ResolvePpd(
+          ppd_ref, base::Bind(&PPDResolve, base::Passed(&printer), cb));
     }
-
-    // Ref taken because printer is moved.
-    const chromeos::Printer::PpdReference& ppd_ref = printer->ppd_reference();
-    ppd_provider_->Resolve(ppd_ref,
-                           base::Bind(&PPDResolve, base::Passed(&printer), cb));
   };
 
  private:
   chromeos::PrinterPrefManager* prefs_;
-  std::unique_ptr<chromeos::printing::PpdProvider> ppd_provider_;
+  scoped_refptr<chromeos::printing::PpdProvider> ppd_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(PrinterBackendProxyChromeos);
 };

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

@@ -49,7 +49,7 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
   void HandleRemoveCupsPrinter(const base::ListValue* args);
 
   void HandleAddCupsPrinter(const base::ListValue* args);
-  void OnAddedPrinter(std::unique_ptr<Printer> printer, bool success);
+  void OnAddedPrinter(std::unique_ptr<Printer> printer, int32_t result);
   void OnAddPrinterError();
 
   // Get a list of all manufacturers for which we have at least one model of
@@ -67,15 +67,14 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
   void HandleSelectPPDFile(const base::ListValue* args);
 
   // PpdProvider callback handlers.
-  void QueryAvailableManufacturersDone(
+  void ResolveManufacturersDone(
       const std::string& js_callback,
       chromeos::printing::PpdProvider::CallbackResultCode result_code,
-      const chromeos::printing::PpdProvider::AvailablePrintersMap& available);
-  void QueryAvailableModelsDone(
+      const std::vector<std::string>& available);
+  void ResolvePrintersDone(
       const std::string& js_callback,
-      const std::string& manufacturer,
       chromeos::printing::PpdProvider::CallbackResultCode result_code,
-      const chromeos::printing::PpdProvider::AvailablePrintersMap& available);
+      const std::vector<std::string>& available);
 
   // ui::SelectFileDialog::Listener override:
   void FileSelected(const base::FilePath& path,
@@ -99,12 +98,12 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
                         bool ipp_everywhere);
 
   // Callback for PpdProvider::ResolveCallback.
-  void OnPPDResolved(std::unique_ptr<Printer> printer,
-                     printing::PpdProvider::CallbackResultCode result,
-                     base::FilePath ppd_path);
+  void ResolvePpdDone(std::unique_ptr<Printer> printer,
+                      printing::PpdProvider::CallbackResultCode result,
+                      const std::string& ppd_contents);
 
   std::unique_ptr<chromeos::PrinterDiscoverer> printer_discoverer_;
-  std::unique_ptr<chromeos::printing::PpdProvider> ppd_provider_;
+  scoped_refptr<chromeos::printing::PpdProvider> ppd_provider_;
 
   Profile* profile_;
   scoped_refptr<ui::SelectFileDialog> select_file_dialog_;

chromeos/printing/ppd_cache.h

@@ -8,12 +8,12 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
-#include "base/optional.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_export.h"
-#include "chromeos/printing/ppd_provider.h"
-#include "chromeos/printing/printer_configuration.h"
 
 namespace chromeos {
 namespace printing {
@@ -27,66 +27,46 @@ namespace printing {
 // re-run the resolution logic if we have new meta-information about a printer.
 //
 // All cache functions must be called on a thread which is permitted to do I/O.
-class CHROMEOS_EXPORT PpdCache {
+class CHROMEOS_EXPORT PpdCache : public base::RefCounted<PpdCache> {
  public:
-  // Options that can be tweaked.  These should all have sane defaults.  This
-  // structure is copyable.
-  struct Options {
-    Options() {}
+  struct FindResult {
+    // Did we find something?  If this is false, none of the other fields are
+    // valid.
+    bool success = false;
 
-    // If the cached available printer data is older than this, we will consider
-    // it stale and won't return it.  A non-positive value here means we will
-    // always consider data stale (which is useful for tests).
-    // Default is 14 days.
-    base::TimeDelta max_available_list_staleness =
-        base::TimeDelta::FromDays(14);
+    // How old is this entry?
+    base::TimeDelta age;
 
-    // Size limit on the serialized cached available printer list, in bytes.
-    // This is just a check to make sure we don't consume ridiculous amounts of
-    // disk if we get bad data.
-    // Default is 10 MB
-    size_t max_available_list_cached_size = 10 * 1024 * 1024;
+    // Contents of the entry.
+    std::string contents;
   };
 
+  using FindCallback = base::Callback<void(const FindResult& result)>;
+
   // Create and return a Ppdcache that uses cache_dir to store state.  If
   // cache_base_dir does not exist, it will be lazily created the first time the
   // cache needs to store state.
-  static std::unique_ptr<PpdCache> Create(const base::FilePath& cache_base_dir,
-                                          const Options& options = Options());
-  virtual ~PpdCache() {}
+  static scoped_refptr<PpdCache> Create(
+      const base::FilePath& cache_base_dir,
+      scoped_refptr<base::SequencedTaskRunner> disk_task_runner);
 
-  // Find a PPD that was previously cached with the given reference.  Note that
-  // all fields of the reference must be the same, otherwise we'll miss in the
-  // cache and re-run resolution for the PPD.
-  //
-  // If a FilePath is returned, it is guaranteed to be non-empty and
-  // remain valid until the next Store() call.
-  virtual base::Optional<base::FilePath> Find(
-      const Printer::PpdReference& reference) const = 0;
+  // Start a Find, looking, for an entry with the given key that is at most
+  // |max_age| old.  |cb| will be invoked on the calling thread.
+  virtual void Find(const std::string& key, const FindCallback& cb) = 0;
 
-  // Take the contents of a PPD file, store it to the cache, and return the
-  // path to the stored file keyed on reference.
-  //
-  // If a different PPD was previously Stored for the given reference, it
-  // will be replaced.
-  //
-  // If a FilePath is returned, it is guaranteed to be non-empty and
-  // remain valid until the next Store() call.
-  virtual base::Optional<base::FilePath> Store(
-      const Printer::PpdReference& reference,
-      const std::string& ppd_contents) = 0;
+  // Store the given contents at the given key.  If cb is non-null, it will
+  // be invoked on completion.
+  virtual void Store(const std::string& key,
+                     const std::string& contents,
+                     const base::Callback<void()>& cb) = 0;
 
-  // Return a map of available printers, if we have one available and it's
-  // not too stale.  Returns nullopt if no map is available.  Note that, if the
-  // number of printers gets extremely large this map copy could get expensive
-  // and need to be reworked.
-  virtual base::Optional<PpdProvider::AvailablePrintersMap>
-  FindAvailablePrinters() = 0;
+  // Hook for testing.  Returns true if all outstanding cache operations
+  // are complete.
+  virtual bool Idle() const = 0;
 
-  // Store |available_printers|, replacing any existing entry.
-  virtual void StoreAvailablePrinters(
-      std::unique_ptr<PpdProvider::AvailablePrintersMap>
-          available_printers) = 0;
+ protected:
+  friend class base::RefCounted<PpdCache>;
+  virtual ~PpdCache() {}
 };
 
 }  // namespace printing

chromeos/printing/ppd_cache_unittest.cc

@@ -23,31 +23,11 @@ namespace chromeos {
 namespace printing {
 namespace {
 
-const char kTestManufacturer[] = "FooPrinters, Inc.";
-const char kTestModel[] = "Laser BarMatic 1000";
-const char kTestUserUrl[] = "/some/path/to/some/ppd/file";
-const char kTestPpdContents[] = "This is the ppd for the first printer";
-// Output of 'gzip -9' on a file with contents 'ppd contents'
-const char kTestGZippedPpdContents[] = {
-    0x1f, 0x8b, 0x08, 0x08, 0xe4, 0x2e, 0x00, 0x58, 0x02, 0x03, 0x70,
-    0x70, 0x64, 0x2e, 0x74, 0x78, 0x74, 0x00, 0x2b, 0x28, 0x48, 0x51,
-    0x48, 0xce, 0xcf, 0x2b, 0x49, 0xcd, 0x2b, 0x29, 0xe6, 0x02, 0x00,
-    0x2b, 0x51, 0x91, 0x24, 0x0d, 0x00, 0x00, 0x00};
-
-// Generate and return a PPDReference that has the fields set from the kTest
-// constants above.
-Printer::PpdReference TestReference() {
-  Printer::PpdReference ret;
-  ret.effective_manufacturer = kTestManufacturer;
-  ret.effective_model = kTestModel;
-  ret.user_supplied_ppd_url = kTestUserUrl;
-  return ret;
-}
-
 // This fixture just points the cache at a temporary directory for the life of
 // the test.
 class PpdCacheTest : public ::testing::Test {
  public:
+  PpdCacheTest() : loop_(base::MessageLoop::TYPE_IO) {}
   void SetUp() override {
     ASSERT_TRUE(ppd_cache_temp_dir_.CreateUniqueTempDir());
   }
@@ -56,139 +36,76 @@ class PpdCacheTest : public ::testing::Test {
   // which is cleaned up at the end of the test.  Note that we pass
   // a (nonexistant) subdirectory of temp_dir_ to the cache to exercise
   // the lazy-creation-of-the-cache-directory code.
-  std::unique_ptr<PpdCache> CreateTestCache(
-      const PpdCache::Options& options = PpdCache::Options()) {
+  scoped_refptr<PpdCache> CreateTestCache() {
     return PpdCache::Create(ppd_cache_temp_dir_.GetPath().Append("Cache"),
-                            options);
+                            loop_.task_runner().get());
+  }
+
+  void CaptureFindResult(const PpdCache::FindResult& result) {
+    ++captured_find_results_;
+    find_result_ = result;
   }
 
+  void CaptureStoreResult() { ++captured_store_results_; }
+
  protected:
+  // Number of find results we've captured.
+  int captured_find_results_ = 0;
+
+  // Most recent captured result.
+  PpdCache::FindResult find_result_;
+
+  // Number of store callbacks we've seen.
+  int captured_store_results_ = 0;
+
   // Overrider for DIR_CHROMEOS_PPD_CACHE that points it at a temporary
   // directory for the life of the test.
   base::ScopedTempDir ppd_cache_temp_dir_;
+  base::MessageLoop loop_;
 };
 
-// Check that path has a value, and the contents of the referenced file are
-// |expected_contents|.
-void CheckFileContentsAre(base::Optional<base::FilePath> result,
-                          const std::string& expected_contents) {
-  // Make sure we have a value.
-  ASSERT_TRUE(result);
-  // Make sure we get the ppd back that we stored.
-  std::string contents;
-  ASSERT_TRUE(base::ReadFileToString(result.value(), &contents));
-  EXPECT_EQ(expected_contents, contents);
-}
 
 // Test that we miss on an empty cache.
 TEST_F(PpdCacheTest, SimpleMiss) {
   auto cache = CreateTestCache();
-  EXPECT_FALSE(cache->Find(TestReference()));
+  cache->Find("foo", base::Bind(&PpdCacheTest::CaptureFindResult,
+                                base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(captured_find_results_, 1);
+  EXPECT_FALSE(find_result_.success);
 }
 
-// Test that when we store stuff, we get it back.
 TEST_F(PpdCacheTest, MissThenHit) {
   auto cache = CreateTestCache();
-  auto ref = TestReference();
-  EXPECT_FALSE(cache->Find(ref));
-
-  // Store should give us a reference to the file.
-  CheckFileContentsAre(cache->Store(ref, kTestPpdContents), kTestPpdContents);
-
-  // We should also get it back in response to a Find.
-  CheckFileContentsAre(cache->Find(ref), kTestPpdContents);
-}
-
-// Test that mutating any field in the reference causes us to miss in the cache
-// when we change the highest priority resolve criteria.
-TEST_F(PpdCacheTest, FieldChangeMeansCacheMiss) {
-  auto cache = CreateTestCache();
-  auto ref = TestReference();
-  CheckFileContentsAre(cache->Store(ref, kTestPpdContents), kTestPpdContents);
-
-  // We have a user url, so should still cache hit on manufacturer change.
-  ref.effective_manufacturer = "Something else";
-  EXPECT_TRUE(cache->Find(ref));
-  ref = TestReference();
-
-  // We have a user url, so should still cache hit on model change.
-  ref.effective_model = "Something else";
-  EXPECT_TRUE(cache->Find(ref));
-  ref = TestReference();
-
-  // But if we change th user url, that's a cache miss.
-  ref.user_supplied_ppd_url = "Something else";
-  EXPECT_FALSE(cache->Find(ref));
-  ref = TestReference();
-  // Should still find the initial Store when ref *is* identical.
-  CheckFileContentsAre(cache->Find(ref), kTestPpdContents);
-
-  // Now store the reference with no test url.
-  ref.user_supplied_ppd_url.clear();
-  CheckFileContentsAre(cache->Store(ref, kTestPpdContents), kTestPpdContents);
-
-  // Now changing the model or manufacturer should cause a cache miss.
-  ref.effective_manufacturer = "Something else";
-  EXPECT_FALSE(cache->Find(ref));
-  ref = TestReference();
-  ref.user_supplied_ppd_url.clear();
-  ref.effective_model = "Something else";
-  EXPECT_FALSE(cache->Find(ref));
-}
-
-// Test that gzipped contents get stored as .ppd.gz, and non-gzipped
-// contents get stored as .ppd
-TEST_F(PpdCacheTest, StoredExtensions) {
-  auto cache = CreateTestCache();
-  auto ref = TestReference();
-
-  // Not compressed, so should store as .ppd
-  auto result = cache->Store(ref, kTestPpdContents);
-  EXPECT_EQ(".ppd", cache->Store(ref, kTestPpdContents).value().Extension());
-
-  // Compressed, so should identify this and store as .ppd.gz
-  std::string gzipped_contents(kTestGZippedPpdContents,
-                               sizeof(kTestGZippedPpdContents));
-  EXPECT_EQ(".ppd.gz", cache->Store(ref, gzipped_contents).value().Extension());
-}
-
-// Test that we get back what we stored when we store an available printers
-// list.
-TEST_F(PpdCacheTest, StoreAndRetrieveAvailablePrinters) {
-  auto cache = CreateTestCache();
-
-  // Nothing stored, so should miss in the cache.
-  auto result = cache->FindAvailablePrinters();
-  EXPECT_FALSE(result);
-
-  // Create something to store.
-  auto a = base::MakeUnique<PpdProvider::AvailablePrintersMap>();
-  (*a)["foo"] = {"bar", "baz", "sna"};
-  (*a)["bar"] = {"c", "d", "e"};
-  (*a)["baz"] = {"f", "g", "h"};
-  PpdProvider::AvailablePrintersMap original = *a;
-
-  // Store it, get it back.
-  cache->StoreAvailablePrinters(std::move(a));
-  result = cache->FindAvailablePrinters();
-  ASSERT_TRUE(result);
-
-  EXPECT_EQ(original, result.value());
-}
-
-// When an entry is too old, we shouldn't return it.
-TEST_F(PpdCacheTest, ExpireStaleAvailablePrinters) {
-  PpdCache::Options options;
-  // Expire stuff immediately by setting the staleness limit to 0.
-  options.max_available_list_staleness = base::TimeDelta();
-  auto cache = CreateTestCache(options);
-
-  // Store an empty map.  (Contents don't really matter for this test).
-  cache->StoreAvailablePrinters(
-      base::MakeUnique<PpdProvider::AvailablePrintersMap>());
-
-  // Should *miss* in the cache because the entry is already expired.
-  EXPECT_FALSE(cache->FindAvailablePrinters());
+  const char kTestKey[] = "My totally awesome key";
+  const char kTestKey2[] = "A different key";
+  const char kTestContents[] = "Like, totally awesome contents";
+
+  cache->Find(kTestKey, base::Bind(&PpdCacheTest::CaptureFindResult,
+                                   base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(captured_find_results_, 1);
+  EXPECT_FALSE(find_result_.success);
+
+  cache->Store(
+      kTestKey, kTestContents,
+      base::Bind(&PpdCacheTest::CaptureStoreResult, base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(captured_store_results_, 1);
+
+  cache->Find(kTestKey, base::Bind(&PpdCacheTest::CaptureFindResult,
+                                   base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(captured_find_results_, 2);
+  EXPECT_TRUE(find_result_.success);
+  EXPECT_EQ(find_result_.contents, kTestContents);
+  EXPECT_LT(find_result_.age, base::TimeDelta::FromMinutes(5));
+
+  cache->Find(kTestKey2, base::Bind(&PpdCacheTest::CaptureFindResult,
+                                    base::Unretained(this)));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(captured_find_results_, 3);
+  EXPECT_FALSE(find_result_.success);
 }
 
 }  // namespace

chromeos/printing/ppd_provider.h

@@ -30,10 +30,10 @@ class PpdCache;
 // user previously identified for use, and falls back to querying quirksserver
 // based on manufacturer/model of the printer.
 //
-// This class can be accessed from any thread.
-class CHROMEOS_EXPORT PpdProvider {
+// All functions in this class must be called from a sequenced context.
+class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
  public:
-  // Possible result codes of a Resolve() or QueryAvailable() call.
+  // Possible result codes of a Resolve*() call.
   enum CallbackResultCode {
     SUCCESS,
 
@@ -48,73 +48,83 @@ class CHROMEOS_EXPORT PpdProvider {
     INTERNAL_ERROR,
   };
 
-  // Result of a Resolve().  If the result code is SUCCESS, then FilePath holds
-  // the path to a PPD file (that may or may not be gzipped).  Otherwise, the
-  // FilePath will be empty.
-  using ResolveCallback =
-      base::Callback<void(CallbackResultCode, base::FilePath)>;
-
-  // Available printers are represented as a map from manufacturer to
-  // list-of-printer-models.
-  using AvailablePrintersMap = std::map<std::string, std::vector<std::string>>;
-
-  // Result of a QueryAvailable.  If the result code is SUCCESS, then
-  // AvailablePrintersMap holds a map from manufacturer name to list of printer
-  // names.  Otherwise the map will be empty.
-  using QueryAvailableCallback =
-      base::Callback<void(CallbackResultCode, const AvailablePrintersMap&)>;
-
   // Construction-time options.  Everything in this structure should have
   // a sane default.
   struct Options {
     Options() {}
 
-    // hostname of quirks server to query.
-    std::string quirks_server = "chromeosquirksserver-pa.googleapis.com";
-
-    // Maximum size of the contents of a PPD file, in bytes.  Trying to use a
-    // PPD file bigger than this will cause INTERNAL_ERRORs at resolution time.
-    size_t max_ppd_contents_size_ = 100 * 1024;
+    // Root of the ppd serving hierarchy.
+    std::string ppd_server_root = "https://www.gstatic.com/chromeos_printing";
   };
 
+  // Result of a ResolvePpd() call.  If the result code is SUCCESS, then the
+  // string holds the contents of a PPD (that may or may not be gzipped).
+  // Otherwise, the string will be empty.
+  using ResolvePpdCallback =
+      base::Callback<void(CallbackResultCode, const std::string&)>;
+
+  // Result of a ResolveManufacturers() call.  If the result code is SUCCESS,
+  // then the vector contains a sorted list of manufacturers for which we have
+  // at least one printer driver.
+  using ResolveManufacturersCallback =
+      base::Callback<void(CallbackResultCode, const std::vector<std::string>&)>;
+
+  // 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.
+  using ResolvePrintersCallback =
+      base::Callback<void(CallbackResultCode, const std::vector<std::string>&)>;
+
   // Create and return a new PpdProvider with the given cache and options.
-  // |io_task_runner| is used to run operations that are long latency and should
-  // not be on the UI thread.  References to |url_context_getter| and
-  // |io_task_runner| are taken.
-  static std::unique_ptr<PpdProvider> Create(
-      const std::string& api_key,
+  // A references to |url_context_getter| is taken.
+  static scoped_refptr<PpdProvider> Create(
+      const std::string& browser_locale,
       scoped_refptr<net::URLRequestContextGetter> url_context_getter,
-      scoped_refptr<base::SequencedTaskRunner> io_task_runner,
-      std::unique_ptr<PpdCache> cache,
+      scoped_refptr<PpdCache> cache,
+      scoped_refptr<base::SequencedTaskRunner> disk_task_runner,
       const Options& options = Options());
 
-  virtual ~PpdProvider() {}
-
-  // Given a PpdReference, attempt to resolve the PPD for printing.
+  // Get all manufacturers for which we have drivers.  Keys of the map will be
+  // localized in the default browser locale or the closest available fallback.
   //
-  // Must be called from a Sequenced Task context (i.e.
-  // base::SequencedTaskRunnerHandle::IsSet() must be true).
-  //
-  // |cb| will only be called after the task invoking Resolve() is finished.
-  virtual void Resolve(const Printer::PpdReference& ppd_reference,
-                       const ResolveCallback& cb) = 0;
+  // |cb| will be called on the invoking thread, and will be sequenced.
+  virtual void ResolveManufacturers(const ResolveManufacturersCallback& cb) = 0;
 
-  // Get all the printer makes and models we can support.
+  // 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 |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.
   //
-  // Must be called from a Sequenced Task context (i.e.
-  // base::SequencedTaskRunnerHandle::IsSet() must be true).
+  // 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;
+
+  // Given a PpdReference, attempt to get the PPD for printing.
   //
-  // |cb| will only be called after the task invoking QueryAvailable() is
-  // finished.
-  virtual void QueryAvailable(const QueryAvailableCallback& cb) = 0;
-
-  // Most of the time, the cache is just an invisible backend to the Provider,
-  // consulted at Resolve time, but in the case of the user doing "Add Printer"
-  // and "Select PPD" locally, then we get into a state where we want to put
-  // whatever they give us directly into the cache without doing a resolve.
-  // This hook lets is do that.
-  virtual bool CachePpd(const Printer::PpdReference& ppd_reference,
-                        const base::FilePath& ppd_path) = 0;
+  // |cb| will be called on the invoking thread, and will be sequenced.
+  virtual void ResolvePpd(const Printer::PpdReference& reference,
+                          const ResolvePpdCallback& cb) = 0;
+
+  // Hook for testing.  Returns true if there are no API calls that have not
+  // yet completed.
+  virtual bool Idle() const = 0;
+
+ protected:
+  friend class base::RefCounted<PpdProvider>;
+  virtual ~PpdProvider() {}
 };
 
 }  // namespace printing

chromeos/printing/printer_configuration.h

@@ -20,27 +20,21 @@ class CHROMEOS_EXPORT Printer {
   // If you add fields to this struct, you almost certainly will
   // want to update PpdResolver and PpdCache::GetCachePath.
   //
+  // Exactly one of the fields below should be filled in.
+  //
   // At resolution time, we look for a cached PPD that used the same
   // PpdReference before.
   //
-  // If one is not found and user_supplied_ppd_url is set, we'll fail
-  // out with NOT FOUND
-  //
-  // Otherwise, we'll hit QuirksServer to see if we can resolve a Ppd
-  // using manufacturer/model
   struct PpdReference {
     // If non-empty, this is the url of a specific PPD the user has specified
-    // for use with this printer.
+    // for use with this printer.  The ppd can be gzipped or uncompressed.  This
+    // url must use a file:// scheme.
     std::string user_supplied_ppd_url;
 
-    // The *effective* manufacturer and model of this printer, in other words,
-    // the manufacturer and model of the printer for which we grab a PPD.  This
-    // doesn’t have to (but can) be the actual manufacturer/model of the
-    // printer.  We should always try to fill these fields in, even if we don’t
-    // think they’ll be needed, as they provide a fallback mechanism for
-    // finding a PPD.
-    std::string effective_manufacturer;
-    std::string effective_model;
+    // String that identifies which ppd to use from the ppd server.
+    // Where possible, this is the same as the ipp/ldap
+    // printer-make-and-model field.
+    std::string effective_make_and_model;
   };
 
   // The location where the printer is stored.

chromeos/printing/printer_translator.cc

@@ -21,7 +21,8 @@ namespace {
 
 // PPD reference fields
 const char kUserSuppliedPpdUrl[] = "user_supplied_ppd_url";
-const char kEffectiveManufacturer[] = "effective_manufacturer";
+// TODO(justincarlson) -- This should be changed to effective_make_and_model to
+// match the implementation.
 const char kEffectiveModel[] = "effective_model";
 
 // printer fields
@@ -40,8 +41,7 @@ Printer::PpdReference DictionaryToPpdReference(
     const base::DictionaryValue* value) {
   Printer::PpdReference ppd;
   value->GetString(kUserSuppliedPpdUrl, &ppd.user_supplied_ppd_url);
-  value->GetString(kEffectiveManufacturer, &ppd.effective_manufacturer);
-  value->GetString(kEffectiveModel, &ppd.effective_model);
+  value->GetString(kEffectiveModel, &ppd.effective_make_and_model);
   return ppd;
 }
 
@@ -52,11 +52,8 @@ std::unique_ptr<base::DictionaryValue> PpdReferenceToDictionary(
   if (!ppd.user_supplied_ppd_url.empty()) {
     dictionary->SetString(kUserSuppliedPpdUrl, ppd.user_supplied_ppd_url);
   }
-  if (!ppd.effective_manufacturer.empty()) {
-    dictionary->SetString(kEffectiveManufacturer, ppd.effective_manufacturer);
-  }
-  if (!ppd.effective_model.empty()) {
-    dictionary->SetString(kEffectiveModel, ppd.effective_model);
+  if (!ppd.effective_make_and_model.empty()) {
+    dictionary->SetString(kEffectiveModel, ppd.effective_make_and_model);
   }
   return dictionary;
 }

chromeos/printing/printer_translator_unittest.cc

@@ -25,8 +25,7 @@ const char kUUID[] = "UUID-UUID-UUID";
 
 // PpdReference test data
 const char kUserSuppliedPpdUrl[] = "/some/path/to/user.url";
-const char kEffectiveManufacturer[] = "Ehch Pea";
-const char kEffectiveModel[] = "PrintBlaster 2000";
+const char kEffectiveMakeAndModel[] = "PrintBlaster 2000";
 
 TEST(PrinterTranslatorTest, PrefToPrinterMissingId) {
   base::DictionaryValue value;
@@ -81,16 +80,13 @@ TEST(PrinterTranslatorTest, PrinterToPrefPpdReference) {
   Printer printer("UNIQUE_ID");
   auto* ppd = printer.mutable_ppd_reference();
   ppd->user_supplied_ppd_url = kUserSuppliedPpdUrl;
-  ppd->effective_manufacturer = kEffectiveManufacturer;
-  ppd->effective_model = kEffectiveModel;
+  ppd->effective_make_and_model = kEffectiveMakeAndModel;
 
   std::unique_ptr<base::DictionaryValue> actual = PrinterToPref(printer);
 
   base::ExpectDictStringValue(kUserSuppliedPpdUrl, *actual,
                               "ppd_reference.user_supplied_ppd_url");
-  base::ExpectDictStringValue(kEffectiveManufacturer, *actual,
-                              "ppd_reference.effective_manufacturer");
-  base::ExpectDictStringValue(kEffectiveModel, *actual,
+  base::ExpectDictStringValue(kEffectiveMakeAndModel, *actual,
                               "ppd_reference.effective_model");
 }
 
@@ -100,8 +96,7 @@ TEST(PrinterTranslatorTest, PrinterToPrefPpdReferenceLazy) {
   std::unique_ptr<base::DictionaryValue> actual = PrinterToPref(printer);
 
   EXPECT_FALSE(actual->HasKey("ppd_reference.user_supplied_ppd_url"));
-  EXPECT_FALSE(actual->HasKey("ppd_reference.effective_manufacturer"));
-  EXPECT_FALSE(actual->HasKey("ppd_reference.effective_model"));
+  EXPECT_FALSE(actual->HasKey("ppd_reference.ppd_server_key"));
 }
 
 TEST(PrinterTranslatorTest, PrefToPrinterRoundTrip) {
@@ -116,9 +111,7 @@ TEST(PrinterTranslatorTest, PrefToPrinterRoundTrip) {
 
   preference.SetString("ppd_reference.user_supplied_ppd_url",
                        kUserSuppliedPpdUrl);
-  preference.SetString("ppd_reference.effective_manufacturer",
-                       kEffectiveManufacturer);
-  preference.SetString("ppd_reference.effective_model", kEffectiveModel);
+  preference.SetString("ppd_reference.effective_model", kEffectiveMakeAndModel);
 
   std::unique_ptr<Printer> printer = PrefToPrinter(preference);
   std::unique_ptr<base::DictionaryValue> pref_copy = PrinterToPref(*printer);

chromeos/printing/printing_constants.h

+// Copyright 2016 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.
+
+#ifndef CHROMEOS_PRINTING_PRINTING_CONSTANTS_H_
+#define CHROMEOS_PRINTING_PRINTING_CONSTANTS_H_
+
+namespace chromeos {
+namespace printing {
+
+// Maximum size of a PPD file that we will accept, currently 250k.  This number
+// is relatively
+// arbitrary, we just don't want to try to handle ridiculously huge files.
+constexpr size_t kMaxPpdSizeBytes = 250 * 1024;
+
+}  // namespace printing
+}  // namespace chromeos
+
+#endif  // CHROMEOS_PRINTING_PRINTING_CONSTANTS_H_

components/sync/protocol/printer_specifics.proto

@@ -18,9 +18,13 @@ message PrinterPPDReference {
   // Url for user provided file.  Overrides other fields.
   optional string user_supplied_ppd_url = 1;
 
-  // Make and model to reference into PPD Server.
-  optional string effective_manufacturer = 2;
-  optional string effective_model = 3;
+  // Retired fields
+  optional string effective_manufacturer = 2 [deprecated = true];
+  optional string effective_model = 3 [deprecated = true];
+
+  // String identifying the type of printer, used to look up a ppd to drive the
+  // printer.
+  optional string effective_make_and_model = 4;
 }
 
 message PrinterSpecifics {

components/sync/protocol/proto_visitors.h

@@ -205,8 +205,7 @@ VISIT_PROTO_FIELDS(const sync_pb::ArcPackageSpecifics& proto) {
 
 VISIT_PROTO_FIELDS(const sync_pb::PrinterPPDReference& proto) {
   VISIT(user_supplied_ppd_url);
-  VISIT(effective_manufacturer);
-  VISIT(effective_model);
+  VISIT(effective_make_and_model);
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::ReadingListSpecifics& proto) {