Extract HammerdClient from DbusThreadManager

Bug: 938717
Change-Id: Ied6a40e873849bdd91d499fe1a9093752d144763
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504500
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638326}

ash/DEPS

@@ -62,6 +62,7 @@ include_rules = [
   "+chromeos/dbus/dbus_thread_manager.h",
   "+chromeos/dbus/fake_power_manager_client.h",
   "+chromeos/dbus/fake_session_manager_client.h",
+  "+chromeos/dbus/hammerd",
   "+chromeos/dbus/power_manager",
   "+chromeos/dbus/power_manager_client.h",
   "+chromeos/dbus/power_policy_controller.h",

ash/ash_service.cc

@@ -17,6 +17,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 #include "chromeos/dbus/power_policy_controller.h"
 #include "chromeos/dbus/system_clock/system_clock_client.h"
 #include "chromeos/network/network_connect.h"
@@ -80,15 +81,19 @@ AshService::~AshService() {
   Shell::DeleteInstance();
 
   statistics_provider_.reset();
-  // NOTE: PowerStatus is shutdown by Shell.
-  chromeos::CrasAudioHandler::Shutdown();
+
   chromeos::NetworkConnect::Shutdown();
   network_connect_delegate_.reset();
   chromeos::NetworkHandler::Shutdown();
   device::BluetoothAdapterFactory::Shutdown();
   bluez::BluezDBusManager::Shutdown();
   chromeos::PowerPolicyController::Shutdown();
+
+  chromeos::SystemClockClient::Shutdown();
   chromeos::PowerManagerClient::Shutdown();
+  chromeos::HammerdClient::Shutdown();
+  chromeos::CrasAudioHandler::Shutdown();
+
   chromeos::DBusThreadManager::Shutdown();
 
   // |gpu_host_| must be completely destroyed before Env as GpuHost depends on
@@ -154,9 +159,13 @@ void AshService::InitializeDBusClients() {
   // dbus::Thread, dbus::Bus and required clients directly.
   chromeos::DBusThreadManager::Initialize(chromeos::DBusThreadManager::kShared);
   dbus::Bus* bus = chromeos::DBusThreadManager::Get()->GetSystemBus();
-  chromeos::SystemClockClient::Initialize(bus);
 
+  // TODO(jamescook): Initialize real audio handler.
+  chromeos::CrasAudioHandler::InitializeForTesting();
+  chromeos::HammerdClient::Initialize(bus);
   chromeos::PowerManagerClient::Initialize(bus);
+  chromeos::SystemClockClient::Initialize(bus);
+
   chromeos::PowerPolicyController::Initialize(
       chromeos::PowerManagerClient::Get());
 
@@ -168,9 +177,6 @@ void AshService::InitializeDBusClients() {
   chromeos::NetworkHandler::Initialize();
   network_connect_delegate_ = std::make_unique<NetworkConnectDelegateMus>();
   chromeos::NetworkConnect::Initialize(network_connect_delegate_.get());
-
-  // TODO(jamescook): Initialize real audio handler.
-  chromeos::CrasAudioHandler::InitializeForTesting();
 }
 
 void AshService::OnStart() {

ash/detachable_base/detachable_base_handler.cc

@@ -55,7 +55,8 @@ DetachableBaseHandler::DetachableBaseHandler(Shell* shell)
   if (shell_)
     shell_->AddShellObserver(this);
 
-  hammerd_observer_.Add(chromeos::DBusThreadManager::Get()->GetHammerdClient());
+  if (chromeos::HammerdClient::Get())  // May be null in tests
+    hammerd_observer_.Add(chromeos::HammerdClient::Get());
   chromeos::PowerManagerClient* power_manager_client =
       chromeos::PowerManagerClient::Get();
   power_manager_observer_.Add(power_manager_client);

ash/detachable_base/detachable_base_handler.h

@@ -18,7 +18,7 @@
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"
-#include "chromeos/dbus/hammerd_client.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 #include "chromeos/dbus/power_manager_client.h"
 
 class PrefRegistrySimple;

ash/detachable_base/detachable_base_handler_unittest.cc

@@ -14,9 +14,8 @@
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/time/time.h"
-#include "chromeos/dbus/dbus_thread_manager.h"
-#include "chromeos/dbus/fake_hammerd_client.h"
 #include "chromeos/dbus/fake_power_manager_client.h"
+#include "chromeos/dbus/hammerd/fake_hammerd_client.h"
 #include "components/account_id/account_id.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -86,12 +85,8 @@ class DetachableBaseHandlerTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
-    std::unique_ptr<chromeos::DBusThreadManagerSetter> dbus_setter =
-        chromeos::DBusThreadManager::GetSetterForTesting();
-
-    auto hammerd_client = std::make_unique<chromeos::FakeHammerdClient>();
-    hammerd_client_ = hammerd_client.get();
-    dbus_setter->SetHammerdClient(std::move(hammerd_client));
+    chromeos::HammerdClient::Initialize(nullptr /* bus */);
+    hammerd_client_ = chromeos::FakeHammerdClient::Get();
 
     chromeos::PowerManagerClient::Initialize();
     chromeos::FakePowerManagerClient::Get()->SetTabletMode(
@@ -110,7 +105,7 @@ class DetachableBaseHandlerTest : public testing::Test {
     handler_.reset();
     hammerd_client_ = nullptr;
     chromeos::PowerManagerClient::Shutdown();
-    chromeos::DBusThreadManager::Shutdown();
+    chromeos::HammerdClient::Shutdown();
   }
 
  protected:
@@ -142,7 +137,6 @@ class DetachableBaseHandlerTest : public testing::Test {
     handler_->OnLocalStatePrefServiceInitialized(&local_state_);
   }
 
-  // Owned by DBusThreadManager:
   chromeos::FakeHammerdClient* hammerd_client_ = nullptr;
 
   TestBaseObserver detachable_base_observer_;

ash/test/ash_test_helper.cc

@@ -31,6 +31,7 @@
 #include "base/token.h"
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 #include "chromeos/dbus/power_policy_controller.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/system/fake_statistics_provider.h"
@@ -160,6 +161,8 @@ void AshTestHelper::SetUp(bool start_session, bool provide_local_state) {
   if (!chromeos::DBusThreadManager::IsInitialized()) {
     chromeos::DBusThreadManager::Initialize(
         chromeos::DBusThreadManager::kShared);
+    chromeos::HammerdClient::Initialize(
+        chromeos::DBusThreadManager::Get()->GetSystemBus());
     dbus_thread_manager_initialized_ = true;
   }
 
@@ -272,6 +275,7 @@ void AshTestHelper::TearDown() {
   }
 
   if (dbus_thread_manager_initialized_) {
+    chromeos::HammerdClient::Shutdown();
     chromeos::DBusThreadManager::Shutdown();
     dbus_thread_manager_initialized_ = false;
   }

chrome/browser/chromeos/dbus/dbus_helper.cc

@@ -7,9 +7,11 @@
 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chromeos/cryptohome/system_salt_getter.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "chromeos/dbus/system_clock/system_clock_client.h"
 #include "chromeos/tpm/install_attributes.h"
+#include "ui/base/ui_base_features.h"
 
 namespace chromeos {
 
@@ -21,6 +23,13 @@ void InitializeDBus() {
 
   // Initialize Chrome dbus clients.
   dbus::Bus* bus = DBusThreadManager::Get()->GetSystemBus();
+
+  // Features only needed in Ash. Initialize them here for non MultiProcessMash
+  // to limit the number of places where dbus handlers are initialized. For
+  // MultiProcessMash they are initialized in AshService::InitializeDBusClients.
+  if (!::features::IsMultiProcessMash())
+    chromeos::HammerdClient::Initialize(bus);
+
   PowerManagerClient::Initialize(bus);
   SystemClockClient::Initialize(bus);
 
@@ -32,9 +41,13 @@ void InitializeDBus() {
 }
 
 void ShutdownDBus() {
-  // NOTE: These must only be called if InitializeDBus() was called.
-  PowerManagerClient::Shutdown();
   SystemClockClient::Shutdown();
+  PowerManagerClient::Shutdown();
+
+  // See comment in InitializeDBus() for MultiProcessMash behavior.
+  if (!::features::IsMultiProcessMash())
+    chromeos::HammerdClient::Shutdown();
+
   DBusThreadManager::Shutdown();
   SystemSaltGetter::Shutdown();
 }

chromeos/dbus/BUILD.gn

@@ -119,8 +119,6 @@ component("dbus") {
     "fake_easy_unlock_client.h",
     "fake_gsm_sms_client.cc",
     "fake_gsm_sms_client.h",
-    "fake_hammerd_client.cc",
-    "fake_hammerd_client.h",
     "fake_image_burner_client.cc",
     "fake_image_burner_client.h",
     "fake_image_loader_client.cc",
@@ -169,8 +167,10 @@ component("dbus") {
     "fake_virtual_file_provider_client.h",
     "gsm_sms_client.cc",
     "gsm_sms_client.h",
-    "hammerd_client.cc",
-    "hammerd_client.h",
+    "hammerd/fake_hammerd_client.cc",
+    "hammerd/fake_hammerd_client.h",
+    "hammerd/hammerd_client.cc",
+    "hammerd/hammerd_client.h",
     "image_burner_client.cc",
     "image_burner_client.h",
     "image_loader_client.cc",

chromeos/dbus/dbus_clients_common.cc

@@ -15,7 +15,6 @@
 #include "chromeos/dbus/fake_cras_audio_client.h"
 #include "chromeos/dbus/fake_cryptohome_client.h"
 #include "chromeos/dbus/fake_gsm_sms_client.h"
-#include "chromeos/dbus/fake_hammerd_client.h"
 #include "chromeos/dbus/fake_modem_messaging_client.h"
 #include "chromeos/dbus/fake_permission_broker_client.h"
 #include "chromeos/dbus/fake_shill_device_client.h"
@@ -27,7 +26,6 @@
 #include "chromeos/dbus/fake_sms_client.h"
 #include "chromeos/dbus/fake_upstart_client.h"
 #include "chromeos/dbus/gsm_sms_client.h"
-#include "chromeos/dbus/hammerd_client.h"
 #include "chromeos/dbus/machine_learning_client.h"
 #include "chromeos/dbus/modem_messaging_client.h"
 #include "chromeos/dbus/permission_broker_client.h"
@@ -91,12 +89,6 @@ DBusClientsCommon::DBusClientsCommon(bool use_real_clients) {
     gsm_sms_client_.reset(gsm_sms_client);
   }
 
-  if (use_real_clients) {
-    hammerd_client_ = HammerdClient::Create();
-  } else {
-    hammerd_client_ = std::make_unique<FakeHammerdClient>();
-  }
-
   machine_learning_client_ = MachineLearningClient::Create(client_impl_type);
 
   if (use_real_clients)
@@ -134,7 +126,6 @@ void DBusClientsCommon::Initialize(dbus::Bus* system_bus) {
   cras_audio_client_->Init(system_bus);
   cryptohome_client_->Init(system_bus);
   gsm_sms_client_->Init(system_bus);
-  hammerd_client_->Init(system_bus);
   machine_learning_client_->Init(system_bus);
   modem_messaging_client_->Init(system_bus);
   permission_broker_client_->Init(system_bus);

chromeos/dbus/dbus_clients_common.h

@@ -21,7 +21,6 @@ class CecServiceClient;
 class CrasAudioClient;
 class CryptohomeClient;
 class GsmSMSClient;
-class HammerdClient;
 class MachineLearningClient;
 class ModemMessagingClient;
 class PermissionBrokerClient;
@@ -56,7 +55,6 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) DBusClientsCommon {
   std::unique_ptr<CrasAudioClient> cras_audio_client_;
   std::unique_ptr<CryptohomeClient> cryptohome_client_;
   std::unique_ptr<GsmSMSClient> gsm_sms_client_;
-  std::unique_ptr<HammerdClient> hammerd_client_;
   std::unique_ptr<MachineLearningClient> machine_learning_client_;
   std::unique_ptr<ModemMessagingClient> modem_messaging_client_;
   std::unique_ptr<ShillDeviceClient> shill_device_client_;

chromeos/dbus/dbus_thread_manager.cc

@@ -29,7 +29,6 @@
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/dbus/easy_unlock_client.h"
 #include "chromeos/dbus/gsm_sms_client.h"
-#include "chromeos/dbus/hammerd_client.h"
 #include "chromeos/dbus/image_burner_client.h"
 #include "chromeos/dbus/image_loader_client.h"
 #include "chromeos/dbus/lorgnette_manager_client.h"
@@ -215,10 +214,6 @@ GsmSMSClient* DBusThreadManager::GetGsmSMSClient() {
   return clients_common_->gsm_sms_client_.get();
 }
 
-HammerdClient* DBusThreadManager::GetHammerdClient() {
-  return clients_common_->hammerd_client_.get();
-}
-
 ImageBurnerClient* DBusThreadManager::GetImageBurnerClient() {
   return clients_browser_ ? clients_browser_->image_burner_client_.get()
                           : nullptr;
@@ -419,12 +414,6 @@ void DBusThreadManagerSetter::SetDebugDaemonClient(
       std::move(client);
 }
 
-void DBusThreadManagerSetter::SetHammerdClient(
-    std::unique_ptr<HammerdClient> client) {
-  DBusThreadManager::Get()->clients_common_->hammerd_client_ =
-      std::move(client);
-}
-
 void DBusThreadManagerSetter::SetRuntimeProbeClient(
     std::unique_ptr<RuntimeProbeClient> client) {
   DBusThreadManager::Get()->clients_browser_->runtime_probe_client_ =

chromeos/dbus/dbus_thread_manager.h

@@ -43,7 +43,6 @@ class DebugDaemonClient;
 class DiagnosticsdClient;
 class EasyUnlockClient;
 class GsmSMSClient;
-class HammerdClient;
 class ImageBurnerClient;
 class ImageLoaderClient;
 class LorgnetteManagerClient;
@@ -150,7 +149,6 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) DBusThreadManager {
   DiagnosticsdClient* GetDiagnosticsdClient();
   EasyUnlockClient* GetEasyUnlockClient();
   GsmSMSClient* GetGsmSMSClient();
-  HammerdClient* GetHammerdClient();
   ImageBurnerClient* GetImageBurnerClient();
   ImageLoaderClient* GetImageLoaderClient();
   LorgnetteManagerClient* GetLorgnetteManagerClient();
@@ -214,7 +212,6 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) DBusThreadManagerSetter {
   void SetCrosDisksClient(std::unique_ptr<CrosDisksClient> client);
   void SetCryptohomeClient(std::unique_ptr<CryptohomeClient> client);
   void SetDebugDaemonClient(std::unique_ptr<DebugDaemonClient> client);
-  void SetHammerdClient(std::unique_ptr<HammerdClient> client);
   void SetImageBurnerClient(std::unique_ptr<ImageBurnerClient> client);
   void SetImageLoaderClient(std::unique_ptr<ImageLoaderClient> client);
   void SetMediaAnalyticsClient(std::unique_ptr<MediaAnalyticsClient> client);

chromeos/dbus/fake_hammerd_client.cc → chromeos/dbus/hammerd/fake_hammerd_client.cc

@@ -2,15 +2,29 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chromeos/dbus/fake_hammerd_client.h"
+#include "chromeos/dbus/hammerd/fake_hammerd_client.h"
 
 namespace chromeos {
 
-FakeHammerdClient::FakeHammerdClient() = default;
+namespace {
+FakeHammerdClient* g_instance = nullptr;
+}
 
-FakeHammerdClient::~FakeHammerdClient() = default;
+FakeHammerdClient::FakeHammerdClient() {
+  CHECK(!g_instance);
+  g_instance = this;
+}
 
-void FakeHammerdClient::Init(dbus::Bus* bus) {}
+FakeHammerdClient::~FakeHammerdClient() {
+  CHECK_EQ(g_instance, this);
+  g_instance = nullptr;
+}
+
+// static
+FakeHammerdClient* FakeHammerdClient::Get() {
+  CHECK(g_instance);
+  return g_instance;
+}
 
 void FakeHammerdClient::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);

chromeos/dbus/fake_hammerd_client.h → chromeos/dbus/hammerd/fake_hammerd_client.h

@@ -2,15 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROMEOS_DBUS_FAKE_HAMMERD_CLIENT_H_
-#define CHROMEOS_DBUS_FAKE_HAMMERD_CLIENT_H_
+#ifndef CHROMEOS_DBUS_HAMMERD_FAKE_HAMMERD_CLIENT_H_
+#define CHROMEOS_DBUS_HAMMERD_FAKE_HAMMERD_CLIENT_H_
 
 #include <vector>
 
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
-#include "chromeos/dbus/hammerd_client.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 
 namespace chromeos {
 
@@ -19,8 +19,10 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeHammerdClient : public HammerdClient {
   FakeHammerdClient();
   ~FakeHammerdClient() override;
 
+  // Checks that a FakeHammerdClient instance was initialized and returns it.
+  static FakeHammerdClient* Get();
+
   // HammerdClient:
-  void Init(dbus::Bus* bus) override;
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
 
@@ -41,4 +43,4 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeHammerdClient : public HammerdClient {
 
 }  // namespace chromeos
 
-#endif  // CHROMEOS_DBUS_FAKE_HAMMERD_CLIENT_H_
+#endif  // CHROMEOS_DBUS_HAMMERD_FAKE_HAMMERD_CLIENT_H_

chromeos/dbus/hammerd_client.cc → chromeos/dbus/hammerd/hammerd_client.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/dbus/hammerd_client.h"
+#include "chromeos/dbus/hammerd/hammerd_client.h"
 
 #include <string>
 
@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "chromeos/dbus/hammerd/fake_hammerd_client.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
@@ -21,13 +22,15 @@ namespace chromeos {
 
 namespace {
 
+HammerdClient* g_instance = nullptr;
+
 class HammerdClientImpl : public HammerdClient {
  public:
   HammerdClientImpl() = default;
   ~HammerdClientImpl() override = default;
 
   // HammerdClient:
-  void Init(dbus::Bus* bus) override {
+  void Init(dbus::Bus* bus) {
     bus_proxy_ =
         bus->GetObjectProxy(hammerd::kHammerdServiceName,
                             dbus::ObjectPath(hammerd::kHammerdServicePath));
@@ -156,9 +159,32 @@ class HammerdClientImpl : public HammerdClient {
 
 }  // namespace
 
+HammerdClient::HammerdClient() = default;
+
+HammerdClient::~HammerdClient() = default;
+
+// static
+void HammerdClient::Initialize(dbus::Bus* bus) {
+  CHECK(!g_instance);
+  if (bus) {
+    auto* instance = new HammerdClientImpl();
+    instance->Init(bus);
+    g_instance = instance;
+  } else {
+    g_instance = new FakeHammerdClient();
+  }
+}
+
+// static
+void HammerdClient::Shutdown() {
+  CHECK(g_instance);
+  delete g_instance;
+  g_instance = nullptr;
+}
+
 // static
-std::unique_ptr<HammerdClient> HammerdClient::Create() {
-  return std::make_unique<HammerdClientImpl>();
+HammerdClient* HammerdClient::Get() {
+  return g_instance;
 }
 
 }  // namespace chromeos

chromeos/dbus/hammerd_client.h → chromeos/dbus/hammerd/hammerd_client.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROMEOS_DBUS_HAMMERD_CLIENT_H_
-#define CHROMEOS_DBUS_HAMMERD_CLIENT_H_
+#ifndef CHROMEOS_DBUS_HAMMERD_HAMMERD_CLIENT_H_
+#define CHROMEOS_DBUS_HAMMERD_HAMMERD_CLIENT_H_
 
 #include <stdint.h>
 
@@ -23,7 +23,7 @@ namespace chromeos {
 //  * the connected base pairing events.
 // The client forwards the received signals to its observers (together with any
 // data extracted from the signal object).
-class COMPONENT_EXPORT(CHROMEOS_DBUS) HammerdClient : public DBusClient {
+class COMPONENT_EXPORT(CHROMEOS_DBUS) HammerdClient {
  public:
   class Observer {
    public:
@@ -54,14 +54,19 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) HammerdClient : public DBusClient {
     virtual void InvalidBaseConnected() = 0;
   };
 
-  ~HammerdClient() override = default;
+  HammerdClient();
+  virtual ~HammerdClient();
+
+  // Creates and initializes the global instance. If |bus| is null, a
+  // FakeHammerdClient instance will be created.
+  static void Initialize(dbus::Bus* bus);
+  static void Shutdown();
+  static HammerdClient* Get();
 
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
-
-  static std::unique_ptr<HammerdClient> Create();
 };
 
 }  // namespace chromeos
 
-#endif  // CHROMEOS_DBUS_HAMMERD_CLIENT_H_
+#endif  // CHROMEOS_DBUS_HAMMERD_HAMMERD_CLIENT_H_