Merged FakedDBusThreadManager with DBusThreadManager.

chromeos/dbus/dbus_thread_manager.h

Index: chromeos/dbus/dbus_thread_manager.h
diff --git a/chromeos/dbus/dbus_thread_manager.h b/chromeos/dbus/dbus_thread_manager.h
index 053763c984d0854b2b56150db1755e7d0dac341d..4ecd1623b3805965aa21f31af02a8faf92630aa9 100644
--- a/chromeos/dbus/dbus_thread_manager.h
+++ b/chromeos/dbus/dbus_thread_manager.h
@@ -36,6 +36,8 @@ class CrasAudioClient;
 class CrosDisksClient;
 class CryptohomeClient;
 class DBusClient;
+class DBusThreadManagerImpl;
+class DBusThreadManagerTestHelper;
 class DebugDaemonClient;
 class EasyUnlockClient;
 class GsmSMSClient;
@@ -60,7 +62,6 @@ class ShillServiceClient;
 class SMSClient;
 class SystemClockClient;
 class UpdateEngineClient;

[hashimoto, 2014/08/18 04:55:58]

nit: Please keep a blank line here.

[zel, 2014/08/18 23:41:52]

Done.

-
 // DBusThreadManager manages the D-Bus thread, the thread dedicated to
 // handling asynchronous D-Bus operations.
 //
