PaymentApp: Implement PaymentAppManager.setManifest().

content/browser/payments/payment_app_manager_unittest.cc

Index: content/browser/payments/payment_app_manager_unittest.cc
diff --git a/content/browser/payments/payment_app_manager_unittest.cc b/content/browser/payments/payment_app_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c5433a7dfb3dd42648f06d0f3119455aa7dbfd99
--- /dev/null
+++ b/content/browser/payments/payment_app_manager_unittest.cc
@@ -0,0 +1,167 @@
+// 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>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/run_loop.h"
+#include "components/payments/payment_app.mojom.h"
+#include "content/browser/service_worker/embedded_worker_test_helper.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/browser/storage_partition_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "mojo/public/cpp/bindings/interface_ptr.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+

[please use gerrit instead, 2016/11/15 20:04:27]

No newline.

[zino, 2016/11/16 18:12:58]

Done.

+namespace {
+
+const char kServiceWorkerPattern[] = "https://example.com/a";
+const char kServiceWorkerScript[] = "https://example.com/a/script.js";
+const char kPaymentAppManifestDataKey[] = "PaymentAppManifestData";
+
+void RegisterServiceWorkerCallback(bool* called,
+                                   int64_t* store_registration_id,
+                                   ServiceWorkerStatusCode status,
+                                   const std::string& status_message,
+                                   int64_t registration_id) {
+  EXPECT_EQ(SERVICE_WORKER_OK, status) << ServiceWorkerStatusToString(status);
+  *called = true;
+  *store_registration_id = registration_id;
+}
+
+void SetManifestCallback(payments::mojom::PaymentAppManifestError* out_error,
+                         payments::mojom::PaymentAppManifestError error) {
+  *out_error = error;
+}
+
+void ReadManifestDataCallback(std::vector<std::string>* out_data,
+                              const std::vector<std::string>& data,
+                              ServiceWorkerStatusCode status) {
+  *out_data = data;
+}
+
+}  // namespace
+
+class PaymentAppManagerTest : public testing::Test {
+ public:
+  PaymentAppManagerTest()
+      : thread_bundle_(
+            new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)) {
+  }

[please use gerrit instead, 2016/11/15 20:04:27]

If you have a constructor, you should have a destructor. Note that this
destructor should have "override" keyword because you're deriving from
"testing::Test" class.

[zino, 2016/11/16 18:12:59]

Done.

