[CrOS Cellular] Implement CellularSetup::StartActivation().

This CL hooks up the StartActivation() call with OtaActivatorImpl. With
this CL, the flow is ready to be used by the WebUI dialog.

This CL also adds virtual inheritance for FakeCarrierPortalHandler to
ensure that we avoid the "diamond problem" for multiple inheritance.

Bug: 961084
Change-Id: I79e0e583d6ed3fade8961f6c82241020394945e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1629788
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Auto-Submit: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666343}

chromeos/services/cellular_setup/BUILD.gn

@@ -48,14 +48,17 @@ source_set("unit_tests") {
   testonly = true
 
   sources = [
+    "cellular_setup_impl_unittest.cc",
     "cellular_setup_service_unittest.cc",
     "ota_activator_impl_unittest.cc",
   ]
 
   deps = [
     ":cellular_setup",
+    ":test_support",
     "//base",
     "//base/test:test_support",
+    "//chromeos/dbus/shill",
     "//chromeos/network:test_support",
     "//chromeos/services/cellular_setup/public/cpp:test_support",
     "//services/service_manager/public/cpp/test:test_support",

chromeos/services/cellular_setup/cellular_setup_impl.cc

@@ -4,8 +4,13 @@
 
 #include "chromeos/services/cellular_setup/cellular_setup_impl.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/no_destructor.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/services/cellular_setup/ota_activator_impl.h"
 
 namespace chromeos {
 
@@ -39,9 +44,29 @@ CellularSetupImpl::~CellularSetupImpl() = default;
 void CellularSetupImpl::StartActivation(
     mojom::ActivationDelegatePtr delegate,
     StartActivationCallback callback) {
-  // TODO(khorimoto): Actually return a CarrierPortalObserver instead of
-  // passing null.
-  std::move(callback).Run(nullptr /* observer */);
+  size_t request_id = next_request_id_;
+  ++next_request_id_;
+
+  NetworkHandler* network_handler = NetworkHandler::Get();
+  std::unique_ptr<OtaActivator> ota_activator =
+      OtaActivatorImpl::Factory::Create(
+          std::move(delegate),
+          base::BindOnce(&CellularSetupImpl::OnActivationAttemptFinished,
+                         base::Unretained(this), request_id),
+          network_handler->network_state_handler(),
+          network_handler->network_connection_handler(),
+          network_handler->network_activation_handler());
+
+  std::move(callback).Run(ota_activator->GenerateInterfacePtr());
+
+  // Store the OtaActivator instance in a map indexed by request ID; once the
+  // attempt has finished, the map entry will be deleted in
+  // OnActivationAttemptFinished().
+  ota_activator_map_.AddWithID(std::move(ota_activator), request_id);
+}
+
+void CellularSetupImpl::OnActivationAttemptFinished(size_t request_id) {
+  ota_activator_map_.Remove(request_id);
 }
 
 }  // namespace cellular_setup

chromeos/services/cellular_setup/cellular_setup_impl.h

@@ -5,6 +5,9 @@
 #ifndef CHROMEOS_SERVICES_CELLULAR_SETUP_CELLULAR_SETUP_IMPL_H_
 #define CHROMEOS_SERVICES_CELLULAR_SETUP_CELLULAR_SETUP_IMPL_H_
 
+#include <memory>
+
+#include "base/containers/id_map.h"
 #include "base/macros.h"
 #include "chromeos/services/cellular_setup/cellular_setup_base.h"
 
@@ -12,7 +15,11 @@ namespace chromeos {
 
 namespace cellular_setup {
 
-// Concrete mojom::CellularSetup implementation.
+class OtaActivator;
+
+// Concrete mojom::CellularSetup implementation. This class creates a new
+// OtaActivator instance per each StartActivation() invocation and passes a
+// pointer back to the client.
 class CellularSetupImpl : public CellularSetupBase {
  public:
   class Factory {
@@ -32,6 +39,11 @@ class CellularSetupImpl : public CellularSetupBase {
   void StartActivation(mojom::ActivationDelegatePtr delegate,
                        StartActivationCallback callback) override;
 
+  void OnActivationAttemptFinished(size_t request_id);
+
+  size_t next_request_id_ = 0u;
+  base::IDMap<std::unique_ptr<OtaActivator>, size_t> ota_activator_map_;
+
   DISALLOW_COPY_AND_ASSIGN(CellularSetupImpl);
 };
 

chromeos/services/cellular_setup/cellular_setup_impl_unittest.cc

+// Copyright 2019 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 "chromeos/services/cellular_setup/cellular_setup_impl.h"
+
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "chromeos/dbus/shill/shill_clients.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/services/cellular_setup/cellular_setup_base.h"
+#include "chromeos/services/cellular_setup/cellular_setup_impl.h"
+#include "chromeos/services/cellular_setup/fake_ota_activator.h"
+#include "chromeos/services/cellular_setup/ota_activator_impl.h"
+#include "chromeos/services/cellular_setup/public/cpp/fake_activation_delegate.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace cellular_setup {
+
+namespace {
+
+class FakeOtaActivatorFactory : public OtaActivatorImpl::Factory {
+ public:
+  FakeOtaActivatorFactory() = default;
+  ~FakeOtaActivatorFactory() override = default;
+
+  std::vector<FakeOtaActivator*>& created_instances() {
+    return created_instances_;
+  }
+
+ private:
+  // OtaActivatorImpl::Factory:
+  std::unique_ptr<OtaActivator> BuildInstance(
+      mojom::ActivationDelegatePtr activation_delegate,
+      base::OnceClosure on_finished_callback,
+      NetworkStateHandler* network_state_handler,
+      NetworkConnectionHandler* network_connection_handler,
+      NetworkActivationHandler* network_activation_handler) override {
+    EXPECT_TRUE(activation_delegate);
+    EXPECT_TRUE(network_state_handler);
+    EXPECT_TRUE(network_connection_handler);
+    EXPECT_TRUE(network_activation_handler);
+
+    auto fake_ota_activator =
+        std::make_unique<FakeOtaActivator>(std::move(on_finished_callback));
+    created_instances_.push_back(fake_ota_activator.get());
+
+    return fake_ota_activator;
+  }
+
+  std::vector<FakeOtaActivator*> created_instances_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeOtaActivatorFactory);
+};
+
+}  // namespace
+
+class CellularSetupImplTest : public testing::Test {
+ protected:
+  CellularSetupImplTest() = default;
+  ~CellularSetupImplTest() override = default;
+
+  // testing::Test:
+  void SetUp() override {
+    OtaActivatorImpl::Factory::SetFactoryForTesting(
+        &fake_ota_activator_factory_);
+    shill_clients::InitializeFakes();
+    NetworkHandler::Initialize();
+    cellular_setup_ = CellularSetupImpl::Factory::Create();
+  }
+
+  void TearDown() override {
+    cellular_setup_.reset();
+    NetworkHandler::Shutdown();
+    shill_clients::Shutdown();
+    OtaActivatorImpl::Factory::SetFactoryForTesting(nullptr);
+  }
+
+  void CallStartActivation(FakeActivationDelegate* fake_activation_delegate) {
+    size_t num_before_call = num_carrier_portal_handlers_received_;
+    EXPECT_EQ(num_before_call,
+              fake_ota_activator_factory_.created_instances().size());
+
+    base::RunLoop run_loop;
+    cellular_setup_->StartActivation(
+        fake_activation_delegate->GenerateInterfacePtr(),
+        base::BindOnce(&CellularSetupImplTest::OnCarrierPortalHandlerReceived,
+                       base::Unretained(this), run_loop.QuitClosure()));
+    run_loop.Run();
+
+    EXPECT_EQ(num_before_call + 1u, num_carrier_portal_handlers_received_);
+    EXPECT_EQ(num_before_call + 1u,
+              fake_ota_activator_factory_.created_instances().size());
+
+    fake_ota_activator_factory_.created_instances()[num_before_call]
+        ->InvokeOnFinishedCallback();
+  }
+
+ private:
+  void OnCarrierPortalHandlerReceived(
+      base::OnceClosure quit_closure,
+      mojom::CarrierPortalHandlerPtr carrier_portal_handler) {
+    ++num_carrier_portal_handlers_received_;
+    std::move(quit_closure).Run();
+  }
+
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  FakeOtaActivatorFactory fake_ota_activator_factory_;
+
+  std::unique_ptr<CellularSetupBase> cellular_setup_;
+
+  size_t num_carrier_portal_handlers_received_ = 0u;
+
+  DISALLOW_COPY_AND_ASSIGN(CellularSetupImplTest);
+};
+
+TEST_F(CellularSetupImplTest, StartActivation_SingleAttempt) {
+  auto fake_activation_delegate = std::make_unique<FakeActivationDelegate>();
+  CallStartActivation(fake_activation_delegate.get());
+}
+
+TEST_F(CellularSetupImplTest, StartActivation_MultipleAttempts) {
+  auto fake_activation_delegate_1 = std::make_unique<FakeActivationDelegate>();
+  CallStartActivation(fake_activation_delegate_1.get());
+
+  auto fake_activation_delegate_2 = std::make_unique<FakeActivationDelegate>();
+  CallStartActivation(fake_activation_delegate_2.get());
+}
+
+}  // namespace cellular_setup
+
+}  // namespace chromeos

chromeos/services/cellular_setup/fake_ota_activator.cc

@@ -15,6 +15,11 @@ FakeOtaActivator::FakeOtaActivator(base::OnceClosure on_finished_callback)
 
 FakeOtaActivator::~FakeOtaActivator() = default;
 
+void FakeOtaActivator::OnCarrierPortalStatusChange(
+    mojom::CarrierPortalStatus status) {
+  fake_carrier_portal_handler_.OnCarrierPortalStatusChange(status);
+}
+
 }  // namespace cellular_setup
 
 }  // namespace chromeos

chromeos/services/cellular_setup/fake_ota_activator.h

@@ -15,14 +15,23 @@ namespace chromeos {
 namespace cellular_setup {
 
 // Test OtaActivator implementation.
-class FakeOtaActivator : public OtaActivator, public FakeCarrierPortalHandler {
+class FakeOtaActivator : public OtaActivator {
  public:
   explicit FakeOtaActivator(base::OnceClosure on_finished_callback);
   ~FakeOtaActivator() override;
 
   using OtaActivator::InvokeOnFinishedCallback;
 
+  const std::vector<mojom::CarrierPortalStatus>& status_updates() const {
+    return fake_carrier_portal_handler_.status_updates();
+  }
+
  private:
+  // mojom::CarrierPortalHandler:
+  void OnCarrierPortalStatusChange(mojom::CarrierPortalStatus status) override;
+
+  FakeCarrierPortalHandler fake_carrier_portal_handler_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeOtaActivator);
 };
 

chromeos/services/cellular_setup/public/cpp/fake_carrier_portal_handler.h

@@ -27,13 +27,12 @@ class FakeCarrierPortalHandler : public mojom::CarrierPortalHandler {
     return status_updates_;
   }
 
- private:
   // mojom::CarrierPortalHandler:
   void OnCarrierPortalStatusChange(
       mojom::CarrierPortalStatus carrier_portal_status) override;
 
+ private:
   std::vector<mojom::CarrierPortalStatus> status_updates_;
-
   mojo::BindingSet<mojom::CarrierPortalHandler> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeCarrierPortalHandler);