PaymentApp: Introduce PaymentAppDatabase class.

content/browser/payments/payment_app_manager_unittest.cc

 // 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 "content/browser/payments/payment_app_manager.h"
 
 #include <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 43 matching lines @@
 
 class PaymentAppManagerTest : public testing::Test {
  public:
   PaymentAppManagerTest()
       : thread_bundle_(
             new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
         embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())),
         storage_partition_impl_(new StoragePartitionImpl(
             embedded_worker_helper_->browser_context(), base::FilePath(),
             nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
             nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr)) {

[please use gerrit instead, 2016/12/14 19:44:00]

Initialize sw_registration_id_(0) here to avoid a period of time when the value
of this variable is random.

[zino, 2016/12/15 17:56:52]

Done.

 
     embedded_worker_helper_->context_wrapper()->set_storage_partition(
         storage_partition_impl_.get());
 
-    payment_app_context_ =
-        new PaymentAppContextImpl(embedded_worker_helper_->context_wrapper());
+    payment_app_context_ = new PaymentAppContextImpl();
+    payment_app_context_->Init(embedded_worker_helper_->context_wrapper());
 
     bool called = false;
     embedded_worker_helper_->context()->RegisterServiceWorker(
         GURL(kServiceWorkerPattern), GURL(kServiceWorkerScript), NULL,
         base::Bind(&RegisterServiceWorkerCallback, &called,
                    &sw_registration_id_));
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(called);

[please use gerrit instead, 2016/12/14 19:44:00]

Would be nice to have this EXPECT_TRUE(called) also for SetManifest() and
GetManifest() methods below.

[zino, 2016/12/15 17:56:52]

Done.

 
     mojo::InterfaceRequest<payments::mojom::PaymentAppManager> request =
         mojo::GetProxy(&service_);
     payment_app_context_->CreateService(std::move(request));
     base::RunLoop().RunUntilIdle();
 
     manager_ = payment_app_context_->services_.begin()->first;
     EXPECT_TRUE(manager_);

[please use gerrit instead, 2016/12/14 19:44:00]

EXPECT_NE(null, manager_) is better, because manager_ is not a boolean.

[zino, 2016/12/15 23:33:56]

Done.

   }
 
   ~PaymentAppManagerTest() override {
     payment_app_context_->Shutdown();
     base::RunLoop().RunUntilIdle();
   }
 
   void SetManifest(const std::string& scope,
                    payments::mojom::PaymentAppManifestPtr manifest,
                    const PaymentAppManager::SetManifestCallback& callback) {
@@ skipping 28 matching lines @@
   option->id = "payment-app-id";
   option->icon = std::string("payment-app-icon");
   option->enabled_methods.push_back("visa");
 
   payments::mojom::PaymentAppManifestPtr manifest =
       payments::mojom::PaymentAppManifest::New();
   manifest->icon = std::string("payment-app-icon");
   manifest->label = "Payment App";
   manifest->options.push_back(std::move(option));
 
   payments::mojom::PaymentAppManifestError error;

[please use gerrit instead, 2016/12/14 19:44:00]

Initialize it to something that's not NONE to avoid a bug where "error" is never
set, but it's default value happens to be NONE in some cases or something else
in other cases. For example:

payments::mojom::PaymentAppManifestError error =
payments::mojom::PaymentAppManifestError::MANIFEST_STORAGE_OPERATION_FAILED;

[zino, 2016/12/15 17:56:52]

Done.

   SetManifest(kServiceWorkerPattern, std::move(manifest),
               base::Bind(&SetManifestCallback, &error));
 
   ASSERT_EQ(error, payments::mojom::PaymentAppManifestError::NONE);
 
   payments::mojom::PaymentAppManifestPtr read_manifest;
   payments::mojom::PaymentAppManifestError read_error;

[please use gerrit instead, 2016/12/14 19:44:00]

init to something other than NONE.

[zino, 2016/12/15 17:56:52]

Done.

   GetManifest(kServiceWorkerPattern,
               base::Bind(&GetManifestCallback, &read_manifest, &read_error));
 
   ASSERT_EQ(read_error, payments::mojom::PaymentAppManifestError::NONE);
   EXPECT_EQ(read_manifest->icon, std::string("payment-app-icon"));
   EXPECT_EQ(read_manifest->label, "Payment App");
   ASSERT_EQ(read_manifest->options.size(), 1U);
   EXPECT_EQ(read_manifest->options[0]->icon, std::string("payment-app-icon"));
   EXPECT_EQ(read_manifest->options[0]->label, "Visa ****");
   EXPECT_EQ(read_manifest->options[0]->id, "payment-app-id");
   ASSERT_EQ(read_manifest->options[0]->enabled_methods.size(), 1U);
   EXPECT_EQ(read_manifest->options[0]->enabled_methods[0], "visa");
 }
 
 TEST_F(PaymentAppManagerTest, GetManifestWithoutAssociatedServiceWorker) {

[please use gerrit instead, 2016/12/14 19:44:00]

Need a symmetric test case for SetManifestWithoutAssociatedServiceWorker?

[zino, 2016/12/15 17:56:52]

The name was confusing.
Rename the test to GetManifestWithNoSavedManifest and then add two tests for no
associated SW.

Done.

   payments::mojom::PaymentAppManifestPtr read_manifest;
   payments::mojom::PaymentAppManifestError read_error;

[please use gerrit instead, 2016/12/14 19:44:00]

init to something other than MANIFEST_STORAGE_OPERATION_FAILED.

[zino, 2016/12/15 17:56:52]

Done.

   GetManifest(kServiceWorkerPattern,
               base::Bind(&GetManifestCallback, &read_manifest, &read_error));
 
   EXPECT_EQ(read_error, payments::mojom::PaymentAppManifestError::
                             MANIFEST_STORAGE_OPERATION_FAILED);
 }
 
 }  // namespace content