Add PPDProvider barebones implementation and associated cache skeleton.

chromeos/printing/ppd_provider_unittest.cc

Index: chromeos/printing/ppd_provider_unittest.cc
diff --git a/chromeos/printing/ppd_provider_unittest.cc b/chromeos/printing/ppd_provider_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8675576ad57d727d97a988c5bd83b8ab3df39209
--- /dev/null
+++ b/chromeos/printing/ppd_provider_unittest.cc
@@ -0,0 +1,191 @@
+// 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.
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chromeos/chromeos_paths.h"
+#include "chromeos/printing/ppd_cache.h"
+#include "chromeos/printing/ppd_provider.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"
+
+using base::FilePath;
+using std::string;
+using std::unique_ptr;
+
+namespace chromeos {
+namespace printing {
+namespace {
+
+const char kTestQuirksServer[] = "bogusserver.bogus.com";
+const char kTestAPIKey[] = "BOGUSAPIKEY";
+const char kLocalPpdUrl[] = "/some/path";
+const char kTestManufacturer[] = "Bogus Printer Corp";
+const char kTestModel[] = "MegaPrint 9000";
+const char kQuirksResponse[] =
+    "{\n"
+    "  \"compressedPpd\": \"This is the quirks ppd\",\n"
+    "  \"lastUpdatedTime\": \"1\"\n"
+    "}\n";
+const char kQuirksPpd[] = "This is the quirks ppd";
+
+class PpdProviderTest : public ::testing::Test {
+ public:
+  PpdProviderTest()
+      : loop_(base::MessageLoop::TYPE_IO),
+        request_context_getter_(
+            new ::net::TestURLRequestContextGetter(loop_.task_runner().get())) {
+    CHECK(ppd_cache_temp_dir_.CreateUniqueTempDir());

[Lei Zhang, 2016/10/24 22:26:24]

Also move to SetUp() and ASSERT_TRUE() instead?

[Carlson, 2016/10/24 22:48:44]

Done.

+  }
+
+  // Create the ppd_provider_ to be used for this test.

[Lei Zhang, 2016/10/24 22:26:24]

|ppd_provider_|

[Carlson, 2016/10/24 22:48:44]

Comment was sort of pointless anyways, so removed.

+  void SetUp() override {
+    auto provider_options = PpdProvider::Options();
+    provider_options.quirks_server = kTestQuirksServer;
+    ppd_provider_ = PpdProvider::Create(
+        kTestAPIKey, request_context_getter_,
+        PpdCache::Create(ppd_cache_temp_dir_.GetPath()), provider_options);
+  }
+
+ protected:
+  base::ScopedTempDir ppd_cache_temp_dir_;
+
+  // Provider to be used in the test.
+  unique_ptr<PpdProvider> ppd_provider_;
+
+  // Misc extra stuff needed for the test environment to function.
+  base::MessageLoop loop_;
+  scoped_refptr<::net::URLRequestContextGetter> request_context_getter_;

[Lei Zhang, 2016/10/24 22:26:24]

No leading "::". No other code refers to URLRequestContextGetter that way.

[Carlson, 2016/10/24 22:48:44]

Done, cleaned up a few others, too.

+};
+
+// Struct that just captures the callback result for a PpdProvider lookup and
+// saves it for inspection by the test.
+struct CapturedResolveResult {
+  bool initialized = false;
+  PpdProvider::ResolveResult result;
+  FilePath file;
+};
+
+// Callback for saving a resolve callback.
+void CaptureResolveResultCallback(CapturedResolveResult* capture,
+                                  PpdProvider::ResolveResult result,
+                                  FilePath file) {
+  capture->initialized = true;
+  capture->result = result;
+  capture->file = file;
+}
+
+// For a resolve result that should end up successful, check that it is
+// successful and the contents are expected_contents.
+void CheckResolveSuccessful(const CapturedResolveResult& captured,
+                            const string& expected_contents) {
+  ASSERT_TRUE(captured.initialized);
+  EXPECT_EQ(PpdProvider::SUCCESS, captured.result);
+
+  string contents;
+  ASSERT_TRUE(base::ReadFileToString(captured.file, &contents));
+  EXPECT_EQ(expected_contents, contents);
+}
+
+// Resolve a PPD via the quirks server.
+TEST_F(PpdProviderTest, QuirksServerResolve) {
+  base::ScopedTempDir temp_dir;
+  CHECK(temp_dir.CreateUniqueTempDir());
+
+  Printer::PpdReference ppd_reference;
+  ppd_reference.effective_manufacturer = kTestManufacturer;
+  ppd_reference.effective_model = kTestModel;
+
+  {
+    ::net::TestURLRequestInterceptor interceptor(
+        "https", kTestQuirksServer, base::ThreadTaskRunnerHandle::Get(),
+        base::ThreadTaskRunnerHandle::Get());
+
+    GURL expected_url(string("https://") + kTestQuirksServer  //

[Lei Zhang, 2016/10/24 22:26:24]

foo +
bar +
qux

is preferred over

foo
+bar
+qux

[Lei Zhang, 2016/10/24 22:26:24]

Using empty comments to force formatting still feels wrong. Imagine what the
code base would look like if everyone did this to get their piece of code
formatted just the way they like it.

[Carlson, 2016/10/24 22:48:44]

Done.

[Carlson, 2016/10/24 22:48:44]

Acknowledged.

[Lei Zhang, 2016/10/24 23:33:43]

So does base::StringPrintf() help here like it did in ppd_provider.cc? If yes,
great, let's go with that. If not, can you please remove the comments, and let
clang-format do its thing? It's what all of chromeos/ does so it would be nice
to be consistent. Once again, this is a creative use of comments, but nobody
else does this.

[Carlson, 2016/10/25 00:04:01]

I think base::StringPrintf is less readable here than vertical formatting.

Pulled the lines back a bit to pair url prefixes with associated variable
contents, which I actually think looks a little better.  

We seem to have fundamentally incompatible views about how "not commonly done"
should inform the structure of new code.  While I think it's a factor in
deciding how to do things, I don't think it should totally override personal
judgement on complicated expressions (unless, of course style guide says
otherwise).

[Lei Zhang, 2016/10/25 00:33:44]

Ack.
The factors that I'm considering are:

- You strongly prefer this style. (I'm not against listing things vertically)
- Listing arguments vertically sometimes does make it more readable. (I've
certainly done this in cases, and even had clang-format format it away)
- In chromeos/ the top level OWNERS have stated they want the directory
clang-formatted, as is the case in many, but not all Chromium code.
- Even though this is a newly created file, it lives in chromeos/ and should
stick with chromeos/ rules. It is part of a large project and at some point
other people will have to read / modify this code. For their benefit, it would
be good to "be consistent with existing code" and stick with the expectations
set in the larger project.
- I can't find another example in the code base where someone checked in code
that does this to defeat clang-format.

Of the above, consistency is the one that weighs most heavily to me, as seen at
the parting words of the C++ style guide:

"Use common sense and BE CONSISTENT.

If you are editing code, take a few minutes to look at the code around you and
determine its style. If they use spaces around their if clauses, you should,
too. If their comments have little boxes of stars around them, make your
comments have little boxes of stars around them too."

[Carlson, 2016/10/25 01:59:32]

Took discussion offline, but made change.

+                      + "/v2/printer/manufacturers/"          //
+                      + kTestManufacturer                     //
+                      + "/models/"                            //
+                      + kTestModel                            //
+                      + "?key="                               //
+                      + kTestAPIKey);
+
+    FilePath contents_path = temp_dir.GetPath().Append("response");
+    string contents = kQuirksResponse;
+    int bytes_written = base::WriteFile(contents_path, kQuirksResponse,
+                                        strlen(kQuirksResponse));
+    CHECK_EQ(bytes_written, static_cast<int>(strlen(kQuirksResponse)));

[Lei Zhang, 2016/10/24 22:26:24]

ASSERT_EQ?

[Carlson, 2016/10/24 22:48:44]

Done.

+
+    interceptor.SetResponse(expected_url, contents_path);
+
+    CapturedResolveResult captured;
+    ppd_provider_->Resolve(ppd_reference,
+                           base::Bind(CaptureResolveResultCallback, &captured));
+    base::RunLoop().RunUntilIdle();
+    CheckResolveSuccessful(captured, kQuirksPpd);
+  }
+
+  // Now that the interceptor is out of scope, re-run the query.  We should
+  // hit in the cache, and thus *not* re-run the query.
+  CapturedResolveResult captured;
+  ppd_provider_->Resolve(ppd_reference,
+                         base::Bind(CaptureResolveResultCallback, &captured));
+  base::RunLoop().RunUntilIdle();
+  CheckResolveSuccessful(captured, kQuirksPpd);
+}
+
+// Test storage and retrieval of PPDs that are added manually.  Even though we
+// supply a manufacturer and model, we should *not* hit the network for this
+// resolution since we should find the stored version already cached.
+TEST_F(PpdProviderTest, LocalResolve) {
+  Printer::PpdReference ppd_reference;
+  ppd_reference.user_supplied_ppd_url = kLocalPpdUrl;
+  ppd_reference.effective_manufacturer = kTestManufacturer;
+  ppd_reference.effective_model = kTestModel;
+
+  // Initially, should not resolve.
+  {
+    CapturedResolveResult captured;
+    ppd_provider_->Resolve(ppd_reference,
+                           base::Bind(CaptureResolveResultCallback, &captured));
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(captured.initialized);
+    EXPECT_EQ(PpdProvider::NOT_FOUND, captured.result);
+  }
+
+  // Store a local ppd.
+  const string kLocalPpdContents("My local ppd contents");
+  {
+    base::ScopedTempDir temp_dir;
+    CHECK(temp_dir.CreateUniqueTempDir());
+
+    FilePath local_ppd_path = temp_dir.GetPath().Append("local_ppd");
+    ASSERT_EQ(base::WriteFile(local_ppd_path, kLocalPpdContents.data(),
+                              kLocalPpdContents.size()),
+              static_cast<int>(kLocalPpdContents.size()));
+    ASSERT_TRUE(ppd_provider_->CachePpd(ppd_reference, local_ppd_path));
+  }
+  // temp_dir should now be deleted, which helps make sure we actually latched a
+  // copy, not a reference.
+
+  // Retry the resove, should get the PPD back now.
+  {
+    CapturedResolveResult captured;
+
+    ppd_provider_->Resolve(ppd_reference,
+                           base::Bind(CaptureResolveResultCallback, &captured));
+    base::RunLoop().RunUntilIdle();
+    CheckResolveSuccessful(captured, kLocalPpdContents);
+  }
+}
+
+}  // namespace
+}  // namespace printing
+}  // namespace chromeos