Improve functionality of FakeShillProfileClient

Improve functionality of FakeShillProfileClient

Note: The fake shill client behaviors are tested by the higher level
tests that use them. They may be reaching a complexity level that merits
explicit testing, but it's not clear whether or not that would actually
be a win.

These changes are being added to support https://codereview.chromium.org/275543005/.

BUG=none
R=pneubeck@chromium.org

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

[stevenjb, 2014-05-13 01:14:44]

ptal

[stevenjb, 2014-05-13 17:51:32]

SO, reviewing the other CL was not super helpful without this one since they are
dependent :P

Today is full of meetings so I can wait until tomorrow to try to push the GUID
CL, but please review this one first, thanks!

[pneubeck (no reviews), 2014-05-14 07:43:44]

On 2014/05/13 17:51:32, stevenjb wrote:
> SO, reviewing the other CL was not super helpful without this one since they
are
> dependent :P
> 
> Today is full of meetings so I can wait until tomorrow to try to push the GUID
> CL, but please review this one first, thanks!

yeah, but the other one was a smaller diff ;-)

[pneubeck (no reviews), 2014-05-14 08:12:16]

lgtm

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
chromeos/dbus/fake_shill_profile_client.cc:243: profiles_.clear();
Redundant, STLDeleteValues also clears.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
File chromeos/dbus/fake_shill_profile_client.h (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
chromeos/dbus/fake_shill_profile_client.h:68: bool
AddOrUpdateServiceImpl(ProfileProperties* profile,
argument order: profile (because non-const) should be at the end.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
chromeos/dbus/fake_shill_service_client.cc:347: // If |guid| is not provided,
leave it blank. Shill does not enforce a
move the comment in front of the if, since it applies to both branches.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
chromeos/dbus/fake_shill_service_client.cc:354: if
(profile_properties.GetStringWithoutPathExpansion(
optional nit: IMO clearer, if profile_guid would be declared earlier and you
would use a 'if else'. You could also reuse 'guid'.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/shill_profile_...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/shill_profile_...
chromeos/dbus/shill_profile_client.h:67: // Updates an existing profile service
entry. Returns false if no entry
unclear what 'Updates' means.

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler_unittest.cc:150:
DBusThreadManager::InitializeWithStub();
instead of setting up the default network environment here and clearing it
below, the idea was at some time to instead create/set stubs explicitly.

Or if that is too much explicitness for your taste:

FakeDBusThreadManager* dbus_manager = new FakeDBusThreadManager;
dbus_manager->SetFakeClients();
DBusThreadManager::InitializeTesting(dbus_manager);

[stevenjb, 2014-05-14 17:08:10]

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
chromeos/dbus/fake_shill_profile_client.cc:243: profiles_.clear();
On 2014/05/14 08:12:17, pneubeck wrote:
> Redundant, STLDeleteValues also clears.
So it does. done.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
File chromeos/dbus/fake_shill_profile_client.h (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_pro...
chromeos/dbus/fake_shill_profile_client.h:68: bool
AddOrUpdateServiceImpl(ProfileProperties* profile,
On 2014/05/14 08:12:17, pneubeck wrote:
> argument order: profile (because non-const) should be at the end.

Done.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
chromeos/dbus/fake_shill_service_client.cc:347: // If |guid| is not provided,
leave it blank. Shill does not enforce a
On 2014/05/14 08:12:17, pneubeck wrote:
> move the comment in front of the if, since it applies to both branches.

Done.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/fake_shill_ser...
chromeos/dbus/fake_shill_service_client.cc:354: if
(profile_properties.GetStringWithoutPathExpansion(
On 2014/05/14 08:12:17, pneubeck wrote:
> optional nit: IMO clearer, if profile_guid would be declared earlier and you
> would use a 'if else'. You could also reuse 'guid'.

We can't use 'guid' here since it's const, but I'll try to clean this up a
little.

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/shill_profile_...
File chromeos/dbus/shill_profile_client.h (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/dbus/shill_profile_...
chromeos/dbus/shill_profile_client.h:67: // Updates an existing profile service
entry. Returns false if no entry
On 2014/05/14 08:12:17, pneubeck wrote:
> unclear what 'Updates' means.

Done.

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler_unittest.cc:150:
DBusThreadManager::InitializeWithStub();
On 2014/05/14 08:12:17, pneubeck wrote:
> instead of setting up the default network environment here and clearing it
> below, the idea was at some time to instead create/set stubs explicitly.
> 
> Or if that is too much explicitness for your taste:
> 
> FakeDBusThreadManager* dbus_manager = new FakeDBusThreadManager;
> dbus_manager->SetFakeClients();
> DBusThreadManager::InitializeTesting(dbus_manager);

That seems like an unrelated re-factor.

[pneubeck (no reviews), 2014-05-14 18:08:35]

still lgtm.

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler_unittest.cc:150:
DBusThreadManager::InitializeWithStub();
On 2014/05/14 17:08:10, stevenjb wrote:
> On 2014/05/14 08:12:17, pneubeck wrote:
> > instead of setting up the default network environment here and clearing it
> > below, the idea was at some time to instead create/set stubs explicitly.
> > 
> > Or if that is too much explicitness for your taste:
> > 
> > FakeDBusThreadManager* dbus_manager = new FakeDBusThreadManager;
> > dbus_manager->SetFakeClients();
> > DBusThreadManager::InitializeTesting(dbus_manager);
> 
> That seems like an unrelated re-factor.

I just wanted to mentioned it because it makes the ClearProfiles function
unnecessary.

[stevenjb, 2014-05-14 19:13:07]

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/284673004/diff/1/chromeos/network/network_sta...
chromeos/network/network_state_handler_unittest.cc:150:
DBusThreadManager::InitializeWithStub();
On 2014/05/14 18:08:35, pneubeck wrote:
> On 2014/05/14 17:08:10, stevenjb wrote:
> > On 2014/05/14 08:12:17, pneubeck wrote:
> > > instead of setting up the default network environment here and clearing it
> > > below, the idea was at some time to instead create/set stubs explicitly.
> > > 
> > > Or if that is too much explicitness for your taste:
> > > 
> > > FakeDBusThreadManager* dbus_manager = new FakeDBusThreadManager;
> > > dbus_manager->SetFakeClients();
> > > DBusThreadManager::InitializeTesting(dbus_manager);
> > 
> > That seems like an unrelated re-factor.
> 
> I just wanted to mentioned it because it makes the ClearProfiles function
> unnecessary.

Ah. Perhaps. At some point we should probably test unloading profiles too. I
still have mixed feelings about the Fake Shill implementations in general. Baby
steps.

[stevenjb, 2014-05-14 22:05:58]

Committed patchset #3 manually as r270486 (presubmit successful).