+
+  void SetUp() override {

[please use gerrit instead, 2016/11/15 20:04:27]

SetUp is usually not used. You can put all these function calls in the
constructor.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

[zino, 2016/11/16 18:12:58]

Done.

+    CreateTestHelper();
+    CreateStoragePartition();
+    CreatePaymentAppContext();
+    CreateServiceWorkerRegistration();
+    CreatePaymentAppManager();
+  }
+
+  void TearDown() override {

[please use gerrit instead, 2016/11/15 20:04:27]

TearDown is usually not used. You can put all these function calls in the
destructor.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

[zino, 2016/11/16 18:12:58]

Done.

+    payment_app_context_->Shutdown();
+    base::RunLoop().RunUntilIdle();
+    payment_app_context_ = nullptr;

[please use gerrit instead, 2016/11/15 20:04:27]

Clearing the scoped_refptr should not be necessary immediately before its
destruction. (The googletest framework does not reuse instances of this class.)

[zino, 2016/11/16 18:12:58]

Done.

+  }
+
+  void SetManifest(const std::string& scope,
+                   payments::mojom::PaymentAppManifestPtr manifest,
+                   const PaymentAppManager::SetManifestCallback& callback) {
+    manager_->SetManifest(scope, std::move(manifest), callback);
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void ReadManifestData(std::vector<std::string>* out_data) {
+    embedded_worker_helper_->context_wrapper()->GetRegistrationUserData(
+        sw_registration_id_, {{kPaymentAppManifestDataKey}},
+        base::Bind(&ReadManifestDataCallback, out_data));
+    base::RunLoop().RunUntilIdle();
+  }
+
+ private:
+  void CreateTestHelper() {
+    embedded_worker_helper_.reset(
+        new EmbeddedWorkerTestHelper(base::FilePath()));

[please use gerrit instead, 2016/11/15 20:04:27]

When a function is used only once, it's generally preferred to place its
contents directly at the call site (SetUp) instead. (I usually call this copying
"inlining", but it's not the same as the "inline" keyword, which is confusing,
sorry.)

Furthermore, if you follow my advice of initializing in the constructor instead
of the constructor, then you can initialize embedded_worker_helper_ even simpler
like so:

PaymentAppManagerTest()
    : thread_bundle_(
          new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
      embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())) {}

[zino, 2016/11/16 18:12:59]

Done.

+  }
+
+  void CreateStoragePartition() {
+    storage_partition_impl_.reset(new StoragePartitionImpl(
+        embedded_worker_helper_->browser_context(), base::FilePath(), nullptr,
+        nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+        nullptr, nullptr, nullptr, nullptr, nullptr));
+    embedded_worker_helper_->context_wrapper()->set_storage_partition(
+        storage_partition_impl_.get());

[please use gerrit instead, 2016/11/15 20:04:27]

Also can be "inlined".

[zino, 2016/11/16 18:12:59]

Done.

+  }
+
+  void CreatePaymentAppContext() {
+    payment_app_context_ =
+        new PaymentAppContext(embedded_worker_helper_->context_wrapper());

[please use gerrit instead, 2016/11/15 20:04:27]

Also can be "inlined".

[zino, 2016/11/16 18:12:59]

Done.

+  }
+
+  void CreateServiceWorkerRegistration() {
+    bool called = false;
+    embedded_worker_helper_->context()->RegisterServiceWorker(
+        GURL(kServiceWorkerPattern), GURL(kServiceWorkerScript), NULL,
+        base::Bind(&RegisterServiceWorkerCallback, &called,
+                   &sw_registration_id_));
+    base::RunLoop().RunUntilIdle();

[please use gerrit instead, 2016/11/15 20:04:27]

Is there no way to avoid running the loop?

[zino, 2016/11/16 18:12:59]

The RegisterServiceWorker() will run asynchronously. So, this is needed to make
this test easier.
Background Sync and Many Service Worker tests are already using it.

+    EXPECT_TRUE(called);

[please use gerrit instead, 2016/11/15 20:04:27]

Also can be "inlined".

[zino, 2016/11/16 18:12:59]

Done.

+  }
+
+  void CreatePaymentAppManager() {
+    mojo::InterfaceRequest<payments::mojom::PaymentAppManager> request =
+        mojo::GetProxy(&service_);
+    payment_app_context_->CreateService(std::move(request));
+    base::RunLoop().RunUntilIdle();

[please use gerrit instead, 2016/11/15 20:04:27]

Is there no way to avoid running the loop?

[zino, 2016/11/16 18:12:59]

ditto

+    manager_ = payment_app_context_->services_.begin()->first;
+    EXPECT_TRUE(manager_);

[please use gerrit instead, 2016/11/15 20:04:27]

Also can be "inlined".

[zino, 2016/11/16 18:12:59]

Done.

+  }
+
+  std::unique_ptr<TestBrowserThreadBundle> thread_bundle_;
+  std::unique_ptr<EmbeddedWorkerTestHelper> embedded_worker_helper_;
+  std::unique_ptr<StoragePartitionImpl> storage_partition_impl_;
+  int64_t sw_registration_id_;
+  scoped_refptr<PaymentAppContext> payment_app_context_;
+  payments::mojom::PaymentAppManagerPtr service_;
+  PaymentAppManager* manager_;

[please use gerrit instead, 2016/11/15 20:04:26]

Add a note about ownership of this pointer. If you own it, then it should be
std::unique_ptr.

[zino, 2016/11/16 18:12:58]

This class doesn't have the ownership of this pointer. PaymentAppContext owns
this.

+};

[please use gerrit instead, 2016/11/15 20:04:26]

DISALLOW_COPY_AND_ASSIGN is always a good idea.

[zino, 2016/11/16 18:12:58]

Done.

+
+TEST_F(PaymentAppManagerTest, SetManifest) {
+  payments::mojom::PaymentAppOptionPtr option =
+      payments::mojom::PaymentAppOption::New();
+  option->label = "Visa ****";
+  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;
+  SetManifest(kServiceWorkerPattern, std::move(manifest),
+              base::Bind(&SetManifestCallback, &error));
+
+  EXPECT_EQ(error, payments::mojom::PaymentAppManifestError::NONE);
+
+  std::vector<std::string> data;
+  ReadManifestData(&data);
+  EXPECT_EQ(data.size(), 1UL);

[please use gerrit instead, 2016/11/15 20:04:27]

ASSERT_EQ

If the data size is not 1UL, you want to terminate the test, so use ASSERT_EQ
instead of EXPECT_EQ for this check. If you don't terminate the test, data[0] on
line 164 will crash it. Crashing tests are harder to debug.

[zino, 2016/11/16 18:12:58]

Done.

+  EXPECT_EQ(
+      "\n\vPayment App\x12\x10payment-app-icon\x1A"
+      "3\n\tVisa ****\x12\x10payment-app-icon\x1A\xEpayment-app-id\"\x4visa",
+      data[0]);
+}
+
+}  // namespace content