Clean up shill fake implementations

Clean up shill fake implementations

This is in preparation of merging NetworkState and FavoriteState,
crbug.com/375955.

The merge will involve a number of changes to ShillPropertyHandler
and in the process of making those changes some additional logging
and test cleanup was necessary that isn't directly related to the
merge itself.

BUG=none
For trivial changes to chrome/browser/ui/webui/options/
TBR=dbeam@chromium.org

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

[stevenjb, 2014-05-28 22:47:22]

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_dev...
File chromeos/dbus/fake_shill_device_client.h (right):

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_dev...
chromeos/dbus/fake_shill_device_client.h:91: const std::string& name) OVERRIDE;
I'm not sure why we were ever passing a separate "object_path" here. It's
confusing and unhelpful; sometimes it was being used like device_path, others
like name. Changed to name to be consistent throughout.

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.cc (left):

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:531:
CallNotifyObserversPropertyChanged(property);
This is redundant and was triggering extra/unnecessary updates (especially in
the new code).

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
chromeos/network/shill_property_handler.cc:295:
NET_LOG_DEBUG("ManagerPropertyChanged", key);
This should be happening infrequently, i.e. it should be helpful not spammy.

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
File chromeos/network/shill_property_handler_unittest.cc (left):

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
chromeos/network/shill_property_handler_unittest.cc:220: bool add_to_watch_list)
{
We no longer pay attention to the watch list, so I've removed any "testing" of
it.

[stevenjb, 2014-05-29 22:18:00]

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_man...
File chromeos/dbus/fake_shill_manager_client.cc (left):

https://codereview.chromium.org/299403012/diff/1/chromeos/dbus/fake_shill_man...
chromeos/dbus/fake_shill_manager_client.cc:531:
CallNotifyObserversPropertyChanged(property);
On 2014/05/28 22:47:22, stevenjb wrote:
> This is redundant and was triggering extra/unnecessary updates (especially in
> the new code).
Actually, this is not redundant when SetviceList actually changes. I changed
this to check for that and restored the notification.

[stevenjb, 2014-06-03 19:22:22]

ping

[pneubeck (no reviews), 2014-06-04 09:30:27]

Sorry, being very busy this week.

lgtm with minor comments.

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
File chromeos/network/shill_property_handler_unittest.cc (left):

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
chromeos/network/shill_property_handler_unittest.cc:220: bool add_to_watch_list)
{
On 2014/05/28 22:47:22, stevenjb wrote:
> We no longer pay attention to the watch list, so I've removed any "testing" of
> it.

Could add_to_watch_list then also be removed from the client's test interface?

https://codereview.chromium.org/299403012/diff/80001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:147: VLOG(2) << "SetProperty: " <<
name;
Just noting that these are ending up in release/production builds because of the
ability to switch to DBus stubs on startup.

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:47: device_count_(0),
nit:
You could consider setting the absolute counters
{device_,network_,favorite_}count_ to a value that is clearly invalid, like -1.
Otherwise, it could appear in some test that there are indeed zero devices,
although the counts are not initialized yet (which happens e.g. in
*ListChanged()).

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:278:
test_observer_->reset_change_counts();
maybe move this to the end of SetUp() in the fixture?

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/shill_p...
File chromeos/network/shill_property_handler_unittest.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/shill_p...
chromeos/network/shill_property_handler_unittest.cc:386: // Add should trigger a
service list update and update entries.
second 'update' -> 'updated' ?

[stevenjb, 2014-06-04 19:49:57]

https://codereview.chromium.org/299403012/diff/80001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:147: VLOG(2) << "SetProperty: " <<
name;
On 2014/06/04 09:30:27, pneubeck wrote:
> Just noting that these are ending up in release/production builds because of
the
> ability to switch to DBus stubs on startup. 
Will switch to DVLOG

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:47: device_count_(0),
On 2014/06/04 09:30:27, pneubeck wrote:
> nit:
> You could consider setting the absolute counters
> {device_,network_,favorite_}count_ to a value that is clearly invalid, like
-1.
> Otherwise, it could appear in some test that there are indeed zero devices,
> although the counts are not initialized yet (which happens e.g. in
> *ListChanged()).
We don't ever currently test for 0 entries, and probably wouldn't. We also track
*list_changed_count for confirming that the update was called.

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:278:
test_observer_->reset_change_counts();
On 2014/06/04 09:30:27, pneubeck wrote:
> maybe move this to the end of SetUp() in the fixture?
Done.

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/shill_p...
File chromeos/network/shill_property_handler_unittest.cc (right):

https://codereview.chromium.org/299403012/diff/80001/chromeos/network/shill_p...
chromeos/network/shill_property_handler_unittest.cc:386: // Add should trigger a
service list update and update entries.
On 2014/06/04 09:30:27, pneubeck wrote:
> second 'update' -> 'updated' ?
-> and should update

[stevenjb, 2014-06-04 19:50:35]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-04 19:52:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/299403012/100001

[commit-bot: I haz the power, 2014-06-04 23:25:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-04 23:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-04 23:29:42]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[pneubeck (no reviews), 2014-06-05 07:55:30]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-05 07:57:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/299403012/100001

[commit-bot: I haz the power, 2014-06-05 09:22:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-05 10:55:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 10:55:42]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[pneubeck (no reviews), 2014-06-05 12:27:46]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-05 12:28:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/299403012/100001

[commit-bot: I haz the power, 2014-06-05 14:04:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-05 15:35:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 15:35:29]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[pneubeck (no reviews), 2014-06-05 15:36:21]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-06-05 15:37:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/299403012/100001

[commit-bot: I haz the power, 2014-06-05 16:57:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-05 17:44:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 17:44:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[stevenjb, 2014-06-05 20:38:20]

PTAL. Removed the unused 'watch' code and fixed some tests relying on 'eth1' as
the default service path.

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
File chromeos/network/shill_property_handler_unittest.cc (left):

https://codereview.chromium.org/299403012/diff/1/chromeos/network/shill_prope...
chromeos/network/shill_property_handler_unittest.cc:220: bool add_to_watch_list)
{
On 2014/06/04 09:30:27, pneubeck wrote:
> On 2014/05/28 22:47:22, stevenjb wrote:
> > We no longer pay attention to the watch list, so I've removed any "testing"
of
> > it.
> 
> Could add_to_watch_list then also be removed from the client's test interface?

Yes. I was originally going to avoid that big of a change, but since I separated
this out anyway, I should do that cleanup here also.
Done.

[stevenjb, 2014-06-06 18:36:37]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-06 18:38:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/299403012/160001

[pneubeck (no reviews), 2014-06-06 20:04:24]

awesome! lgtm!
one minor comment

https://codereview.chromium.org/299403012/diff/160001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/299403012/diff/160001/chromeos/network/networ...
chromeos/network/network_state_handler.h:245: const std::string&
default_network_path() const {
equivalent to DefaultNetwork()->path() ?
might be worth a comment.

[stevenjb, 2014-06-06 20:06:12]

https://codereview.chromium.org/299403012/diff/160001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/299403012/diff/160001/chromeos/network/networ...
chromeos/network/network_state_handler.h:245: const std::string&
default_network_path() const {
On 2014/06/06 20:04:24, pneubeck wrote:
> equivalent to DefaultNetwork()->path() ?
> might be worth a comment.

It's not quite, because DefaultNetwork() might be NULL, whereas this is always
safe to call. Also this doesn't require knowledge of NetworkState.

[commit-bot: I haz the power, 2014-06-06 23:36:25]

Change committed as 275551

[stevenjb, 2014-06-10 16:16:18]

Posting my comments for consideration / discussion. Mostly comment changes so
far if we move forward with this. Will add and upload those in a bit.

[stevenjb, 2014-06-10 16:16:44]

On 2014/06/10 16:16:18, stevenjb wrote:
> Posting my comments for consideration / discussion. Mostly comment changes so
> far if we move forward with this. Will add and upload those in a bit.

Oops, posted on the wrong CL.