Chrome OS: Use Manager.DefaultService for Default Network

Chrome OS: Use Manager.DefaultService for Default Network

This changes Chrome to use Manager.DefaultService to provide the default
network to Chrome (i.e. NetworkChangeNotifier) instead of relying on
Manager.Services[0]. This should fix some timing issues and make the NCS
more in sync with Shill.

One change in behavior that should be benign is that Shill considers a
connecting network to be the default network, whereas previously only
a connected network would be considered.

BUG=159540, 330873
R=gauravsh@chromium.org, pneubeck@chromium.org, tbarzic@chromium.org

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

[stevenjb, 2014-02-21 19:38:05]

OK, I've tested this on my device and everything seems to be working OK, so
hopefully this will make the NetworkChangeNotifier code on Chrome OS a bit more
robust.

gauravsh@ - if you have a few cycles to spare to take a look at this, I would
appreciate it.

Otherwise, does anyone know who owns NetworkChangeNotifierChromeOS these days?
 
pneubeck@ - I think you're the other person most familiar with
NetworkStateHandler at this point, so please take a look when you can.

quiche@/benhchan@ - FYI, but it's probably not a bad idea to one or both of you
to look through the NetworkStateHandler and ShillPropertyHandler code.

Thanks!

[mukesh agrawal, 2014-02-21 21:28:47]

stevenjb@, good catch about the connecting network issue. i'm going to change
shill to not update the default network until after it is connected. (the
current behavior is contrary to our api documentation, and probably confusing in
general.)

[mukesh agrawal, 2014-02-21 21:32:09]

crbug.com/345829 for the shill issue

[stevenjb, 2014-02-21 21:51:50]

On 2014/02/21 21:32:09, mukesh agrawal wrote:
> crbug.com/345829 for the shill issue

Great, that should make this change a bit less risky.

[pneubeck (no reviews), 2014-02-24 09:22:58]

We should ensure that the exact order of property updates in the cases:
  - going offline
  - going online
  - switching to another network
is reflected in the unit tests.

Apart from that lgtm with some minor comments.

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:182: const NetworkState*
NetworkStateHandler::DefaultNetwork() const {
you dropped the check for IsConnectedState.
Considering that Mukesh is adapting Shill's side, we might still add an error
log if it's not the case (e.g. because the State wasn't updated quickly enough).

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:183: return
GetNetworkState(default_network_path_);
should early return on default_network_path_.empty()
(it works as it is, but we shouldn't on purpose search for incorrect paths)

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:756: GetManagedStateLogName(network));
just an idea: you could also add "(default)" to the result of
GetManagedStateLogName if the network is the default one.

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

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:382: EXPECT_EQ(4u,
test_observer_->default_network_change_count());
nit: instead of counting indefinitely upwards, could you reset the count in
"default_network_change_count()" ?

[stevenjb, 2014-02-24 23:59:41]

Good suggestion on the tests, separating the conditions out helped reveal some
discrepencies in the fake handler timings, so I fixed those also.
PTAL

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:182: const NetworkState*
NetworkStateHandler::DefaultNetwork() const {
On 2014/02/24 09:22:58, pneubeck wrote:
> you dropped the check for IsConnectedState.
> Considering that Mukesh is adapting Shill's side, we might still add an error
> log if it's not the case (e.g. because the State wasn't updated quickly
enough).
So, this is kind of tricky. I want to keep the return value for this consistent
with what gets passed to DefaultNetworkChanged(). So, instead of checking
connection state here, I added logging in DefaultNetworkServiceChanged() and
OnNetworkConnectionStateChanged().

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:183: return
GetNetworkState(default_network_path_);
On 2014/02/24 09:22:58, pneubeck wrote:
> should early return on default_network_path_.empty()
> (it works as it is, but we shouldn't on purpose search for incorrect paths)

Done.

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler.cc:756: GetManagedStateLogName(network));
On 2014/02/24 09:22:58, pneubeck wrote:
> just an idea: you could also add "(default)" to the result of
> GetManagedStateLogName if the network is the default one.
Interesting thought. I'm not sure I want to lengthen every log event for the
network though (whereas calling attention to specific events that affect the NCN
is helpful).

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

