Merged FakedDBusThreadManager with DBusThreadManager.

chromeos/dbus/dbus_thread_manager.h

 // Copyright (c) 2012 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_DBUS_DBUS_THREAD_MANAGER_H_
 #define CHROMEOS_DBUS_DBUS_THREAD_MANAGER_H_
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 18 matching lines @@
 class BluetoothGattCharacteristicClient;
 class BluetoothGattDescriptorClient;
 class BluetoothGattManagerClient;
 class BluetoothGattServiceClient;
 class BluetoothInputClient;
 class BluetoothProfileManagerClient;
 class CrasAudioClient;
 class CrosDisksClient;
 class CryptohomeClient;
 class DBusClient;
+class DBusThreadManagerImpl;
+class DBusThreadManagerTestHelper;
 class DebugDaemonClient;
 class EasyUnlockClient;
 class GsmSMSClient;
 class ImageBurnerClient;
 class IntrospectableClient;
 class LorgnetteManagerClient;
 class ModemMessagingClient;
 class NfcAdapterClient;
 class NfcDeviceClient;
 class NfcManagerClient;
 class NfcRecordClient;
 class NfcTagClient;
 class PermissionBrokerClient;
 class PowerManagerClient;
 class PowerPolicyController;
 class SessionManagerClient;
 class ShillDeviceClient;
 class ShillIPConfigClient;
 class ShillManagerClient;
 class ShillProfileClient;
 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.
 //
 // This class also manages D-Bus connections and D-Bus clients, which
 // depend on the D-Bus thread to ensure the right order of shutdowns for
 // the D-Bus thread, the D-Bus connections, and the D-Bus clients.
 //
 // CALLBACKS IN D-BUS CLIENTS:
 //
 // D-Bus clients managed by DBusThreadManager are guaranteed to be deleted
 // after the D-Bus thread so the clients don't need to worry if new
 // incoming messages arrive from the D-Bus thread during shutdown of the
 // clients. The UI message loop is not running during the shutdown hence
 // the UI message loop won't post tasks to D-BUS clients during the
 // shutdown. However, to be extra cautious, clients should use
 // WeakPtrFactory when creating callbacks that run on UI thread. See
 // session_manager_client.cc for examples.
 //
 class CHROMEOS_EXPORT DBusThreadManager {
  public:
   // Sets the global instance. Must be called before any calls to Get().
   // We explicitly initialize and shut down the global object, rather than
   // 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();
 
   // Destroys the global instance.
   static void Shutdown();
 
   // Gets the global instance. Initialize() must be called first.
   static DBusThreadManager* Get();
 
@@ skipping 49 matching lines @@
   DBusThreadManager();
 
  private:
   // Initialize global thread manager instance.
   static void InitializeRegular();
 
   // Initialize with stub implementations for only certain clients that are
   // 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_;
 
   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_