[CrOS Tether] Refactor CrashRecoveryManager.

Now, CrashRecoveryManager is a pure virtual base class with a
CrashRecoveryManagerImpl implementation and a FakeCrashRecoveryManager
test double.

This will be used in a follow-up CL to improve the robustness of the
initialization flow.

Bug: 761532, 672263
Change-Id: I104a24e8c580bf7e5b767b7ab0af1aa7cd8f3f3d
Reviewed-on: https://chromium-review.googlesource.com/727184
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510201}

chromeos/components/tether/BUILD.gn

@@ -29,8 +29,9 @@ static_library("tether") {
     "ble_synchronizer_base.h",
     "connect_tethering_operation.cc",
     "connect_tethering_operation.h",
-    "crash_recovery_manager.cc",
     "crash_recovery_manager.h",
+    "crash_recovery_manager_impl.cc",
+    "crash_recovery_manager_impl.h",
     "device_id_tether_network_guid_map.cc",
     "device_id_tether_network_guid_map.h",
     "device_status_util.cc",
@@ -144,6 +145,8 @@ static_library("test_support") {
     "fake_ble_scanner.h",
     "fake_ble_synchronizer.cc",
     "fake_ble_synchronizer.h",
+    "fake_crash_recovery_manager.cc",
+    "fake_crash_recovery_manager.h",
     "fake_disconnect_tethering_request_sender.cc",
     "fake_disconnect_tethering_request_sender.h",
     "fake_error_tolerant_ble_advertisement.cc",
@@ -204,7 +207,7 @@ source_set("unit_tests") {
     "ble_scanner_impl_unittest.cc",
     "ble_synchronizer_unittest.cc",
     "connect_tethering_operation_unittest.cc",
-    "crash_recovery_manager_unittest.cc",
+    "crash_recovery_manager_impl_unittest.cc",
     "device_status_util_unittest.cc",
     "disconnect_tethering_operation_unittest.cc",
     "disconnect_tethering_request_sender_impl_unittest.cc",

chromeos/components/tether/active_host.h

@@ -140,7 +140,7 @@ class ActiveHost {
       const std::string& new_wifi_network_guid);
 
  private:
-  friend class CrashRecoveryManager;
+  friend class CrashRecoveryManagerImpl;
 
   void SetActiveHost(ActiveHostStatus active_host_status,
                      const std::string& active_host_device_id,

chromeos/components/tether/crash_recovery_manager.h

@@ -5,74 +5,30 @@
 #ifndef CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_H_
 #define CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_H_
 
-#include <string>
-
 #include "base/callback.h"
 #include "base/macros.h"
-#include "chromeos/components/tether/active_host.h"
 
 namespace chromeos {
 
-class NetworkStateHandler;
-
 namespace tether {
 
-class HostScanCache;
-
 // Restores Tether state after a browser crash.
 class CrashRecoveryManager {
  public:
-  class Factory {
-   public:
-    static std::unique_ptr<CrashRecoveryManager> NewInstance(
-        NetworkStateHandler* network_state_handler,
-        ActiveHost* active_host,
-        HostScanCache* host_scan_cache);
-
-    static void SetInstanceForTesting(Factory* factory);
-
-   protected:
-    virtual std::unique_ptr<CrashRecoveryManager> BuildInstance(
-        NetworkStateHandler* network_state_handler,
-        ActiveHost* active_host,
-        HostScanCache* host_scan_cache);
-
-   private:
-    static Factory* factory_instance_;
-  };
-
-  CrashRecoveryManager(NetworkStateHandler* network_state_handler,
-                       ActiveHost* active_host,
-                       HostScanCache* host_scan_cache);
-  virtual ~CrashRecoveryManager();
+  CrashRecoveryManager() {}
+  virtual ~CrashRecoveryManager() {}
 
   // Restores state which was lost by a browser crash. If a crash did not occur
-  // the last time that the Tether component was active, this function is a
-  // no-op. If there was an active Tether connection and the browser crashed,
-  // this function restores the Tether connection.
+  // the last time that TetherComponent was active, this function is a no-op.
+  // If there was an active Tether connection and the browser crashed, this
+  // function restores the Tether connection.
   //
-  // This function should only be called during the initialization of the Tether
-  // component.
-  void RestorePreCrashStateIfNecessary(
-      const base::Closure& on_restoration_finished);
+  // This function should only be called during the initialization of
+  // TetherComponent.
+  virtual void RestorePreCrashStateIfNecessary(
+      const base::Closure& on_restoration_finished) = 0;
 
  private:
-  void RestoreConnectedState(const base::Closure& on_restoration_finished,
-                             const std::string& active_host_device_id,
-                             const std::string& tether_network_guid,
-                             const std::string& wifi_network_guid);
-  void OnActiveHostFetched(const base::Closure& on_restoration_finished,
-                           ActiveHost::ActiveHostStatus active_host_status,
-                           std::shared_ptr<cryptauth::RemoteDevice> active_host,
-                           const std::string& tether_network_guid,
-                           const std::string& wifi_network_guid);
-
-  NetworkStateHandler* network_state_handler_;
-  ActiveHost* active_host_;
-  HostScanCache* host_scan_cache_;
-
-  base::WeakPtrFactory<CrashRecoveryManager> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManager);
 };
 

chromeos/components/tether/crash_recovery_manager.cc → chromeos/components/tether/crash_recovery_manager_impl.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/components/tether/crash_recovery_manager.h"
+#include "chromeos/components/tether/crash_recovery_manager_impl.h"
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
@@ -18,12 +18,12 @@ namespace chromeos {
 namespace tether {
 
 // static
-CrashRecoveryManager::Factory*
-    CrashRecoveryManager::Factory::factory_instance_ = nullptr;
+CrashRecoveryManagerImpl::Factory*
+    CrashRecoveryManagerImpl::Factory::factory_instance_ = nullptr;
 
 // static
 std::unique_ptr<CrashRecoveryManager>
-CrashRecoveryManager::Factory::NewInstance(
+CrashRecoveryManagerImpl::Factory::NewInstance(
     NetworkStateHandler* network_state_handler,
     ActiveHost* active_host,
     HostScanCache* host_scan_cache) {
@@ -35,20 +35,21 @@ CrashRecoveryManager::Factory::NewInstance(
 }
 
 // static
-void CrashRecoveryManager::Factory::SetInstanceForTesting(Factory* factory) {
+void CrashRecoveryManagerImpl::Factory::SetInstanceForTesting(
+    Factory* factory) {
   factory_instance_ = factory;
 }
 
 std::unique_ptr<CrashRecoveryManager>
-CrashRecoveryManager::Factory::BuildInstance(
+CrashRecoveryManagerImpl::Factory::BuildInstance(
     NetworkStateHandler* network_state_handler,
     ActiveHost* active_host,
     HostScanCache* host_scan_cache) {
-  return base::MakeUnique<CrashRecoveryManager>(network_state_handler,
-                                                active_host, host_scan_cache);
+  return base::MakeUnique<CrashRecoveryManagerImpl>(
+      network_state_handler, active_host, host_scan_cache);
 }
 
-CrashRecoveryManager::CrashRecoveryManager(
+CrashRecoveryManagerImpl::CrashRecoveryManagerImpl(
     NetworkStateHandler* network_state_handler,
     ActiveHost* active_host,
     HostScanCache* host_scan_cache)
@@ -57,9 +58,9 @@ CrashRecoveryManager::CrashRecoveryManager(
       host_scan_cache_(host_scan_cache),
       weak_ptr_factory_(this) {}
 
-CrashRecoveryManager::~CrashRecoveryManager() {}
+CrashRecoveryManagerImpl::~CrashRecoveryManagerImpl() {}
 
-void CrashRecoveryManager::RestorePreCrashStateIfNecessary(
+void CrashRecoveryManagerImpl::RestorePreCrashStateIfNecessary(
     const base::Closure& on_restoration_finished) {
   ActiveHost::ActiveHostStatus status = active_host_->GetActiveHostStatus();
   std::string active_host_device_id = active_host_->GetActiveHostDeviceId();
@@ -67,7 +68,7 @@ void CrashRecoveryManager::RestorePreCrashStateIfNecessary(
   std::string tether_network_guid = active_host_->GetTetherNetworkGuid();
 
   if (status == ActiveHost::ActiveHostStatus::DISCONNECTED) {
-    // There was no active Tether session, so either the last Tether component
+    // There was no active Tether session, so either the last TetherComponent
     // shutdown occurred normally (i.e., without a crash), or it occurred due
     // to a crash and there was no active host at the time of the crash.
     on_restoration_finished.Run();
@@ -92,7 +93,7 @@ void CrashRecoveryManager::RestorePreCrashStateIfNecessary(
                         tether_network_guid, wifi_network_guid);
 }
 
-void CrashRecoveryManager::RestoreConnectedState(
+void CrashRecoveryManagerImpl::RestoreConnectedState(
     const base::Closure& on_restoration_finished,
     const std::string& active_host_device_id,
     const std::string& tether_network_guid,
@@ -144,11 +145,11 @@ void CrashRecoveryManager::RestoreConnectedState(
       tether_network_guid, wifi_network_guid);
 
   active_host_->GetActiveHost(
-      base::Bind(&CrashRecoveryManager::OnActiveHostFetched,
+      base::Bind(&CrashRecoveryManagerImpl::OnActiveHostFetched,
                  weak_ptr_factory_.GetWeakPtr(), on_restoration_finished));
 }
 
-void CrashRecoveryManager::OnActiveHostFetched(
+void CrashRecoveryManagerImpl::OnActiveHostFetched(
     const base::Closure& on_restoration_finished,
     ActiveHost::ActiveHostStatus active_host_status,
     std::shared_ptr<cryptauth::RemoteDevice> active_host,
@@ -163,7 +164,8 @@ void CrashRecoveryManager::OnActiveHostFetched(
   // that there is an active connection.
   //
   // Note: SendActiveHostChangedUpdate() is a protected function of ActiveHost
-  // which is only invoked here because CrashRecoveryManager is a friend class.
+  // which is only invoked here because CrashRecoveryManagerImpl is a friend
+  // class.
   // It is invoked directly here because we are sending out a "fake" change
   // event which has equal old and new values.
   active_host_->SendActiveHostChangedUpdate(

chromeos/components/tether/crash_recovery_manager_impl.h

+// Copyright 2017 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.
+
+#ifndef CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_IMPL_H_
+#define CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_IMPL_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "chromeos/components/tether/active_host.h"
+#include "chromeos/components/tether/crash_recovery_manager.h"
+
+namespace chromeos {
+
+class NetworkStateHandler;
+
+namespace tether {
+
+class HostScanCache;
+
+// Concrete CrashRecoveryManager implementation.
+class CrashRecoveryManagerImpl : public CrashRecoveryManager {
+ public:
+  class Factory {
+   public:
+    static std::unique_ptr<CrashRecoveryManager> NewInstance(
+        NetworkStateHandler* network_state_handler,
+        ActiveHost* active_host,
+        HostScanCache* host_scan_cache);
+
+    static void SetInstanceForTesting(Factory* factory);
+
+   protected:
+    virtual std::unique_ptr<CrashRecoveryManager> BuildInstance(
+        NetworkStateHandler* network_state_handler,
+        ActiveHost* active_host,
+        HostScanCache* host_scan_cache);
+
+   private:
+    static Factory* factory_instance_;
+  };
+
+  CrashRecoveryManagerImpl(NetworkStateHandler* network_state_handler,
+                           ActiveHost* active_host,
+                           HostScanCache* host_scan_cache);
+  ~CrashRecoveryManagerImpl() override;
+
+  // CrashRecoveryManager:
+  void RestorePreCrashStateIfNecessary(
+      const base::Closure& on_restoration_finished) override;
+
+ private:
+  void RestoreConnectedState(const base::Closure& on_restoration_finished,
+                             const std::string& active_host_device_id,
+                             const std::string& tether_network_guid,
+                             const std::string& wifi_network_guid);
+  void OnActiveHostFetched(const base::Closure& on_restoration_finished,
+                           ActiveHost::ActiveHostStatus active_host_status,
+                           std::shared_ptr<cryptauth::RemoteDevice> active_host,
+                           const std::string& tether_network_guid,
+                           const std::string& wifi_network_guid);
+
+  NetworkStateHandler* network_state_handler_;
+  ActiveHost* active_host_;
+  HostScanCache* host_scan_cache_;
+
+  base::WeakPtrFactory<CrashRecoveryManagerImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManagerImpl);
+};
+
+}  // namespace tether
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_IMPL_H_

chromeos/components/tether/crash_recovery_manager_unittest.cc → chromeos/components/tether/crash_recovery_manager_impl_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/components/tether/crash_recovery_manager.h"
+#include "chromeos/components/tether/crash_recovery_manager_impl.h"
 
 #include <memory>
 #include <sstream>
@@ -44,11 +44,11 @@ std::string CreateConfigurationJsonString(bool is_connected) {
 
 }  // namespace
 
-class CrashRecoveryManagerTest : public NetworkStateTest {
+class CrashRecoveryManagerImplTest : public NetworkStateTest {
  protected:
-  CrashRecoveryManagerTest()
+  CrashRecoveryManagerImplTest()
       : test_device_(cryptauth::GenerateTestRemoteDevices(1u)[0]) {}
-  ~CrashRecoveryManagerTest() override {}
+  ~CrashRecoveryManagerImplTest() override {}
 
   void SetUp() override {
     DBusThreadManager::Initialize();
@@ -61,7 +61,7 @@ class CrashRecoveryManagerTest : public NetworkStateTest {
     fake_active_host_ = base::MakeUnique<FakeActiveHost>();
     fake_host_scan_cache_ = base::MakeUnique<FakeHostScanCache>();
 
-    crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManager>(
+    crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManagerImpl>(
         network_state_handler(), fake_active_host_.get(),
         fake_host_scan_cache_.get());
 
@@ -108,7 +108,7 @@ class CrashRecoveryManagerTest : public NetworkStateTest {
 
   void StartRestoration() {
     crash_recovery_manager_->RestorePreCrashStateIfNecessary(
-        base::Bind(&CrashRecoveryManagerTest::OnRestorationFinished,
+        base::Bind(&CrashRecoveryManagerImplTest::OnRestorationFinished,
                    base::Unretained(this)));
   }
 
@@ -132,18 +132,18 @@ class CrashRecoveryManagerTest : public NetworkStateTest {
 
   bool is_restoration_finished_;
 
-  std::unique_ptr<CrashRecoveryManager> crash_recovery_manager_;
+  std::unique_ptr<CrashRecoveryManagerImpl> crash_recovery_manager_;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManagerTest);
+  DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManagerImplTest);
 };
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostDisconnected) {
+TEST_F(CrashRecoveryManagerImplTest, ActiveHostDisconnected) {
   StartRestoration();
   VerifyDisconnectedAfterRestoration();
 }
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostConnecting) {
+TEST_F(CrashRecoveryManagerImplTest, ActiveHostConnecting) {
   fake_active_host_->SetActiveHostConnecting(test_device_.GetDeviceId(),
                                              GetTetherNetworkGuid());
 
@@ -151,14 +151,14 @@ TEST_F(CrashRecoveryManagerTest, ActiveHostConnecting) {
   VerifyDisconnectedAfterRestoration();
 }
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_EntryNotInCache) {
+TEST_F(CrashRecoveryManagerImplTest, ActiveHostConnected_EntryNotInCache) {
   SetConnected();
 
   StartRestoration();
   VerifyDisconnectedAfterRestoration();
 }
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkMissing) {
+TEST_F(CrashRecoveryManagerImplTest, ActiveHostConnected_WifiNetworkMissing) {
   AddScanEntry();
   SetConnected();
 
@@ -166,7 +166,8 @@ TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkMissing) {
   VerifyDisconnectedAfterRestoration();
 }
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkDisconnected) {
+TEST_F(CrashRecoveryManagerImplTest,
+       ActiveHostConnected_WifiNetworkDisconnected) {
   AddScanEntry();
   AddWifiNetwork(false /* is_connected */);
   SetConnected();
@@ -175,7 +176,7 @@ TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkDisconnected) {
   VerifyDisconnectedAfterRestoration();
 }
 
-TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_RestoreSuccessful) {
+TEST_F(CrashRecoveryManagerImplTest, ActiveHostConnected_RestoreSuccessful) {
   AddScanEntry();
   AddWifiNetwork(true /* is_connected */);
   SetConnected();

chromeos/components/tether/fake_crash_recovery_manager.cc

+// Copyright 2017 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/components/tether/fake_crash_recovery_manager.h"
+
+namespace chromeos {
+
+namespace tether {
+
+FakeCrashRecoveryManager::FakeCrashRecoveryManager() {}
+
+FakeCrashRecoveryManager::~FakeCrashRecoveryManager() {}
+
+void FakeCrashRecoveryManager::RestorePreCrashStateIfNecessary(
+    const base::Closure& on_restoration_finished) {
+  on_restoration_finished_callback_ = on_restoration_finished;
+}
+
+}  // namespace tether
+
+}  // namespace chromeos

chromeos/components/tether/fake_crash_recovery_manager.h

+// Copyright 2017 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.
+
+#ifndef CHROMEOS_COMPONENTS_TETHER_FAKE_CRASH_RECOVERY_MANAGER_H_
+#define CHROMEOS_COMPONENTS_TETHER_FAKE_CRASH_RECOVERY_MANAGER_H_
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "chromeos/components/tether/crash_recovery_manager.h"
+
+namespace chromeos {
+
+namespace tether {
+
+// Test double for CrashRecoveryManager.
+class FakeCrashRecoveryManager : public CrashRecoveryManager {
+ public:
+  FakeCrashRecoveryManager();
+  ~FakeCrashRecoveryManager() override;
+
+  base::Closure& on_restoration_finished_callback() {
+    return on_restoration_finished_callback_;
+  }
+
+  // CrashRecoveryManager:
+  void RestorePreCrashStateIfNecessary(
+      const base::Closure& on_restoration_finished) override;
+
+ private:
+  base::Closure on_restoration_finished_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeCrashRecoveryManager);
+};
+
+}  // namespace tether
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_TETHER_FAKE_CRASH_RECOVERY_MANAGER_H_

chromeos/components/tether/tether_component_impl.cc

@@ -12,7 +12,7 @@
 #include "chromeos/components/tether/ble_connection_manager.h"
 #include "chromeos/components/tether/ble_scanner_impl.h"
 #include "chromeos/components/tether/ble_synchronizer.h"
-#include "chromeos/components/tether/crash_recovery_manager.h"
+#include "chromeos/components/tether/crash_recovery_manager_impl.h"
 #include "chromeos/components/tether/device_id_tether_network_guid_map.h"
 #include "chromeos/components/tether/disconnect_tethering_request_sender.h"
 #include "chromeos/components/tether/disconnect_tethering_request_sender_impl.h"
@@ -275,7 +275,7 @@ void TetherComponentImpl::CreateComponent() {
           network_connection_handler_, active_host_.get(),
           tether_connector_.get(), tether_disconnector_.get());
 
-  crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManager>(
+  crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManagerImpl>(
       network_state_handler_, active_host_.get(),
       master_host_scan_cache_.get());
   crash_recovery_manager_->RestorePreCrashStateIfNecessary(