chromeos/dbus/stub_dbus_thread_manager_helper.h - Issue 49773003: ChromeOS: Remove MockDBusThreadManager.

Side by Side Diff: chromeos/dbus/stub_dbus_thread_manager_helper.h

Issue 49773003: ChromeOS: Remove MockDBusThreadManager. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Created 7 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« no previous file with comments | « chromeos/dbus/power_policy_controller_unittest.cc ('k') | chromeos/dbus/stub_dbus_thread_manager_helper.cc » ('j') | chromeos/dbus/stub_dbus_thread_manager_helper.cc » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 // Copyright 2013 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #ifndef CHROMEOS_DBUS_STUB_DBUS_THREAD_MANAGER_HELPER_H_

	6 #define CHROMEOS_DBUS_STUB_DBUS_THREAD_MANAGER_HELPER_H_

	7

	8 #include "chromeos/chromeos_export.h"

	9

	10 namespace chromeos {

	11

	12 class FakeDBusThreadManager;

	13

	14 CHROMEOS_EXPORT void SetStubClients(FakeDBusThreadManager* manager);
	satorux1 2013/11/05 03:55:16 Function comment is missing. Seems to me that th 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:04 By providing this separate function, we can do the Show quoted text 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:56 Oh I see. Does this have to be a function in a sep Show quoted text 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:44 Since it's used in dbus_thread_manager.cc anyways, Show quoted text 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 FakeManager. pneubeck (no reviews)* 2013/11/05 16:14:59 Done. Show quoted text 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.
	15

	16 }

	17

	18 #endif // CHROMEOS_DBUS_STUB_DBUS_THREAD_MANAGER_HELPER_H_

OLD	NEW