@@ -86,25 +87,15 @@ class CHROMEOS_EXPORT DBusThreadManager {
   // making it a Singleton, to ensure clean startup and shutdown.
   static void Initialize();
 
-  // Sets an alternative DBusThreadManager such as MockDBusThreadManager
-  // to be used in |Initialize()| for testing. Tests that call
-  // DBusThreadManager::Initialize() (such as browser_tests and
-  // interactive_ui_tests) should use this instead of calling
-  // |InitiailzeForTesting|.  The injected object will be owned by the
-  // internal pointer and deleted by Shutdown().
-  static void SetInstanceForTesting(DBusThreadManager* dbus_thread_manager);
-
-  // Similar to Initialize(), but injects an alternative
-  // DBusThreadManager using SetInstanceForTest first.  The injected
-  // object will be owned by the internal pointer and deleted by
-  // Shutdown(). Does not create any Fake client implementations.
-  static void InitializeForTesting(DBusThreadManager* dbus_thread_manager);
-
   // Initialize with stub implementations for tests, creating a complete set
   // of fake/stub client implementations. Also initializes a default set of
   // fake Shill devices and services, customizable with switches::kShillStub.
   static void InitializeWithStub();
 
+  // Similar to Initialize(), but configures DBusThreadManager to use stub/fake
+  // clients.
+  static scoped_ptr<DBusThreadManagerTestHelper> InitializeForTesting();

[hashimoto, 2014/08/18 04:55:58]

Can't we delete one of InitializeWithStub() or InitilizeForTesting()?
Having two of them seems confusing.
Probably we can even get rid of both of them and will be OK with having only
Initialize().

Also, how about adding something like GetClientBundleForTesting() to
DBusThreadManager which returns a pointer to the DBusClientBundle?
This way, setters can belong to the bundle (i.e. no need to have friend
statements) and there is no need to add a new class DBusThreadManagerTestHelper.

[zel, 2014/08/18 23:41:52]

InitilizeForTesting() still does one trick for us - makes Initialize() reentrant
which is needed by all browser tests, so I can't just get rid it. I've nuked
InitializeWithStub() though and made explicit method for
GetDBusThreadManagerTestHelper()
Hmm, why would a DBusClientBundle (production code class) implement bunch of
setter methods that we only need in tests?

[hashimoto, 2014/08/19 02:31:58]

Adding setters should be harmless as long as DBusClientBundle is not accessible
from production code outside of DBusThreadManager.

Since now DBusThreadManagerImpl is the only class which implements
DBusThreadManager, I guess we'll soon merge DBusThreadManagerImpl to
DBusThreadManager.
Then there are two ways to create DBusThreadManagerTestHelper, calling
DBusThreadManager::GetDBusThreadManagerTestHelper() (right way) and calling the
ctor of DBusThreadManagerTestHelper with DBusThreadManager::Get() (wrong way).

[zel, 2014/08/19 04:44:39]

What harm are you talking about here? What you are proposing bloats the
production code for no good reason.

It's simply wasteful to instantiate an object that uses more memory than what it
needs in runtime because its bloated with test-specific code like all these
setters. We should not do that in any of our classes and all test seams should
be as light as possible (ideally zero-sum) in regards to the production code.

[hashimoto, 2014/08/19 05:11:59]

The effect on the binary size (i.e. compiled&linked code) of the final product
should not be different as the same amount of code is added for setters.
Regarding the size of the memory allocated for each DBusClientBundle instance
(i.e. amount of memory used to represent the state of an instance), setters has
no effect on it as they don't add any member variables or any vtable entries

[zel, 2014/08/22 22:24:18]

Yeah, you are right about the runtime size I forgot that none of these are
actually virtual.

I still think that separating test code form the production code is worthy
argument. If I move all setters into DBusClientBundle class, you will have no
idea about the context in which these methods are used. 

I have made ctor of DBusThreadManagerTestHelper inaccessible outside of
DBusThreadManagerImpl class though.

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

You can make it clear by naming the bundle getter method like
DBusThreadManager::GetClientBundleForTesting().
Also, our presubmit script can raise warning if this method is used in the
production code.
(https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&l=274)

Anyways, it's up to you.
If you want to keep the test helper class, you might want to name the getter
method like GetDBusThreadManagerTestHelperForTesting().
(This method name looks too verbose, though.)
This should help the presubmit check to make warnings on inappropriate usage.

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

I have picked better class/method names now, it's:

static scoped_ptr<DBusThreadManagerSetter> GetSetterForTesting();

+
   // Returns true if DBusThreadManager has been initialized. Call this to
   // avoid initializing + shutting down DBusThreadManager more than once.
   static bool IsInitialized();
@@ -174,14 +165,6 @@ class CHROMEOS_EXPORT DBusThreadManager {
   // not included in comma-separated |unstub_clients| list.
   static void InitializeWithPartialStub(const std::string& unstub_clients);
 
-  // InitializeClients is called after g_dbus_thread_manager is set.
-  // NOTE: Clients that access other clients in their Init() must be
-  // initialized in the correct order.
-  static void InitializeClients();
-
-  // Initializes |client| with the |system_bus_|.
-  static void InitClient(DBusClient* client);
-
   // Bitmask that defines which dbus clients are not stubbed out. Bitmap flags
   // are defined within DBusClientBundle::DBusClientType enum.
   static DBusClientBundle::DBusClientTypeMask unstub_client_mask_;
@@ -189,6 +172,60 @@ class CHROMEOS_EXPORT DBusThreadManager {
   DISALLOW_COPY_AND_ASSIGN(DBusThreadManager);
 };
 
+class CHROMEOS_EXPORT DBusThreadManagerTestHelper {
+ public:
+  explicit DBusThreadManagerTestHelper(
+      DBusThreadManagerImpl* dbus_thread_manager);
+  ~DBusThreadManagerTestHelper();
+
+  void SetBluetoothAdapterClient(scoped_ptr<BluetoothAdapterClient> client);
+  void SetBluetoothAgentManagerClient(
+      scoped_ptr<BluetoothAgentManagerClient> client);
+  void SetBluetoothDeviceClient(scoped_ptr<BluetoothDeviceClient> client);
+  void SetBluetoothGattCharacteristicClient(
+      scoped_ptr<BluetoothGattCharacteristicClient> client);
+  void SetBluetoothGattDescriptorClient(
+      scoped_ptr<BluetoothGattDescriptorClient> client);
+  void SetBluetoothGattManagerClient(
+      scoped_ptr<BluetoothGattManagerClient> client);
+  void SetBluetoothGattServiceClient(
+      scoped_ptr<BluetoothGattServiceClient> client);
+  void SetBluetoothInputClient(scoped_ptr<BluetoothInputClient> client);
+  void SetBluetoothProfileManagerClient(
+      scoped_ptr<BluetoothProfileManagerClient> client);
+  void SetCrasAudioClient(scoped_ptr<CrasAudioClient> client);
+  void SetCrosDisksClient(scoped_ptr<CrosDisksClient> client);
+  void SetCryptohomeClient(scoped_ptr<CryptohomeClient> client);
+  void SetDebugDaemonClient(scoped_ptr<DebugDaemonClient> client);
+  void SetEasyUnlockClient(scoped_ptr<EasyUnlockClient> client);
+  void SetLorgnetteManagerClient(scoped_ptr<LorgnetteManagerClient> client);
+  void SetShillDeviceClient(scoped_ptr<ShillDeviceClient> client);
+  void SetShillIPConfigClient(scoped_ptr<ShillIPConfigClient> client);
+  void SetShillManagerClient(scoped_ptr<ShillManagerClient> client);
+  void SetShillServiceClient(scoped_ptr<ShillServiceClient> client);
+  void SetShillProfileClient(scoped_ptr<ShillProfileClient> client);
+  void SetGsmSMSClient(scoped_ptr<GsmSMSClient> client);
+  void SetImageBurnerClient(scoped_ptr<ImageBurnerClient> client);
+  void SetIntrospectableClient(scoped_ptr<IntrospectableClient> client);
+  void SetModemMessagingClient(scoped_ptr<ModemMessagingClient> client);
+  void SetNfcAdapterClient(scoped_ptr<NfcAdapterClient> client);
+  void SetNfcDeviceClient(scoped_ptr<NfcDeviceClient> client);
+  void SetNfcManagerClient(scoped_ptr<NfcManagerClient> client);
+  void SetNfcRecordClient(scoped_ptr<NfcRecordClient> client);
+  void SetNfcTagClient(scoped_ptr<NfcTagClient> client);
+  void SetPermissionBrokerClient(scoped_ptr<PermissionBrokerClient> client);
+  void SetPowerManagerClient(scoped_ptr<PowerManagerClient> client);
+  void SetPowerPolicyController(scoped_ptr<PowerPolicyController> client);
+  void SetSessionManagerClient(scoped_ptr<SessionManagerClient> client);
+  void SetSMSClient(scoped_ptr<SMSClient> client);
+  void SetSystemClockClient(scoped_ptr<SystemClockClient> client);
+  void SetUpdateEngineClient(scoped_ptr<UpdateEngineClient> client);
+
+ private:
+  DBusThreadManagerImpl* dbus_thread_manager_;
+  DISALLOW_COPY_AND_ASSIGN(DBusThreadManagerTestHelper);
+};
+
 }  // namespace chromeos
 
 #endif  // CHROMEOS_DBUS_DBUS_THREAD_MANAGER_H_