Merged FakedDBusThreadManager with DBusThreadManager.

chromeos/dbus/dbus_thread_manager.cc

Index: chromeos/dbus/dbus_thread_manager.cc
diff --git a/chromeos/dbus/dbus_thread_manager.cc b/chromeos/dbus/dbus_thread_manager.cc
index f2edfad992480b24ee8119dc329dc89d5be87192..65cdf96864bd5761dc9e9d5d42da93e8fd527685 100644
--- a/chromeos/dbus/dbus_thread_manager.cc
+++ b/chromeos/dbus/dbus_thread_manager.cc
@@ -24,7 +24,6 @@
 #include "chromeos/dbus/dbus_client_bundle.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/dbus/easy_unlock_client.h"
-#include "chromeos/dbus/fake_dbus_thread_manager.h"
 #include "chromeos/dbus/gsm_sms_client.h"
 #include "chromeos/dbus/image_burner_client.h"
 #include "chromeos/dbus/introspectable_client.h"
@@ -58,10 +57,9 @@ static DBusThreadManager* g_dbus_thread_manager_for_testing = NULL;
 DBusClientBundle::DBusClientTypeMask
     DBusThreadManager::unstub_client_mask_ = DBusClientBundle::NO_CLIENTS;
 
-// The DBusThreadManager implementation used in production.
-class DBusThreadManagerImpl : public DBusThreadManager {
- public:
-  DBusThreadManagerImpl() {
+DBusThreadManager::DBusThreadManager() {
+  dbus::statistics::Initialize();
+  if (!DBusThreadManager::IsUsingStub(DBusClientBundle::ALL_CLIENTS)) {
     // Create the D-Bus thread.
     base::Thread::Options thread_options;
     thread_options.message_loop_type = base::MessageLoop::TYPE_IO;
@@ -74,197 +72,269 @@ class DBusThreadManagerImpl : public DBusThreadManager {
     system_bus_options.connection_type = dbus::Bus::PRIVATE;
     system_bus_options.dbus_task_runner = dbus_thread_->message_loop_proxy();
     system_bus_ = new dbus::Bus(system_bus_options);
-
-    CreateDefaultClients();
   }
 
-  virtual ~DBusThreadManagerImpl() {
-    // PowerPolicyController's destructor depends on PowerManagerClient.
-    power_policy_controller_.reset();
+  CreateDefaultClients();
+}
+
+DBusThreadManager::~DBusThreadManager() {
+  // PowerPolicyController's destructor depends on PowerManagerClient.
+  power_policy_controller_.reset();
 
-    // Delete all D-Bus clients before shutting down the system bus.
-    client_bundle_.reset();
+  // Delete all D-Bus clients before shutting down the system bus.
+  client_bundle_.reset();
 
-    // Shut down the bus. During the browser shutdown, it's ok to shut down
-    // the bus synchronously.
+  // Shut down the bus. During the browser shutdown, it's ok to shut down
+  // the bus synchronously.
+  if (system_bus_)
     system_bus_->ShutdownOnDBusThreadAndBlock();
 
-    // Stop the D-Bus thread.
+  // Stop the D-Bus thread.
+  if (dbus_thread_)
     dbus_thread_->Stop();
-  }
 
-  void SetupDefaultEnvironment() {
-    return client_bundle_->SetupDefaultEnvironment();
-  }
+  dbus::statistics::Shutdown();
 
-  virtual dbus::Bus* GetSystemBus() OVERRIDE {
-    return system_bus_.get();
-  }
+  if (g_dbus_thread_manager == NULL)
+    return;  // Called form Shutdown() or local test instance.
 
-  virtual BluetoothAdapterClient* GetBluetoothAdapterClient() OVERRIDE {
-    return client_bundle_->bluetooth_adapter_client();
+  // There should never be both a global instance and a local instance.
+  CHECK(this == g_dbus_thread_manager);
+  if (g_dbus_thread_manager_for_testing) {
+    g_dbus_thread_manager = NULL;
+    g_dbus_thread_manager_for_testing = NULL;
+    VLOG(1) << "DBusThreadManager destroyed";
+  } else {
+    LOG(FATAL) << "~DBusThreadManager() called outside of Shutdown()";
   }
+}
 
-  virtual BluetoothAgentManagerClient* GetBluetoothAgentManagerClient()
-      OVERRIDE {
-    return client_bundle_->bluetooth_agent_manager_client();
-  }
+// static
+DBusThreadManager* DBusThreadManager::CreateInstance() {

[hashimoto, 2014/08/25 06:56:20]

Is this method needed?
Can't InitializeClients() be called in the ctor?

[zel, 2014/08/25 17:21:42]

Done.

[zel, 2014/08/25 20:19:47]

I take this back since it had broken tons of tests.

+  DBusThreadManager* dbus_thread_manager = new DBusThreadManager();
+  dbus_thread_manager->InitializeClients();
+  return dbus_thread_manager;
+}
 
-  virtual BluetoothDeviceClient* GetBluetoothDeviceClient() OVERRIDE {
-    return client_bundle_->bluetooth_device_client();
-  }
+dbus::Bus* DBusThreadManager::GetSystemBus() {
+  return system_bus_.get();
+}
 
-  virtual BluetoothGattCharacteristicClient*
-      GetBluetoothGattCharacteristicClient() OVERRIDE {
-    return client_bundle_->bluetooth_gatt_characteristic_client();
-  }
+BluetoothAdapterClient* DBusThreadManager::GetBluetoothAdapterClient() {
+  return client_bundle_->bluetooth_adapter_client();
+}
 
-  virtual BluetoothGattDescriptorClient* GetBluetoothGattDescriptorClient()
-      OVERRIDE {
-    return client_bundle_->bluetooth_gatt_descriptor_client();
-  }
+BluetoothAgentManagerClient*
+DBusThreadManager::GetBluetoothAgentManagerClient() {
+  return client_bundle_->bluetooth_agent_manager_client();
+}
 
-  virtual BluetoothGattManagerClient* GetBluetoothGattManagerClient() OVERRIDE {
-    return client_bundle_->bluetooth_gatt_manager_client();
-  }
+BluetoothDeviceClient* DBusThreadManager::GetBluetoothDeviceClient() {
+  return client_bundle_->bluetooth_device_client();
+}
 
-  virtual BluetoothGattServiceClient* GetBluetoothGattServiceClient() OVERRIDE {
-    return client_bundle_->bluetooth_gatt_service_client();
-  }
+BluetoothGattCharacteristicClient*
+DBusThreadManager::GetBluetoothGattCharacteristicClient() {
+  return client_bundle_->bluetooth_gatt_characteristic_client();
+}
 
-  virtual BluetoothInputClient* GetBluetoothInputClient() OVERRIDE {
-    return client_bundle_->bluetooth_input_client();
-  }
+BluetoothGattDescriptorClient*
+DBusThreadManager::GetBluetoothGattDescriptorClient() {
+  return client_bundle_->bluetooth_gatt_descriptor_client();
+}
 
-  virtual BluetoothProfileManagerClient* GetBluetoothProfileManagerClient()
-      OVERRIDE {
-    return client_bundle_->bluetooth_profile_manager_client();
-  }
+BluetoothGattManagerClient*
+DBusThreadManager::GetBluetoothGattManagerClient() {
+  return client_bundle_->bluetooth_gatt_manager_client();
+}
 
-  virtual CrasAudioClient* GetCrasAudioClient() OVERRIDE {
-    return client_bundle_->cras_audio_client();
-  }
+BluetoothGattServiceClient*
+DBusThreadManager::GetBluetoothGattServiceClient() {
+  return client_bundle_->bluetooth_gatt_service_client();
+}
 
-  virtual CrosDisksClient* GetCrosDisksClient() OVERRIDE {
-    return client_bundle_->cros_disks_client();
-  }
+BluetoothInputClient* DBusThreadManager::GetBluetoothInputClient() {
+  return client_bundle_->bluetooth_input_client();
+}
 
-  virtual CryptohomeClient* GetCryptohomeClient() OVERRIDE {
-    return client_bundle_->cryptohome_client();
-  }
+BluetoothProfileManagerClient*
+DBusThreadManager::GetBluetoothProfileManagerClient() {
+  return client_bundle_->bluetooth_profile_manager_client();
+}
 
-  virtual DebugDaemonClient* GetDebugDaemonClient() OVERRIDE {
-    return client_bundle_->debug_daemon_client();
-  }
+CrasAudioClient* DBusThreadManager::GetCrasAudioClient() {
+  return client_bundle_->cras_audio_client();
+}
 
-  virtual EasyUnlockClient* GetEasyUnlockClient() OVERRIDE {
-    return client_bundle_->easy_unlock_client();
-  }
-  virtual LorgnetteManagerClient* GetLorgnetteManagerClient() OVERRIDE {
-    return client_bundle_->lorgnette_manager_client();
-  }
+CrosDisksClient* DBusThreadManager::GetCrosDisksClient() {
+  return client_bundle_->cros_disks_client();
+}
 
-  virtual ShillDeviceClient* GetShillDeviceClient() OVERRIDE {
-    return client_bundle_->shill_device_client();
-  }
+CryptohomeClient* DBusThreadManager::GetCryptohomeClient() {
+  return client_bundle_->cryptohome_client();
+}
 
-  virtual ShillIPConfigClient* GetShillIPConfigClient() OVERRIDE {
-    return client_bundle_->shill_ipconfig_client();
-  }
+DebugDaemonClient* DBusThreadManager::GetDebugDaemonClient() {
+  return client_bundle_->debug_daemon_client();
+}
 
-  virtual ShillManagerClient* GetShillManagerClient() OVERRIDE {
-    return client_bundle_->shill_manager_client();
-  }
+EasyUnlockClient* DBusThreadManager::GetEasyUnlockClient() {
+  return client_bundle_->easy_unlock_client();
+}
+LorgnetteManagerClient*
+DBusThreadManager::GetLorgnetteManagerClient() {
+  return client_bundle_->lorgnette_manager_client();
+}
 
-  virtual ShillServiceClient* GetShillServiceClient() OVERRIDE {
-    return client_bundle_->shill_service_client();
-  }
+ShillDeviceClient*
+DBusThreadManager::GetShillDeviceClient() {
+  return client_bundle_->shill_device_client();
+}
 
-  virtual ShillProfileClient* GetShillProfileClient() OVERRIDE {
-    return client_bundle_->shill_profile_client();
-  }
+ShillIPConfigClient*
+DBusThreadManager::GetShillIPConfigClient() {
+  return client_bundle_->shill_ipconfig_client();
+}
 
-  virtual GsmSMSClient* GetGsmSMSClient() OVERRIDE {
-    return client_bundle_->gsm_sms_client();
-  }
+ShillManagerClient*
+DBusThreadManager::GetShillManagerClient() {
+  return client_bundle_->shill_manager_client();
+}
 
-  virtual ImageBurnerClient* GetImageBurnerClient() OVERRIDE {
-    return client_bundle_->image_burner_client();
-  }
+ShillServiceClient*
+DBusThreadManager::GetShillServiceClient() {
+  return client_bundle_->shill_service_client();
+}
 
-  virtual IntrospectableClient* GetIntrospectableClient() OVERRIDE {
-    return client_bundle_->introspectable_client();
-  }
+ShillProfileClient*
+DBusThreadManager::GetShillProfileClient() {
+  return client_bundle_->shill_profile_client();
+}
 
-  virtual ModemMessagingClient* GetModemMessagingClient() OVERRIDE {
-    return client_bundle_->modem_messaging_client();
-  }
+GsmSMSClient* DBusThreadManager::GetGsmSMSClient() {
+  return client_bundle_->gsm_sms_client();
+}
 
-  virtual NfcAdapterClient* GetNfcAdapterClient() OVERRIDE {
-    return client_bundle_->nfc_adapter_client();
-  }
+ImageBurnerClient* DBusThreadManager::GetImageBurnerClient() {
+  return client_bundle_->image_burner_client();
+}
 
-  virtual NfcDeviceClient* GetNfcDeviceClient() OVERRIDE {
-    return client_bundle_->nfc_device_client();
-  }
+IntrospectableClient* DBusThreadManager::GetIntrospectableClient() {
+  return client_bundle_->introspectable_client();
+}
 
-  virtual NfcManagerClient* GetNfcManagerClient() OVERRIDE {
-    return client_bundle_->nfc_manager_client();
-  }
+ModemMessagingClient* DBusThreadManager::GetModemMessagingClient() {
+  return client_bundle_->modem_messaging_client();
+}
 
-  virtual NfcRecordClient* GetNfcRecordClient() OVERRIDE {
-    return client_bundle_->nfc_record_client();
-  }
+NfcAdapterClient* DBusThreadManager::GetNfcAdapterClient() {
+  return client_bundle_->nfc_adapter_client();
+}
 
-  virtual NfcTagClient* GetNfcTagClient() OVERRIDE {
-    return client_bundle_->nfc_tag_client();
-  }
+NfcDeviceClient* DBusThreadManager::GetNfcDeviceClient() {
+  return client_bundle_->nfc_device_client();
+}
 
-  virtual PermissionBrokerClient* GetPermissionBrokerClient() OVERRIDE {
-    return client_bundle_->permission_broker_client();
-  }
+NfcManagerClient* DBusThreadManager::GetNfcManagerClient() {
+  return client_bundle_->nfc_manager_client();
+}
 
-  virtual PowerManagerClient* GetPowerManagerClient() OVERRIDE {
-    return client_bundle_->power_manager_client();
-  }
+NfcRecordClient* DBusThreadManager::GetNfcRecordClient() {
+  return client_bundle_->nfc_record_client();
+}
 
-  virtual SessionManagerClient* GetSessionManagerClient() OVERRIDE {
-    return client_bundle_->session_manager_client();
-  }
+NfcTagClient* DBusThreadManager::GetNfcTagClient() {
+  return client_bundle_->nfc_tag_client();
+}
 
-  virtual SMSClient* GetSMSClient() OVERRIDE {
-    return client_bundle_->sms_client();
-  }
+PermissionBrokerClient* DBusThreadManager::GetPermissionBrokerClient() {
+  return client_bundle_->permission_broker_client();
+}
 
-  virtual SystemClockClient* GetSystemClockClient() OVERRIDE {
-    return client_bundle_->system_clock_client();
-  }
+PowerManagerClient* DBusThreadManager::GetPowerManagerClient() {
+  return client_bundle_->power_manager_client();
+}
 
-  virtual UpdateEngineClient* GetUpdateEngineClient() OVERRIDE {
-    return client_bundle_->update_engine_client();
-  }
+SessionManagerClient* DBusThreadManager::GetSessionManagerClient() {
+  return client_bundle_->session_manager_client();
+}
 
-  virtual PowerPolicyController* GetPowerPolicyController() OVERRIDE {
-    return power_policy_controller_.get();
-  }
+SMSClient* DBusThreadManager::GetSMSClient() {
+  return client_bundle_->sms_client();
+}
 
- private:
-  // Constructs all clients and stores them in the respective *_client_ member
-  // variable.
-  void CreateDefaultClients() {
-    client_bundle_.reset(new DBusClientBundle());
-    // TODO(crbug.com/345586): Move PowerPolicyController out of
-    // DBusThreadManagerImpl.
-    power_policy_controller_.reset(new PowerPolicyController);
-  }
+SystemClockClient* DBusThreadManager::GetSystemClockClient() {
+  return client_bundle_->system_clock_client();
+}
 
-  scoped_ptr<base::Thread> dbus_thread_;
-  scoped_refptr<dbus::Bus> system_bus_;
-  scoped_ptr<DBusClientBundle> client_bundle_;
-  scoped_ptr<PowerPolicyController> power_policy_controller_;
+UpdateEngineClient* DBusThreadManager::GetUpdateEngineClient() {
+  return client_bundle_->update_engine_client();
+}
 
-  DISALLOW_COPY_AND_ASSIGN(DBusThreadManagerImpl);
-};
+PowerPolicyController* DBusThreadManager::GetPowerPolicyController() {
+  return power_policy_controller_.get();
+}
+
+void DBusThreadManager::CreateDefaultClients() {
+  client_bundle_.reset(new DBusClientBundle());
+  // TODO(crbug.com/345586): Move PowerPolicyController out of
+  // DBusThreadManager.
+  power_policy_controller_.reset(new PowerPolicyController);
+}
+
+void DBusThreadManager::InitializeClients() {
+  GetBluetoothAdapterClient()->Init(GetSystemBus());
+  GetBluetoothAgentManagerClient()->Init(GetSystemBus());
+  GetBluetoothDeviceClient()->Init(GetSystemBus());
+  GetBluetoothGattCharacteristicClient()->Init(GetSystemBus());
+  GetBluetoothGattDescriptorClient()->Init(GetSystemBus());
+  GetBluetoothGattManagerClient()->Init(GetSystemBus());
+  GetBluetoothGattServiceClient()->Init(GetSystemBus());
+  GetBluetoothInputClient()->Init(GetSystemBus());
+  GetBluetoothProfileManagerClient()->Init(GetSystemBus());
+  GetCrasAudioClient()->Init(GetSystemBus());
+  GetCrosDisksClient()->Init(GetSystemBus());
+  GetCryptohomeClient()->Init(GetSystemBus());
+  GetDebugDaemonClient()->Init(GetSystemBus());
+  GetEasyUnlockClient()->Init(GetSystemBus());
+  GetGsmSMSClient()->Init(GetSystemBus());
+  GetImageBurnerClient()->Init(GetSystemBus());
+  GetIntrospectableClient()->Init(GetSystemBus());
+  GetLorgnetteManagerClient()->Init(GetSystemBus());
+  GetModemMessagingClient()->Init(GetSystemBus());
+  GetPermissionBrokerClient()->Init(GetSystemBus());
+  GetPowerManagerClient()->Init(GetSystemBus());
+  GetSessionManagerClient()->Init(GetSystemBus());
+  GetShillDeviceClient()->Init(GetSystemBus());
+  GetShillIPConfigClient()->Init(GetSystemBus());
+  GetShillManagerClient()->Init(GetSystemBus());
+  GetShillServiceClient()->Init(GetSystemBus());
+  GetShillProfileClient()->Init(GetSystemBus());
+  GetSMSClient()->Init(GetSystemBus());
+  GetSystemClockClient()->Init(GetSystemBus());
+  GetUpdateEngineClient()->Init(GetSystemBus());
+
+  // Initialize the NFC clients in the correct order. The order of
+  // initialization matters due to dependencies that exist between the
+  // client objects.
+  GetNfcManagerClient()->Init(GetSystemBus());
+  GetNfcAdapterClient()->Init(GetSystemBus());
+  GetNfcDeviceClient()->Init(GetSystemBus());
+  GetNfcTagClient()->Init(GetSystemBus());
+  GetNfcRecordClient()->Init(GetSystemBus());
+
+  // PowerPolicyController is dependent on PowerManagerClient, so
+  // initialize it after the main list of clients.
+  if (GetPowerPolicyController())
+    GetPowerPolicyController()->Init(this);
+
+  // This must be called after the list of clients so they've each had a
+  // chance to register with their object g_dbus_thread_managers.
+  if (GetSystemBus())
+    GetSystemBus()->GetManagedObjects();
+
+  client_bundle_->SetupDefaultEnvironment();
+}
 
 // static
 bool DBusThreadManager::IsUsingStub(DBusClientBundle::DBusClientType client) {
@@ -275,15 +345,10 @@ bool DBusThreadManager::IsUsingStub(DBusClientBundle::DBusClientType client) {
 void DBusThreadManager::Initialize() {
   // If we initialize DBusThreadManager twice we may also be shutting it down
   // early; do not allow that.
-  CHECK(g_dbus_thread_manager == NULL);
-
-  if (g_dbus_thread_manager_for_testing) {
-    g_dbus_thread_manager = g_dbus_thread_manager_for_testing;
-    InitializeClients();
-    VLOG(1) << "DBusThreadManager initialized with test implementation";
+  if (g_dbus_thread_manager_for_testing)

[hashimoto, 2014/08/25 06:56:20]

Why is this line needed?
Even tests should not call Initialize() twice.

[zel, 2014/08/25 17:21:42]

Nope - it's still needed. All browser_tests would still call this method, so if
we don't do this check and still use fake clients => crash. 

     return;
-  }
 
+  CHECK(g_dbus_thread_manager == NULL);
   bool use_dbus_stub = !base::SysInfo::IsRunningOnChromeOS() ||
       CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kDbusStub);
@@ -295,37 +360,39 @@ void DBusThreadManager::Initialize() {
         CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
             chromeos::switches::kDbusUnstubClients));
   } else if (use_dbus_stub) {
-    InitializeWithStub();
+    InitializeForTesting();
   } else {
     InitializeRegular();
   }
 }
 
 // static
-void DBusThreadManager::SetInstanceForTesting(
-    DBusThreadManager* dbus_thread_manager) {
-  CHECK(!g_dbus_thread_manager);
-  CHECK(!g_dbus_thread_manager_for_testing);
-  g_dbus_thread_manager_for_testing = dbus_thread_manager;
-}
+scoped_ptr<DBusThreadManagerTestHelper>
+DBusThreadManager::GetDBusThreadManagerTestHelper() {
+  if (!g_dbus_thread_manager_for_testing)

[hashimoto, 2014/08/25 06:56:20]

Why is this line needed?
Tests should be responsible to correctly initialize the class.

[zel, 2014/08/25 17:21:42]

First, InitializeForTesting() was made private so all tests should have just
call Initialize(). If a test needs to change any stubbed-out clients with its
own fakes, they should then call GetDBusThreadManagerTestHelper() instead.

InitializeForTesting() method still existed in the context of
GetDBusThreadManagerTestHelper() only. It provides mechanism for tests to allow
subsequent calls to Initialize() from other parts of the code. This is basically
needed for browser_tests since they would call Initialize() - see my comment
above.
 
In order to make code more readable here, I have renamed the private method
InitializeForTesting() to InitializeWithStubs() and morphed
|g_dbus_thread_manager_for_testing| to
|g_using_dbus_thread_manager_for_testing|. This should better depict what was
happening is this code.

+    InitializeForTesting();
 
-// static
-void DBusThreadManager::InitializeForTesting(
-    DBusThreadManager* dbus_thread_manager) {
-  unstub_client_mask_ = DBusClientBundle::NO_CLIENTS;
-  SetInstanceForTesting(dbus_thread_manager);
-  Initialize();
+  return make_scoped_ptr(new DBusThreadManagerTestHelper());
 }
 
 // static
 void DBusThreadManager::InitializeRegular() {
   unstub_client_mask_ = DBusClientBundle::ALL_CLIENTS;
-  g_dbus_thread_manager = new DBusThreadManagerImpl();
-  InitializeClients();
+  g_dbus_thread_manager = DBusThreadManager::CreateInstance();
   VLOG(1) << "DBusThreadManager initialized for Chrome OS";
 }
 
 // static
+void DBusThreadManager::InitializeForTesting() {
+  CHECK(!g_dbus_thread_manager);
+  CHECK(!g_dbus_thread_manager_for_testing);
+  unstub_client_mask_ = DBusClientBundle::NO_CLIENTS;
+  VLOG(1) << "DBusThreadManager created for testing";
+  g_dbus_thread_manager = DBusThreadManager::CreateInstance();
+  g_dbus_thread_manager_for_testing = g_dbus_thread_manager;
+}
+
+// static
 void DBusThreadManager::InitializeWithPartialStub(
     const std::string& unstub_clients) {
   // If we initialize DBusThreadManager twice we may also be shutting it down
@@ -340,25 +407,8 @@ void DBusThreadManager::InitializeWithPartialStub(
                << " cannot be parsed: "
                << unstub_clients;
   }
-  DBusThreadManagerImpl* dbus_thread_manager = new DBusThreadManagerImpl();
   VLOG(1) << "DBusThreadManager initialized for mixed runtime environment";
-  g_dbus_thread_manager = dbus_thread_manager;
-  InitializeClients();
-  dbus_thread_manager->SetupDefaultEnvironment();
-}
-
-// static
-void DBusThreadManager::InitializeWithStub() {
-  unstub_client_mask_ = DBusClientBundle::NO_CLIENTS;
-  // If we initialize DBusThreadManager twice we may also be shutting it down
-  // early; do not allow that.
-  CHECK(g_dbus_thread_manager == NULL);
-  FakeDBusThreadManager* fake_dbus_thread_manager = new FakeDBusThreadManager;
-  fake_dbus_thread_manager->SetFakeClients();
-  g_dbus_thread_manager = fake_dbus_thread_manager;
-  InitializeClients();
-  fake_dbus_thread_manager->SetupDefaultEnvironment();
-  VLOG(1) << "DBusThreadManager initialized with stub implementation";
+  g_dbus_thread_manager = DBusThreadManager::CreateInstance();
 }
 
 // static
@@ -368,9 +418,10 @@ bool DBusThreadManager::IsInitialized() {
 
 // static
 void DBusThreadManager::Shutdown() {
-  // If we called InitializeForTesting, this may get called more than once.
+  // If we called GetDBusThreadManagerTestHelper(), this may get called more

[hashimoto, 2014/08/25 06:56:20]

Does this comment still make sense?

[zel, 2014/08/25 17:21:42]

Done.

+  // than once.
   // Ensure that we only shutdown DBusThreadManager once.
-  CHECK(g_dbus_thread_manager || g_dbus_thread_manager_for_testing);
+  CHECK(g_dbus_thread_manager);
   DBusThreadManager* dbus_thread_manager = g_dbus_thread_manager;
   g_dbus_thread_manager = NULL;
   g_dbus_thread_manager_for_testing = NULL;
@@ -378,25 +429,6 @@ void DBusThreadManager::Shutdown() {
   VLOG(1) << "DBusThreadManager Shutdown completed";
 }
 
-DBusThreadManager::DBusThreadManager() {
-  dbus::statistics::Initialize();
-}
-
-DBusThreadManager::~DBusThreadManager() {
-  dbus::statistics::Shutdown();
-  if (g_dbus_thread_manager == NULL)
-    return;  // Called form Shutdown() or local test instance.
-  // There should never be both a global instance and a local instance.
-  CHECK(this == g_dbus_thread_manager);
-  if (g_dbus_thread_manager_for_testing) {
-    g_dbus_thread_manager = NULL;
-    g_dbus_thread_manager_for_testing = NULL;
-    VLOG(1) << "DBusThreadManager destroyed";
-  } else {
-    LOG(FATAL) << "~DBusThreadManager() called outside of Shutdown()";
-  }
-}
-
 // static
 DBusThreadManager* DBusThreadManager::Get() {
   CHECK(g_dbus_thread_manager)
@@ -404,65 +436,218 @@ DBusThreadManager* DBusThreadManager::Get() {
   return g_dbus_thread_manager;
 }
 
-// static
-void DBusThreadManager::InitializeClients() {
-  InitClient(g_dbus_thread_manager->GetBluetoothAdapterClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothAgentManagerClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothDeviceClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothGattCharacteristicClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothGattDescriptorClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothGattManagerClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothGattServiceClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothInputClient());
-  InitClient(g_dbus_thread_manager->GetBluetoothProfileManagerClient());
-  InitClient(g_dbus_thread_manager->GetCrasAudioClient());
-  InitClient(g_dbus_thread_manager->GetCrosDisksClient());
-  InitClient(g_dbus_thread_manager->GetCryptohomeClient());
-  InitClient(g_dbus_thread_manager->GetDebugDaemonClient());
-  InitClient(g_dbus_thread_manager->GetEasyUnlockClient());
-  InitClient(g_dbus_thread_manager->GetGsmSMSClient());
-  InitClient(g_dbus_thread_manager->GetImageBurnerClient());
-  InitClient(g_dbus_thread_manager->GetIntrospectableClient());
-  InitClient(g_dbus_thread_manager->GetLorgnetteManagerClient());
-  InitClient(g_dbus_thread_manager->GetModemMessagingClient());
-  InitClient(g_dbus_thread_manager->GetPermissionBrokerClient());
-  InitClient(g_dbus_thread_manager->GetPowerManagerClient());
-  InitClient(g_dbus_thread_manager->GetSessionManagerClient());
-  InitClient(g_dbus_thread_manager->GetShillDeviceClient());
-  InitClient(g_dbus_thread_manager->GetShillIPConfigClient());
-  InitClient(g_dbus_thread_manager->GetShillManagerClient());
-  InitClient(g_dbus_thread_manager->GetShillServiceClient());
-  InitClient(g_dbus_thread_manager->GetShillProfileClient());
-  InitClient(g_dbus_thread_manager->GetSMSClient());
-  InitClient(g_dbus_thread_manager->GetSystemClockClient());
-  InitClient(g_dbus_thread_manager->GetUpdateEngineClient());
+DBusThreadManagerTestHelper::DBusThreadManagerTestHelper() {
+}
 
-  // Initialize the NFC clients in the correct order. The order of
-  // initialization matters due to dependencies that exist between the
-  // client objects.
-  InitClient(g_dbus_thread_manager->GetNfcManagerClient());
-  InitClient(g_dbus_thread_manager->GetNfcAdapterClient());
-  InitClient(g_dbus_thread_manager->GetNfcDeviceClient());
-  InitClient(g_dbus_thread_manager->GetNfcTagClient());
-  InitClient(g_dbus_thread_manager->GetNfcRecordClient());
+DBusThreadManagerTestHelper::~DBusThreadManagerTestHelper() {
+}
 
-  // PowerPolicyController is dependent on PowerManagerClient, so
-  // initialize it after the main list of clients.
-  if (g_dbus_thread_manager->GetPowerPolicyController()) {
-    g_dbus_thread_manager->GetPowerPolicyController()->Init(
-        g_dbus_thread_manager);
-  }
+void DBusThreadManagerTestHelper::SetBluetoothAdapterClient(
+    scoped_ptr<BluetoothAdapterClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_adapter_client_ =
+      client.Pass();
+}
 
-  // This must be called after the list of clients so they've each had a
-  // chance to register with their object g_dbus_thread_managers.
-  if (g_dbus_thread_manager->GetSystemBus())
-    g_dbus_thread_manager->GetSystemBus()->GetManagedObjects();
+void DBusThreadManagerTestHelper::SetBluetoothAgentManagerClient(
+    scoped_ptr<BluetoothAgentManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_agent_manager_client_ =
+      client.Pass();
 }
 
-// static
-void DBusThreadManager::InitClient(DBusClient* client) {
-  if (client)
-    client->Init(g_dbus_thread_manager->GetSystemBus());
+void DBusThreadManagerTestHelper::SetBluetoothDeviceClient(
+    scoped_ptr<BluetoothDeviceClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_device_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothGattCharacteristicClient(
+    scoped_ptr<BluetoothGattCharacteristicClient> client) {
+  DBusThreadManager::Get()->client_bundle_->
+      bluetooth_gatt_characteristic_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothGattDescriptorClient(
+    scoped_ptr<BluetoothGattDescriptorClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_gatt_descriptor_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothGattManagerClient(
+    scoped_ptr<BluetoothGattManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_gatt_manager_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothGattServiceClient(
+    scoped_ptr<BluetoothGattServiceClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_gatt_service_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothInputClient(
+    scoped_ptr<BluetoothInputClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_input_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetBluetoothProfileManagerClient(
+    scoped_ptr<BluetoothProfileManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->bluetooth_profile_manager_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetCrasAudioClient(
+    scoped_ptr<CrasAudioClient> client) {
+  DBusThreadManager::Get()->client_bundle_->cras_audio_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetCrosDisksClient(
+    scoped_ptr<CrosDisksClient> client) {
+  DBusThreadManager::Get()->client_bundle_->cros_disks_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetCryptohomeClient(
+    scoped_ptr<CryptohomeClient> client) {
+  DBusThreadManager::Get()->client_bundle_->cryptohome_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetDebugDaemonClient(
+    scoped_ptr<DebugDaemonClient> client) {
+  DBusThreadManager::Get()->client_bundle_->debug_daemon_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetEasyUnlockClient(
+    scoped_ptr<EasyUnlockClient> client) {
+  DBusThreadManager::Get()->client_bundle_->easy_unlock_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetLorgnetteManagerClient(
+    scoped_ptr<LorgnetteManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->lorgnette_manager_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetShillDeviceClient(
+    scoped_ptr<ShillDeviceClient> client) {
+  DBusThreadManager::Get()->client_bundle_->shill_device_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetShillIPConfigClient(
+    scoped_ptr<ShillIPConfigClient> client) {
+  DBusThreadManager::Get()->client_bundle_->shill_ipconfig_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetShillManagerClient(
+    scoped_ptr<ShillManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->shill_manager_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetShillServiceClient(
+    scoped_ptr<ShillServiceClient> client) {
+  DBusThreadManager::Get()->client_bundle_->shill_service_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetShillProfileClient(
+    scoped_ptr<ShillProfileClient> client) {
+  DBusThreadManager::Get()->client_bundle_->shill_profile_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetGsmSMSClient(
+    scoped_ptr<GsmSMSClient> client) {
+  DBusThreadManager::Get()->client_bundle_->gsm_sms_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetImageBurnerClient(
+    scoped_ptr<ImageBurnerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->image_burner_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetIntrospectableClient(
+    scoped_ptr<IntrospectableClient> client) {
+  DBusThreadManager::Get()->client_bundle_->introspectable_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetModemMessagingClient(
+    scoped_ptr<ModemMessagingClient> client) {
+  DBusThreadManager::Get()->client_bundle_->modem_messaging_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetNfcAdapterClient(
+    scoped_ptr<NfcAdapterClient> client) {
+  DBusThreadManager::Get()->client_bundle_->nfc_adapter_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetNfcDeviceClient(
+    scoped_ptr<NfcDeviceClient> client) {
+  DBusThreadManager::Get()->client_bundle_->nfc_device_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetNfcManagerClient(
+    scoped_ptr<NfcManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->nfc_manager_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetNfcRecordClient(
+    scoped_ptr<NfcRecordClient> client) {
+  DBusThreadManager::Get()->client_bundle_->nfc_record_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetNfcTagClient(
+    scoped_ptr<NfcTagClient> client) {
+  DBusThreadManager::Get()->client_bundle_->nfc_tag_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetPermissionBrokerClient(
+    scoped_ptr<PermissionBrokerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->permission_broker_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetPowerManagerClient(
+    scoped_ptr<PowerManagerClient> client) {

[hashimoto, 2014/08/25 06:56:20]

A method named SetPowerManagerClient overwriting the power policy client is
quite surprising.
Can't you make this a simple setter?

If something goes wrong, it should be handled in the caller's side.

[zel, 2014/08/25 17:21:42]

No, this can't be handled on the caller side due to convoluted power policy
implementation.

This implementation is what derat@ suggested to me to do here.

[Daniel Erat, 2014/08/25 17:28:25]

PowerPolicyController registers itself as an observer of PowerManagerClient, so
not resetting it here results in a stale pointer and eventual crash.

suggestions welcomed at http://crbug.com/345586

[hashimoto, 2014/08/26 01:54:59]

Then can't we remove SetPowerPolicyController?
PowerPrefsTest, this method's exclusive user, may be satisfied by using
GetPowerPolicyController() instead of owning PowerPolicyController itself.

[Daniel Erat, 2014/08/26 16:08:03]

yes, i suspect that this would work.

[zel, 2014/08/26 17:50:45]

Removed SetPowerPolicyController()

+  DBusThreadManager::Get()->power_policy_controller_.reset();
+  DBusThreadManager::Get()->client_bundle_->power_manager_client_ =
+      client.Pass();
+  DBusThreadManager::Get()->power_policy_controller_.reset(
+      new PowerPolicyController);
+  DBusThreadManager::Get()->power_policy_controller_->Init(
+      DBusThreadManager::Get());
+}
+
+void DBusThreadManagerTestHelper::SetPowerPolicyController(
+    scoped_ptr<PowerPolicyController> client) {
+  DBusThreadManager::Get()->power_policy_controller_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetSessionManagerClient(
+    scoped_ptr<SessionManagerClient> client) {
+  DBusThreadManager::Get()->client_bundle_->session_manager_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetSMSClient(scoped_ptr<SMSClient> client) {
+  DBusThreadManager::Get()->client_bundle_->sms_client_ = client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetSystemClockClient(
+    scoped_ptr<SystemClockClient> client) {
+  DBusThreadManager::Get()->client_bundle_->system_clock_client_ =
+      client.Pass();
+}
+
+void DBusThreadManagerTestHelper::SetUpdateEngineClient(
+    scoped_ptr<UpdateEngineClient> client) {
+  DBusThreadManager::Get()->client_bundle_->update_engine_client_ =
+      client.Pass();
 }
 
 }  // namespace chromeos