https://codereview.chromium.org/175243004/diff/30001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:382: EXPECT_EQ(4u,
test_observer_->default_network_change_count());
On 2014/02/24 09:22:58, pneubeck wrote:
> nit: instead of counting indefinitely upwards, could you reset the count in
> "default_network_change_count()" ?

Done.

[mukesh agrawal, 2014-02-25 02:08:41]

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:183: if
(default_network_path_.empty())
Not sure about this check. When there is no connected network, shill will send a
path of "/". (It can't send an empty string, because the value is encoded as a
D-Bus path type, and path types aren't allowed to be empty.)

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:694: if (!service_path.empty()) {
Same here (might need to check equality against '/' instead of .empty()).

[stevenjb, 2014-02-25 21:31:39]

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:183: if
(default_network_path_.empty())
On 2014/02/25 02:08:41, mukesh agrawal wrote:
> Not sure about this check. When there is no connected network, shill will send
a
> path of "/". (It can't send an empty string, because the value is encoded as a
> D-Bus path type, and path types aren't allowed to be empty.)

Hmm, that's unfortunate, but I can add a test for that where we set
default_network_path_. The code will work as-is since GetNetworkState() will
return NULL if the path doesn't have an associated service, but for debugging,
etc, it would be better to use an empty string for no default network.

https://codereview.chromium.org/175243004/diff/360001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:694: if (!service_path.empty()) {
On 2014/02/25 02:08:41, mukesh agrawal wrote:
> Same here (might need to check equality against '/' instead of .empty()).
I will add the check for "/" where we set default_network_path_.

[mukesh agrawal, 2014-02-25 21:36:30]

sgtm

[tbarzic, 2014-02-25 23:27:45]

lgtm

[gauravsh, 2014-02-26 00:44:32]

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:698: if (!service_path.empty()) {
Shouldn't this check also be service_path == kEmptyServicePath

[pneubeck (no reviews), 2014-02-26 08:52:41]

lgtm

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:691: const char* kEmptyServicePath =
"/";
Better add a short comment why we are doing this.

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:695: default_network_path_ =
service_path;
everything after this line should probably use default_network_path_ instead of
service_path.

[stevenjb, 2014-02-26 18:04:47]

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:691: const char* kEmptyServicePath =
"/";
On 2014/02/26 08:52:42, pneubeck wrote:
> Better add a short comment why we are doing this.

Done.

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:695: default_network_path_ =
service_path;
On 2014/02/26 08:52:42, pneubeck wrote:
> everything after this line should probably use default_network_path_ instead
of
> service_path.
Yes indeed, thanks.

https://codereview.chromium.org/175243004/diff/380001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:698: if (!service_path.empty()) {
On 2014/02/26 00:44:32, gauravsh wrote:
> Shouldn't this check also be service_path == kEmptyServicePath
Ugh, thanks. We should use default_network_path_ below line 695 now.
Done.

[stevenjb, 2014-02-26 18:04:52]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-02-26 18:06:16]

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

[gauravsh, 2014-02-26 18:08:21]

lgtm

[commit-bot: I haz the power, 2014-02-26 18:56:41]

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

[commit-bot: I haz the power, 2014-02-26 18:56:42]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[stevenjb, 2014-02-27 02:24:42]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-02-27 02:26:32]

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

[commit-bot: I haz the power, 2014-02-27 03:43:02]

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

[commit-bot: I haz the power, 2014-02-27 03:43:03]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[stevenjb, 2014-03-04 01:36:03]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-04 01:40:15]

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

[commit-bot: I haz the power, 2014-03-04 03:19:53]

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

[commit-bot: I haz the power, 2014-03-04 03:19:54]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[stevenjb, 2014-03-05 19:55:16]

Committed patchset #12 manually as r255124 (presubmit successful).