PaymentApp: Implement GetAllManifests() in PaymentAppContext.

content/browser/payments/payment_app_content_unittest_base.cc

Index: content/browser/payments/payment_app_content_unittest_base.cc
diff --git a/content/browser/payments/payment_app_content_unittest_base.cc b/content/browser/payments/payment_app_content_unittest_base.cc
new file mode 100644
index 0000000000000000000000000000000000000000..79045aaef1063f0056bceba9df8b50ab02943ceb
--- /dev/null
+++ b/content/browser/payments/payment_app_content_unittest_base.cc
@@ -0,0 +1,114 @@
+// 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_content_unittest_base.h"
+
+#include <set>
+#include <utility>
+
+#include "base/run_loop.h"
+#include "content/browser/service_worker/embedded_worker_test_helper.h"
+#include "content/browser/service_worker/service_worker_context_wrapper.h"

[please use gerrit instead, 2016/12/16 21:41:21]

You don't use ServiceWorkerContextWrapper anywhere here.

[zino, 2016/12/17 17:11:54]

Used it here:
embedded_worker_helper_->context_wrapper()->set_storage_partition()

[please use gerrit instead, 2016/12/19 14:48:05]

Acknowledged.

+#include "content/browser/storage_partition_impl.h"
+#include "content/public/test/test_browser_context.h"

[please use gerrit instead, 2016/12/16 21:41:21]

You don't use TestBrowserContext anywhere here/.

[zino, 2016/12/17 17:11:54]

Used it here:
new StoragePartitionImpl(embedded_worker_helper_->browser_context(), ...)

[please use gerrit instead, 2016/12/19 14:48:05]

Calling browser_context() without dereferencing the returned pointer should not
require an include, IIRC.

+#include "content/public/test/test_browser_thread_bundle.h"
+
+namespace content {
+
+namespace {
+
+void RegisterServiceWorkerCallback(bool* called,
+                                   ServiceWorkerStatusCode status,

[please use gerrit instead, 2016/12/16 21:41:21]

#include "content/common/service_worker/service_worker_status_code.h"

[zino, 2016/12/17 17:11:53]

Done.

+                                   const std::string& status_message,
+                                   int64_t registration_id) {

[please use gerrit instead, 2016/12/16 21:41:21]

#include <stdint.h>

[zino, 2016/12/17 17:11:53]

Done.

+  EXPECT_EQ(SERVICE_WORKER_OK, status) << ServiceWorkerStatusToString(status);
+  *called = true;
+}
+
+}  // namespace
+
+PaymentAppContentUnitTestBase::PaymentAppContentUnitTestBase()
+    : thread_bundle_(
+          new TestBrowserThreadBundle(TestBrowserThreadBundle::IO_MAINLOOP)),
+      embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())),

[please use gerrit instead, 2016/12/16 21:41:20]

#include "base/files/file_path.h"

[zino, 2016/12/17 17:11:54]

Done.

+      storage_partition_impl_(
+          new StoragePartitionImpl(embedded_worker_helper_->browser_context(),
+                                   base::FilePath(),
+                                   nullptr)),
+      payment_app_context_(new PaymentAppContextImpl()) {
+  embedded_worker_helper_->context_wrapper()->set_storage_partition(
+      storage_partition_impl_.get());
+  payment_app_context_->Init(embedded_worker_helper_->context_wrapper());

[please use gerrit instead, 2016/12/16 21:41:21]

You should RunUntilIdle here, because Init() posts a task to IO thread.

[zino, 2016/12/17 17:11:53]

Done.

+}
+
+PaymentAppContentUnitTestBase::~PaymentAppContentUnitTestBase() {
+  payment_app_context_->Shutdown();
+  base::RunLoop().RunUntilIdle();
+}
+
+PaymentAppContextImpl* PaymentAppContentUnitTestBase::GetPaymentAppContext()
+    const {
+  return payment_app_context_.get();
+}
+
+PaymentAppManager* PaymentAppContentUnitTestBase::CreatePaymentAppManager(
+    const GURL& scope_url,
+    const GURL& sw_script_url) {
+  // Register service worker for payment app manager.
+  bool called = false;
+  embedded_worker_helper_->context()->RegisterServiceWorker(
+      scope_url, sw_script_url, nullptr,
+      base::Bind(&RegisterServiceWorkerCallback, &called));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+
+  // This function should eventually return created payment app manager
+  // but there is no way to get last created payment app manager from
+  // payment_app_context_->payment_app_managers_ because its type is std::map
+  // and can not ensure its order. So, just make a set of existing payment app
+  // managers before creating a new manager and then check what is a new thing.
+  std::set<PaymentAppManager*> existing_managers;
+  for (const auto& manager : payment_app_context_->payment_app_managers_) {
+    existing_managers.insert(manager.first);
+  }
+
+  // Create a new payment app manager.
+  payments::mojom::PaymentAppManagerPtr manager;

[please use gerrit instead, 2016/12/16 21:41:21]

You're using the name "manager" for both for-loop iteration and as a stack
variable here as well. This can get confusing and lead to bugs. Please use a
different name here.

[zino, 2016/12/17 17:11:53]

Done.

+  mojo::InterfaceRequest<payments::mojom::PaymentAppManager> request =

[please use gerrit instead, 2016/12/16 21:41:21]

#include "mojo/public/cpp/bindings/interface_request.h"

[zino, 2016/12/17 17:11:53]

Done.

+      mojo::GetProxy(&manager);

[please use gerrit instead, 2016/12/16 21:41:20]

#include "mojo/public/cpp/bindings/associated_interface_ptr.h"

[zino, 2016/12/17 17:11:54]

Done.

+  payment_app_managers_.push_back(std::move(manager));
+  payment_app_context_->CreatePaymentAppManager(std::move(request));
+  base::RunLoop().RunUntilIdle();
+
+  // Find a last registered payment app manager.
+  for (const auto& manager : payment_app_context_->payment_app_managers_) {
+    if (existing_managers.size() == 0 ||
+        existing_managers.find(manager.first) != existing_managers.end())

[please use gerrit instead, 2016/12/16 21:41:20]

Your comment and your code disagrees. Use this to match your comment:

if (existing_managers.find(manager.first) == existing_managers.end())

[zino, 2016/12/17 17:11:53]

You're right..
I used base::ContainsKey() instead.

Done.

+      return manager.first;
+  }
+
+  NOTREACHED();
+  return nullptr;
+}
+
+void PaymentAppContentUnitTestBase::SetManifest(
+    PaymentAppManager* manager,
+    const std::string& scope,
+    payments::mojom::PaymentAppManifestPtr manifest,
+    const PaymentAppManager::SetManifestCallback& callback) {
+  ASSERT_TRUE(manager);

[please use gerrit instead, 2016/12/16 21:41:21]

ASSERT_NE(nullptr, manager) is better, because |manager| is not a boolean.

[zino, 2016/12/17 17:11:53]

Done.

+  manager->SetManifest(scope, std::move(manifest), callback);
+  base::RunLoop().RunUntilIdle();
+}
+
+void PaymentAppContentUnitTestBase::GetManifest(
+    PaymentAppManager* manager,
+    const std::string& scope,
+    const PaymentAppManager::GetManifestCallback& callback) {
+  ASSERT_TRUE(manager);
+  manager->GetManifest(scope, callback);
+  base::RunLoop().RunUntilIdle();
+}
+
+}  // namespace content