ChromeOS: Remove MockDBusThreadManager.

ChromeOS: Remove MockDBusThreadManager.

- Extends FakeDBusThreadManager about DBusClient setters.
- Removes default initialization of DBusClients from FakeDBusThreadManager.
- Replaces all occurrences of MockDBusThreadManager in tests by FakeDBusThreadManager.
- Every test now creates exactly the DBusClients that are required.
- Removes all references to Fake/Stub implementations from the DBusThreadManagerImpl used for production.

Depends on:
https://codereview.chromium.org/50243005/

BUG=275286
TBR=miket@chromium.org,mkwst@chromium.org,rtenneti@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234172

[pneubeck (no reviews), 2013-11-03 20:23:56]

Hi Satoru,

I uploaded three new CLs, which I think address all ideas and concerns that you
mentioned.
Please take at least a highlevel overview. I still want to see whether all test
pass.

[satorux1, 2013-11-05 03:55:15]

Thank you for creating this patch! It's a big undertaking.

I like this approach as it makes tests more explicit about what fake D-Bus
clients they use.

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.cc (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.cc:42:
STUB_DBUS_CLIENT_IMPLEMENTATION;
I think we should rename this to FAKE_DBUS_CLIENT_IMPLEMENTATION as classes are
now named Fake* rather than Stub*. Maybe we should do it in a separate patch.

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.h (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.h:14: CHROMEOS_EXPORT void
SetStubClients(FakeDBusThreadManager* manager);
Function comment is missing. 

Seems to me that this function is only used from
DBusThreadManager::InitializeWithStub(). Do you plan to use this from elsewhere?

Otherwise, would be nicer to put this in dbus_thread_manager.cc in favor of less
things to expose.

[pneubeck (no reviews), 2013-11-05 10:03:03]

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.cc (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.cc:42:
STUB_DBUS_CLIENT_IMPLEMENTATION;
On 2013/11/05 03:55:16, satorux1 wrote:
> I think we should rename this to FAKE_DBUS_CLIENT_IMPLEMENTATION as classes
are
> now named Fake* rather than Stub*. Maybe we should do it in a separate patch.

I think we can even remove this enum in a follow up CL.
Since all of the Fake implementations seem to be finally exposed now, we can
directly call their constructors here.

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.h (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.h:14: CHROMEOS_EXPORT void
SetStubClients(FakeDBusThreadManager* manager);
On 2013/11/05 03:55:16, satorux1 wrote:
> Function comment is missing. 
> 
> Seems to me that this function is only used from
> DBusThreadManager::InitializeWithStub(). Do you plan to use this from
elsewhere?
> 
> Otherwise, would be nicer to put this in dbus_thread_manager.cc in favor of
less
> things to expose.

By providing this separate function, we can do the following in tests:

FakeDBusThreadManager* manager = new Fake...;
SetStubClients(manager);
manager->SetShillManagerClient(my_client_);
DBusThreadManager::InitializeForTesting(manager);

This isn't used in any test so far. But I think this is very important, for
example for the FakeShillManagerClient. Currently it (and some other
ShillClients) relies on a GetTestInterface() in the ShillManagerClient (!)
interface and is implemented only by the FakeShillManagerClient.
With the above pattern, we can remove these GetTestInterface() methods.

[satorux1, 2013-11-05 10:18:55]

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.h (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.h:14: CHROMEOS_EXPORT void
SetStubClients(FakeDBusThreadManager* manager);
On 2013/11/05 10:03:04, pneubeck wrote:
> On 2013/11/05 03:55:16, satorux1 wrote:
> > Function comment is missing. 
> > 
> > Seems to me that this function is only used from
> > DBusThreadManager::InitializeWithStub(). Do you plan to use this from
> elsewhere?
> > 
> > Otherwise, would be nicer to put this in dbus_thread_manager.cc in favor of
> less
> > things to expose.
> 
> By providing this separate function, we can do the following in tests:
> 
> FakeDBusThreadManager* manager = new Fake...;
> SetStubClients(manager);
> manager->SetShillManagerClient(my_client_);
> DBusThreadManager::InitializeForTesting(manager);
> 
> This isn't used in any test so far. But I think this is very important, for
> example for the FakeShillManagerClient. Currently it (and some other
> ShillClients) relies on a GetTestInterface() in the ShillManagerClient (!)
> interface and is implemented only by the FakeShillManagerClient.
> With the above pattern, we can remove these GetTestInterface() methods.

Oh I see. Does this have to be a function in a separate file? I thought it'd be
simpler to make it a member of FakeDBusThreadManager, like:

  manager->AddFakeClients();

[pneubeck (no reviews), 2013-11-05 10:58:43]

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.h (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.h:14: CHROMEOS_EXPORT void
SetStubClients(FakeDBusThreadManager* manager);
On 2013/11/05 10:18:56, satorux1 wrote:
> On 2013/11/05 10:03:04, pneubeck wrote:
> > On 2013/11/05 03:55:16, satorux1 wrote:
> > > Function comment is missing. 
> > > 
> > > Seems to me that this function is only used from
> > > DBusThreadManager::InitializeWithStub(). Do you plan to use this from
> > elsewhere?
> > > 
> > > Otherwise, would be nicer to put this in dbus_thread_manager.cc in favor
of
> > less
> > > things to expose.
> > 
> > By providing this separate function, we can do the following in tests:
> > 
> > FakeDBusThreadManager* manager = new Fake...;
> > SetStubClients(manager);
> > manager->SetShillManagerClient(my_client_);
> > DBusThreadManager::InitializeForTesting(manager);
> > 
> > This isn't used in any test so far. But I think this is very important, for
> > example for the FakeShillManagerClient. Currently it (and some other
> > ShillClients) relies on a GetTestInterface() in the ShillManagerClient (!)
> > interface and is implemented only by the FakeShillManagerClient.
> > With the above pattern, we can remove these GetTestInterface() methods.
> 
> Oh I see. Does this have to be a function in a separate file? I thought it'd
be
> simpler to make it a member of FakeDBusThreadManager, like:
> 
>   manager->AddFakeClients();
> 

Since it's used in dbus_thread_manager.cc anyways, I agree that we can make it a
static function of the Fake*Manager.

[pneubeck (no reviews), 2013-11-05 16:14:58]

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
File chromeos/dbus/stub_dbus_thread_manager_helper.h (right):

https://codereview.chromium.org/49773003/diff/310001/chromeos/dbus/stub_dbus_...
chromeos/dbus/stub_dbus_thread_manager_helper.h:14: CHROMEOS_EXPORT void
SetStubClients(FakeDBusThreadManager* manager);
On 2013/11/05 10:58:44, pneubeck wrote:
> On 2013/11/05 10:18:56, satorux1 wrote:
> > On 2013/11/05 10:03:04, pneubeck wrote:
> > > On 2013/11/05 03:55:16, satorux1 wrote:
> > > > Function comment is missing. 
> > > > 
> > > > Seems to me that this function is only used from
> > > > DBusThreadManager::InitializeWithStub(). Do you plan to use this from
> > > elsewhere?
> > > > 
> > > > Otherwise, would be nicer to put this in dbus_thread_manager.cc in favor
> of
> > > less
> > > > things to expose.
> > > 
> > > By providing this separate function, we can do the following in tests:
> > > 
> > > FakeDBusThreadManager* manager = new Fake...;
> > > SetStubClients(manager);
> > > manager->SetShillManagerClient(my_client_);
> > > DBusThreadManager::InitializeForTesting(manager);
> > > 
> > > This isn't used in any test so far. But I think this is very important,
for
> > > example for the FakeShillManagerClient. Currently it (and some other
> > > ShillClients) relies on a GetTestInterface() in the ShillManagerClient (!)
> > > interface and is implemented only by the FakeShillManagerClient.
> > > With the above pattern, we can remove these GetTestInterface() methods.
> > 
> > Oh I see. Does this have to be a function in a separate file? I thought it'd
> be
> > simpler to make it a member of FakeDBusThreadManager, like:
> > 
> >   manager->AddFakeClients();
> > 
> 
> Since it's used in dbus_thread_manager.cc anyways, I agree that we can make it
a
> static function of the Fake*Manager.

Done.

[satorux1, 2013-11-06 01:50:15]

LGTM. Please make sure that all tests pass.

https://codereview.chromium.org/49773003/diff/610001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/49773003/diff/610001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:117: 'dbus/stub_dbus_thread_manager_helper.h',
remove the helper here.

[pneubeck (no reviews), 2013-11-06 14:55:34]

https://codereview.chromium.org/49773003/diff/610001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/49773003/diff/610001/chromeos/chromeos.gyp#ne...
chromeos/chromeos.gyp:117: 'dbus/stub_dbus_thread_manager_helper.h',
On 2013/11/06 01:50:16, satorux1 wrote:
> remove the helper here.

Done.

[satorux1, 2013-11-08 06:23:03]

lots of tests seemed to fail:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[pneubeck (no reviews), 2013-11-08 10:01:48]

On 2013/11/08 06:23:03, satorux1 wrote:
> lots of tests seemed to fail:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

Yeah, some of the broke during the later changes. I'm currently going through
all of them and fixing them.

[pneubeck (no reviews), 2013-11-08 16:46:56]

Please take a look at the test changes:
miket:
  c/b/apps/app_browsertest.cc
  c/b/extensions/api/system_private/system_private_apitest.cc
  device/bluetooth/bluetooth_chromeos_unittest.cc
  device/bluetooth/bluetooth_profile_chromeos_unittest.cc
mkwst:
  c/b/browsing_data/browsing_data_remover_unittest.cc
rtenneti:
  c/b/metrics/metrics_log_unittest.cc

[commit-bot: I haz the power, 2013-11-10 09:54:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/49773003/1080001

[commit-bot: I haz the power, 2013-11-10 16:02:54]

Change committed as 234172