Merge FavoriteState into NetworkState

Merge FavoriteState into NetworkState

BUG=375955
For trivial renaming:

R=armansito@chromium.org, pneubeck@chromium.org
TBR=derat@chromium.org, xiyuan@chromium.org

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

[stevenjb, 2014-05-28 23:06:09]

https://codereview.chromium.org/289383004/diff/60001/chromeos/dbus/shill_devi...
File chromeos/dbus/shill_device_client.h (right):

https://codereview.chromium.org/289383004/diff/60001/chromeos/dbus/shill_devi...
chromeos/dbus/shill_device_client.h:49: const std::string& name) = 0;
This and related changes in a separate CL.

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/client_...
File chromeos/network/client_cert_resolver.cc (right):

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/client_...
chromeos/network/client_cert_resolver.cc:29: #include
"chromeos/network/shill_property_util.h"
no longer needed.

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
File chromeos/network/network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_configuration_handler_unittest.cc:509: const
std::string service_path("/service/wifi1");
Separate CL

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_connection_handler.cc:822:
network_state_handler_->GetNetworkListByType(
GetVisibleNetworksByType?

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
File chromeos/network/network_sms_handler_unittest.cc (right):

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_sms_handler_unittest.cc:69: "stub_cellular_device2");
Separate CL

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

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_state_handler.cc:432: if (complete_list) {
elim {}

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_state_handler.cc:511: if (found != network_map.end()) {
ContainsKey

https://codereview.chromium.org/289383004/diff/60001/chromeos/network/network...
chromeos/network/network_state_handler.cc:538: // The network has been removed
from the list of visible networks.
list of networks

[stevenjb, 2014-06-06 23:38:42]

OK, here it is, the CL you've been waiting for! :)

This works with the latest Shill changes to correctly order ServiceCompleteList
and push those changes. There is some additional cleanup that can be done with
the "Service.Visible" property (we can ignore "Services" completely) but I would
prefer to do that as a followup.

[armansito, 2014-06-07 06:21:03]

lgtm. Thanks for the rework! Phillip might have more constructive comments since
he's more familiar with WiFi/ONC specifics.

[pneubeck (no reviews), 2014-06-10 10:51:37]

so far mostly reviewed chromeos/network/* and rather concerned about observing
two lists (services and completeList). but maybe I just missed something.

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:197: if (!network->visible() ||
!network->IsConnectedState())
Hm. If it's not visible but in a connected state, isn't that a bug?
Better dcheck/log an error?

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:213: if (!network->visible() ||
!network->IsConnectingState())
ditto

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:269: NetworkStateList result;
unused

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:462: NET_LOG_DEBUG("Managed Entry: " +
ManagedState::TypeToString(type), path);
optional nit: too verbose even for debug? Your call, since you're the primary
user of the net log.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:469: void
NetworkStateHandler::UpdateVisibleNetworks(
if this is called because of a kServices change before the kServiceCompleteList
change is signaled, then we ignore networks that are not known (line 493) and
pick up visibility for these networks only on the next list change.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:503: if (!network->IsInProfile())
The old function updated all services from ServicesCompleteList, now you're
restricting it to configured/stored ones.

Seems ok to me, I just wanted to point out the difference.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:644: 1 /* limit */,
the comment and the used limit are off.
just don't use a limit to be on the safe side.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:687: void
NetworkStateHandler::ManagedStateListChanged(
again, this is called on ServicesCompleteList changes but 'visible' counter
depends on Services being already updated.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:733: if (new_service_path ==
default_network_path_)
Just noting that this is an unrelated functional change.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:761: std::string specifier =
network->GetSpecifier();
optional nit: could add a DCHECK(!specifier.empty());

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.h:89: // list of services which
determines the list of Networks. This should be
Isn't this usually unnecessary to call, since we have now Shill signalling
changes of the ServiceCompleteList:
https://chromium-review.googlesource.com/202223 ?

Maybe worth a try to get rid of in a follow up CL.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.h:151: // Sets |list| to contain the list
of visible networks matching |type|. The
both functions GetVisibleNetworkList* should be explained by referring to
GetNetworkListByType.
That function should have a/the detailed comment.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:276: if ((key.empty() || key ==
shill::kServiceCompleteListProperty) &&
I'm very uncertain about this working. It seems to rely on the order in which
Shill sends the kServiceCompleteListProperty and kServices update. If that's the
case, IMO that's too fragile.

Do we have a test checking the following?
A service X is not visible but 'configured' (i.e. X is in ServiceCompleteList
but not in kServicesProperty).
Observer sees it as not visible.
Service X becomes visible, i.e. X is now also in kServicesProperty.
>>In the notification call<< (this is the important condition), the observer
sees X being visible.

Ideally, we would test this for both update orders (Services vs.
ServicesCompleteList).

From reading the code, I'm a bit unsure whether the updating of the visible
property and sending the notification is always in the correct order (the
notification in question is send in line 234).

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:296:
listener_->UpdateVisibleNetworks(*vlist);
Note, that Shill is emitting a 'Visible' property for each service since a while
(https://chromium-review.googlesource.com/202305, and 
https://chromium-review.googlesource.com/202136).

That property could be parsed by NetworkState in the same way as other
properties. Maybe simpler than relying on the Services list.

This would also make my previous comment moot.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:238: size_t new_observed_networks_;
never read?

[stevenjb, 2014-06-10 16:17:29]

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

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:197: if (!network->visible() ||
!network->IsConnectedState())
On 2014/06/10 10:51:37, pneubeck wrote:
> Hm. If it's not visible but in a connected state, isn't that a bug?
> Better dcheck/log an error?
It's not necessarily a bug, we don't clear state when a network is removed from
the visible list. We shouldn't ever be checking State on non visible networks.
The tests aren't related to each other, I could separate them if that would be
more clear.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:213: if (!network->visible() ||
!network->IsConnectingState())
On 2014/06/10 10:51:37, pneubeck wrote:
> ditto
ditto

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:269: NetworkStateList result;
On 2014/06/10 10:51:37, pneubeck wrote:
> unused

Done.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:462: NET_LOG_DEBUG("Managed Entry: " +
ManagedState::TypeToString(type), path);
On 2014/06/10 10:51:37, pneubeck wrote:
> optional nit: too verbose even for debug? Your call, since you're the primary
> user of the net log.
Was debugging. Removed.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:469: void
NetworkStateHandler::UpdateVisibleNetworks(
On 2014/06/10 10:51:37, pneubeck wrote:
> if this is called because of a kServices change before the
kServiceCompleteList
> change is signaled, then we ignore networks that are not known (line 493) and
> pick up visibility for these networks only on the next list change.
Correct. Shill emits these in the order we desire, but this is less than ideal
and will be fixed in a followup when we ignore Services and use
Service.Visibility instead.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:503: if (!network->IsInProfile())
On 2014/06/10 10:51:37, pneubeck wrote:
> The old function updated all services from ServicesCompleteList, now you're
> restricting it to configured/stored ones.
> 
> Seems ok to me, I just wanted to point out the difference.

Yes, that was intentional, but not that you mention it, since we get the profile
path from the service itself (instead of from the Profile) we should probably go
ahead and request all services to be safe. This only ever happens once per
session currently, and will never happen often.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:644: 1 /* limit */,
On 2014/06/10 10:51:37, pneubeck wrote:
> the comment and the used limit are off.
> just don't use a limit to be on the safe side.
Done.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:687: void
NetworkStateHandler::ManagedStateListChanged(
On 2014/06/10 10:51:36, pneubeck wrote:
> again, this is called on ServicesCompleteList changes but 'visible' counter
> depends on Services being already updated.
It's called when all updates have been received so should be fine.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:733: if (new_service_path ==
default_network_path_)
On 2014/06/10 10:51:36, pneubeck wrote:
> Just noting that this is an unrelated functional change.
Not entirely unrelated. This CL changes some of the timing and a lot of stub
behavior so we ended up calling this a bit more frequently. This change makes
the frequency more predictable.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:761: std::string specifier =
network->GetSpecifier();
On 2014/06/10 10:51:37, pneubeck wrote:
> optional nit: could add a DCHECK(!specifier.empty());

Done.

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.h:89: // list of services which
determines the list of Networks. This should be
On 2014/06/10 10:51:37, pneubeck wrote:
> Isn't this usually unnecessary to call, since we have now Shill signalling
> changes of the ServiceCompleteList:
> https://chromium-review.googlesource.com/202223 ?
> 
> Maybe worth a try to get rid of in a follow up CL.
It's mostly unused now, but I will do an audit and confirm any necessary usage
and comment.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.h:151: // Sets |list| to contain the list
of visible networks matching |type|. The
On 2014/06/10 10:51:37, pneubeck wrote:
> both functions GetVisibleNetworkList* should be explained by referring to
> GetNetworkListByType.
> That function should have a/the detailed comment.
Will do.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:276: if ((key.empty() || key ==
shill::kServiceCompleteListProperty) &&
On 2014/06/10 10:51:37, pneubeck wrote:
> I'm very uncertain about this working. It seems to rely on the order in which
> Shill sends the kServiceCompleteListProperty and kServices update. If that's
the
> case, IMO that's too fragile.
> 
> Do we have a test checking the following?
> A service X is not visible but 'configured' (i.e. X is in ServiceCompleteList
> but not in kServicesProperty).
> Observer sees it as not visible.
> Service X becomes visible, i.e. X is now also in kServicesProperty.
> >>In the notification call<< (this is the important condition), the observer
> sees X being visible.
> 
> Ideally, we would test this for both update orders (Services vs.
> ServicesCompleteList).
> 
> From reading the code, I'm a bit unsure whether the updating of the visible
> property and sending the notification is always in the correct order (the
> notification in question is send in line 234).

Yes it relies on the order. Yes it's fragile, but it's going away.
I'll add a comment.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:296:
listener_->UpdateVisibleNetworks(*vlist);
On 2014/06/10 10:51:37, pneubeck wrote:
> Note, that Shill is emitting a 'Visible' property for each service since a
while
> (https://chromium-review.googlesource.com/202305, and 
> https://chromium-review.googlesource.com/202136).
> 
> That property could be parsed by NetworkState in the same way as other
> properties. Maybe simpler than relying on the Services list.
> 
> This would also make my previous comment moot.

That is the intention, but this CL has already taken two weeks to complete and I
really need a reasonably stable intermediate point. I've tested this pretty
carefully, and the update order in manager.cc is pretty clear, so would prefer
to move forward with this (commented).
https://chromium-review.googlesource.com/#/c/202223/2/manager.cc

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:238: size_t new_observed_networks_;
On 2014/06/10 10:51:37, pneubeck wrote:
> never read?
Oops. Was used at one point, turned out to be unnecessary. Removed.

[stevenjb, 2014-06-10 23:42:27]

PTAL

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:276: if ((key.empty() || key ==
shill::kServiceCompleteListProperty) &&
On 2014/06/10 16:17:28, stevenjb wrote:
> On 2014/06/10 10:51:37, pneubeck wrote:
> > I'm very uncertain about this working. It seems to rely on the order in
which
> > Shill sends the kServiceCompleteListProperty and kServices update. If that's
> the
> > case, IMO that's too fragile.
> > 
> > Do we have a test checking the following?
> > A service X is not visible but 'configured' (i.e. X is in
ServiceCompleteList
> > but not in kServicesProperty).
> > Observer sees it as not visible.
> > Service X becomes visible, i.e. X is now also in kServicesProperty.
> > >>In the notification call<< (this is the important condition), the observer
> > sees X being visible.
> > 
> > Ideally, we would test this for both update orders (Services vs.
> > ServicesCompleteList).
> > 
> > From reading the code, I'm a bit unsure whether the updating of the visible
> > property and sending the notification is always in the correct order (the
> > notification in question is send in line 234).
> 
> Yes it relies on the order. Yes it's fragile, but it's going away.
> I'll add a comment.
Actually, this part doesn't rely on order, it relies on all updates being
received, which will always occur after both lists have been updated. The call
to UpdateVisibleNetworks() does rely on the order, so I added a comment and TODO
there.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:296:
listener_->UpdateVisibleNetworks(*vlist);
On 2014/06/10 16:17:28, stevenjb wrote:
> On 2014/06/10 10:51:37, pneubeck wrote:
> > Note, that Shill is emitting a 'Visible' property for each service since a
> while
> > (https://chromium-review.googlesource.com/202305, and 
> > https://chromium-review.googlesource.com/202136).
> > 
> > That property could be parsed by NetworkState in the same way as other
> > properties. Maybe simpler than relying on the Services list.
> > 
> > This would also make my previous comment moot.
> 
> That is the intention, but this CL has already taken two weeks to complete and
I
> really need a reasonably stable intermediate point. I've tested this pretty
> carefully, and the update order in manager.cc is pretty clear, so would prefer
> to move forward with this (commented).
> https://chromium-review.googlesource.com/#/c/202223/2/manager.cc

Also, to be clear, I've tested this pretty thoroughly and am highly confident
that it will work. I am slightly less confident of the robustness of Shill
emitting the Visible property changes (although I don't expect there to be any
problems). Thus, I would rather commit this as-is, then follow up with the
Visible property change after, so that we can revert just that change if it
turns out not to be reliable.

[pneubeck (no reviews), 2014-06-11 12:44:56]

Huge step towards simpler network handlers! Thanks.
Mostly minor comments.

One more highlevel comment: please add a test that covers a change in visibility
(best in both directions). That test will be important for the followup change
where you will use the Visible property as well.

lgtm

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:197: if (!network->visible() ||
!network->IsConnectedState())
On 2014/06/10 16:17:28, stevenjb wrote:
> On 2014/06/10 10:51:37, pneubeck wrote:
> > Hm. If it's not visible but in a connected state, isn't that a bug?
> > Better dcheck/log an error?
> It's not necessarily a bug, we don't clear state when a network is removed
from
> the visible list. We shouldn't ever be checking State on non visible networks.
> The tests aren't related to each other, I could separate them if that would be
> more clear.

Ok. Got it. But considering the documentation of IsConnectedState (it doesn't
mention the visibility) and that this behavior is likely to be surprising to new
readers, I would suggest just changing the the connection_state() accessor to
return Idle if !visible() . (I wouldn't update the state in the set_visible()
function; as that would then rely on the order in which visible and connection
state are updated).

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:687: void
NetworkStateHandler::ManagedStateListChanged(
On 2014/06/10 16:17:28, stevenjb wrote:
> On 2014/06/10 10:51:36, pneubeck wrote:
> > again, this is called on ServicesCompleteList changes but 'visible' counter
> > depends on Services being already updated.
> It's called when all updates have been received so should be fine.
> 

_If_ any updates were requested. If no updates were requested, because only the
visibility changed, the behavior might be different.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:733: if (new_service_path ==
default_network_path_)
On 2014/06/10 16:17:28, stevenjb wrote:
> On 2014/06/10 10:51:36, pneubeck wrote:
> > Just noting that this is an unrelated functional change.
> Not entirely unrelated. This CL changes some of the timing and a lot of stub
> behavior so we ended up calling this a bit more frequently. This change makes
> the frequency more predictable.

If you would want to separate it, you could commit this change first. I'm fine
with either.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:276: if ((key.empty() || key ==
shill::kServiceCompleteListProperty) &&
On 2014/06/10 23:42:27, stevenjb wrote:
> On 2014/06/10 16:17:28, stevenjb wrote:
> > On 2014/06/10 10:51:37, pneubeck wrote:
> > > I'm very uncertain about this working. It seems to rely on the order in
> which
> > > Shill sends the kServiceCompleteListProperty and kServices update. If
that's
> > the
> > > case, IMO that's too fragile.
> > > 
> > > Do we have a test checking the following?
> > > A service X is not visible but 'configured' (i.e. X is in
> ServiceCompleteList
> > > but not in kServicesProperty).
> > > Observer sees it as not visible.
> > > Service X becomes visible, i.e. X is now also in kServicesProperty.
> > > >>In the notification call<< (this is the important condition), the
observer
> > > sees X being visible.
> > > 
> > > Ideally, we would test this for both update orders (Services vs.
> > > ServicesCompleteList).
> > > 
> > > From reading the code, I'm a bit unsure whether the updating of the
visible
> > > property and sending the notification is always in the correct order (the
> > > notification in question is send in line 234).
> > 
> > Yes it relies on the order. Yes it's fragile, but it's going away.
> > I'll add a comment.
> Actually, this part doesn't rely on order, it relies on all updates being
> received, which will always occur after both lists have been updated. The call
> to UpdateVisibleNetworks() does rely on the order, so I added a comment and
TODO
> there.
> 

Again, I think you describe only the case where any updates were requested.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:296:
listener_->UpdateVisibleNetworks(*vlist);
On 2014/06/10 23:42:27, stevenjb wrote:
> On 2014/06/10 16:17:28, stevenjb wrote:
> > On 2014/06/10 10:51:37, pneubeck wrote:
> > > Note, that Shill is emitting a 'Visible' property for each service since a
> > while
> > > (https://chromium-review.googlesource.com/202305, and 
> > > https://chromium-review.googlesource.com/202136).
> > > 
> > > That property could be parsed by NetworkState in the same way as other
> > > properties. Maybe simpler than relying on the Services list.
> > > 
> > > This would also make my previous comment moot.
> > 
> > That is the intention, but this CL has already taken two weeks to complete
and
> I
> > really need a reasonably stable intermediate point. I've tested this pretty
> > carefully, and the update order in manager.cc is pretty clear, so would
prefer
> > to move forward with this (commented).
> > https://chromium-review.googlesource.com/#/c/202223/2/manager.cc
> 
> Also, to be clear, I've tested this pretty thoroughly and am highly confident
> that it will work. I am slightly less confident of the robustness of Shill
> emitting the Visible property changes (although I don't expect there to be any
> problems). Thus, I would rather commit this as-is, then follow up with the
> Visible property change after, so that we can revert just that change if it
> turns out not to be reliable.

Assuming that Shill does emit the signals in the required order, this
intermediate step sounds good to me.

You might want to put a second thought into whether pure visibility changes also
currently work (where no network state updates were requested).

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(left):

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:356:
network_info->SetBoolean(kNetworkInfoKeyConnectable, false);
as you mentioned elsewhere, these properties of visible networks are not cleared
if a network becomes invisible.
Could this lead to issues here?

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:323: bool
has_policy = onc::HasPolicyForNetwork(
yuhuu!

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:327:
std::string icon_url = ash::network_icon::GetImageUrlForNetwork(
I didn't look too thoroughly, but the code of GetImageUrlForNetwork seems not
fallback to GetImageUrlForDisconnectedNetwork; also the latter is now unused and
could be removed / or be called by GetImageUrlForNetwork.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (left):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:830: if (property ==
shill::kDevicesProperty) {
Hm. I wonder why this was here in the first place.
Maybe, the property was not always present and this prevented the LOG(ERROR)
below?

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:42: // have Type |match_type| to
either an active list or an inactive list
comment out-of-date: ... to one of the output lists...

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:467: const base::ListValue* services
= GetListProperty(shill::kServicesProperty);
how about letting SortManagerServices instead return whether it changed the
list?

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:518: if (!complete_list ||
complete_list->empty())
could service_list also be NULL? if neither should, DCHECK instead.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:900:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
should this also notify about kServices?

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_service_client.cc:452: // If the State or Visibility
changes, the sort order of Services may change
sort order of Services -> Service lists

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/shill_man...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/shill_man...
chromeos/dbus/shill_manager_client.h:75: // Called by ShillServiceClient when a
service's State property changes.
State or Visible property

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_profile_handler.cc:198: : network_state_handler_(NULL),
dependency on network_state_handler_ not necessary anymore?

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_sms_handler_unittest.cc (left):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_sms_handler_unittest.cc:65:
ShillManagerClient::TestInterface* manager_test =
optional nit: maybe this change could be moved to a separate CL to be committed
before this one.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state.cc:177: void
NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
seems that you dropped the properties NetworkUIData::kKeyONCSource and
shill::kProfileProperty for debugging.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state.h:96: // Returns true if this is a network stored
in a profile (see note above).
the 'see note above' doesn't apply anymore.
maybe some of the class comment of FavoriteState should be moved to this class.

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

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:58: // * "Visible" networks return true
for NetworkState.IsVisible().
IsVisible() -> visible()

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:150: // Convenience method for
GetVisibleNetworkListByType(All).
It's NetworkTypePattern::Default not All, although I like the latter better.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:247: // Updates the visibility if
entries in |network_list_|. This should not
typo 'if entries' ? 
maybe should have been 'with entries'

[stevenjb, 2014-06-11 23:31:41]

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

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:197: if (!network->visible() ||
!network->IsConnectedState())
On 2014/06/11 12:44:55, pneubeck wrote:
> On 2014/06/10 16:17:28, stevenjb wrote:
> > On 2014/06/10 10:51:37, pneubeck wrote:
> > > Hm. If it's not visible but in a connected state, isn't that a bug?
> > > Better dcheck/log an error?
> > It's not necessarily a bug, we don't clear state when a network is removed
> from
> > the visible list. We shouldn't ever be checking State on non visible
networks.
> > The tests aren't related to each other, I could separate them if that would
be
> > more clear.
> 
> Ok. Got it. But considering the documentation of IsConnectedState (it doesn't
> mention the visibility) and that this behavior is likely to be surprising to
new
> readers, I would suggest just changing the the connection_state() accessor to
> return Idle if !visible() . (I wouldn't update the state in the set_visible()
> function; as that would then rely on the order in which visible and connection
> state are updated).

I went ahead and changed connection_state() to to return kDisconnect if not
visible.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:687: void
NetworkStateHandler::ManagedStateListChanged(
On 2014/06/11 12:44:55, pneubeck wrote:
> On 2014/06/10 16:17:28, stevenjb wrote:
> > On 2014/06/10 10:51:36, pneubeck wrote:
> > > again, this is called on ServicesCompleteList changes but 'visible'
counter
> > > depends on Services being already updated.
> > It's called when all updates have been received so should be fine.
> > 
> 
> _If_ any updates were requested. If no updates were requested, because only
the
> visibility changed, the behavior might be different.
Ugh, you're right, there is an annoying case there. I'll add an extra
ManagedStateListChanged() call when kServicesProperty is emitted.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:276: if ((key.empty() || key ==
shill::kServiceCompleteListProperty) &&
On 2014/06/11 12:44:55, pneubeck wrote:
> On 2014/06/10 23:42:27, stevenjb wrote:
> > On 2014/06/10 16:17:28, stevenjb wrote:
> > > On 2014/06/10 10:51:37, pneubeck wrote:
> > > > I'm very uncertain about this working. It seems to rely on the order in
> > which
> > > > Shill sends the kServiceCompleteListProperty and kServices update. If
> that's
> > > the
> > > > case, IMO that's too fragile.
> > > > 
> > > > Do we have a test checking the following?
> > > > A service X is not visible but 'configured' (i.e. X is in
> > ServiceCompleteList
> > > > but not in kServicesProperty).
> > > > Observer sees it as not visible.
> > > > Service X becomes visible, i.e. X is now also in kServicesProperty.
> > > > >>In the notification call<< (this is the important condition), the
> observer
> > > > sees X being visible.
> > > > 
> > > > Ideally, we would test this for both update orders (Services vs.
> > > > ServicesCompleteList).
> > > > 
> > > > From reading the code, I'm a bit unsure whether the updating of the
> visible
> > > > property and sending the notification is always in the correct order
(the
> > > > notification in question is send in line 234).
> > > 
> > > Yes it relies on the order. Yes it's fragile, but it's going away.
> > > I'll add a comment.
> > Actually, this part doesn't rely on order, it relies on all updates being
> > received, which will always occur after both lists have been updated. The
call
> > to UpdateVisibleNetworks() does rely on the order, so I added a comment and
> TODO
> > there.
> > 
> 
> Again, I think you describe only the case where any updates were requested.
Yes, addressed now.

https://codereview.chromium.org/289383004/diff/150001/chromeos/network/shill_...
chromeos/network/shill_property_handler.cc:296:
listener_->UpdateVisibleNetworks(*vlist);
On 2014/06/11 12:44:55, pneubeck wrote:
> On 2014/06/10 23:42:27, stevenjb wrote:
> > On 2014/06/10 16:17:28, stevenjb wrote:
> > > On 2014/06/10 10:51:37, pneubeck wrote:
> > > > Note, that Shill is emitting a 'Visible' property for each service since
a
> > > while
> > > > (https://chromium-review.googlesource.com/202305, and 
> > > > https://chromium-review.googlesource.com/202136).
> > > > 
> > > > That property could be parsed by NetworkState in the same way as other
> > > > properties. Maybe simpler than relying on the Services list.
> > > > 
> > > > This would also make my previous comment moot.
> > > 
> > > That is the intention, but this CL has already taken two weeks to complete
> and
> > I
> > > really need a reasonably stable intermediate point. I've tested this
pretty
> > > carefully, and the update order in manager.cc is pretty clear, so would
> prefer
> > > to move forward with this (commented).
> > > https://chromium-review.googlesource.com/#/c/202223/2/manager.cc
> > 
> > Also, to be clear, I've tested this pretty thoroughly and am highly
confident
> > that it will work. I am slightly less confident of the robustness of Shill
> > emitting the Visible property changes (although I don't expect there to be
any
> > problems). Thus, I would rather commit this as-is, then follow up with the
> > Visible property change after, so that we can revert just that change if it
> > turns out not to be reliable.
> 
> Assuming that Shill does emit the signals in the required order, this
> intermediate step sounds good to me.
> 
> You might want to put a second thought into whether pure visibility changes
also
> currently work (where no network state updates were requested).
Addressed.

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(left):

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:356:
network_info->SetBoolean(kNetworkInfoKeyConnectable, false);
On 2014/06/11 12:44:55, pneubeck wrote:
> as you mentioned elsewhere, these properties of visible networks are not
cleared
> if a network becomes invisible.
> Could this lead to issues here?
Hmm, maybe. Better safe than sorry, I'll fix this too.

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:323: bool
has_policy = onc::HasPolicyForNetwork(
On 2014/06/11 12:44:55, pneubeck wrote:
> yuhuu!
:)

https://codereview.chromium.org/289383004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:327:
std::string icon_url = ash::network_icon::GetImageUrlForNetwork(
On 2014/06/11 12:44:55, pneubeck wrote:
> I didn't look too thoroughly, but the code of GetImageUrlForNetwork seems not
> fallback to GetImageUrlForDisconnectedNetwork; also the latter is now unused
and
> could be removed / or be called by GetImageUrlForNetwork.
Good catch on GetImageUrlForDisconnectedNetwork no longer being needed, and we
should fallback if !visible. Fixed.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (left):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:830: if (property ==
shill::kDevicesProperty) {
On 2014/06/11 12:44:55, pneubeck wrote:
> Hm. I wonder why this was here in the first place.
> Maybe, the property was not always present and this prevented the LOG(ERROR)
> below?

I believe the code simply wasn't cleaned up doing some earlier re-factoring. The
ERROR should be legitimate.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:42: // have Type |match_type| to
either an active list or an inactive list
On 2014/06/11 12:44:55, pneubeck wrote:
> comment out-of-date: ... to one of the output lists...

Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:467: const base::ListValue* services
= GetListProperty(shill::kServicesProperty);
On 2014/06/11 12:44:55, pneubeck wrote:
> how about letting SortManagerServices instead return whether it changed the
> list?
That would be more efficient, but it's confusing since it modifies both
ServiceCompleteList and Services, so returning true if 'Services' changed would
be awkward. This will get cleaned up shortly, so I think we can live with the
inefficiency for now.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:518: if (!complete_list ||
complete_list->empty())
On 2014/06/11 12:44:55, pneubeck wrote:
> could service_list also be NULL? if neither should, DCHECK instead.
Neither can be NULL, removed the test. I don't think a DCHECK is really needed
since it's an internal function call.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:900:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
On 2014/06/11 12:44:55, pneubeck wrote:
> should this also notify about kServices?
Yes. Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_service_client.cc:452: // If the State or Visibility
changes, the sort order of Services may change
On 2014/06/11 12:44:55, pneubeck wrote:
> sort order of Services -> Service lists

Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/shill_man...
File chromeos/dbus/shill_manager_client.h (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/dbus/shill_man...
chromeos/dbus/shill_manager_client.h:75: // Called by ShillServiceClient when a
service's State property changes.
On 2014/06/11 12:44:56, pneubeck wrote:
> State or Visible property

Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_profile_handler.cc:198: : network_state_handler_(NULL),
On 2014/06/11 12:44:56, pneubeck wrote:
> dependency on network_state_handler_ not necessary anymore?

Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_sms_handler_unittest.cc (left):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_sms_handler_unittest.cc:65:
ShillManagerClient::TestInterface* manager_test =
On 2014/06/11 12:44:56, pneubeck wrote:
> optional nit: maybe this change could be moved to a separate CL to be
committed
> before this one.
meh

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state.cc:177: void
NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
On 2014/06/11 12:44:56, pneubeck wrote:
> seems that you dropped the properties NetworkUIData::kKeyONCSource and
> shill::kProfileProperty for debugging.

Yes, that was intentional, we now add those explicitly in
TranslateNetworkListToONC based on a 'debugging' flag instead.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state.h:96: // Returns true if this is a network stored
in a profile (see note above).
On 2014/06/11 12:44:56, pneubeck wrote:
> the 'see note above' doesn't apply anymore.
> maybe some of the class comment of FavoriteState should be moved to this
class.

Done.

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

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:58: // * "Visible" networks return true
for NetworkState.IsVisible().
On 2014/06/11 12:44:56, pneubeck wrote:
> IsVisible() -> visible()

Done.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:150: // Convenience method for
GetVisibleNetworkListByType(All).
On 2014/06/11 12:44:56, pneubeck wrote:
> It's NetworkTypePattern::Default not All, although I like the latter better.
Yeah, I was going to add "All" to be synonymous but decided that would just be
confusing. Here "All" is better, but for
{Connected|Connecting|First}NetworkByType(), "Default" is maybe better. Maybe
not, we could change it later.

https://codereview.chromium.org/289383004/diff/170001/chromeos/network/networ...
chromeos/network/network_state_handler.h:247: // Updates the visibility if
entries in |network_list_|. This should not
On 2014/06/11 12:44:56, pneubeck wrote:
> typo 'if entries' ? 
> maybe should have been 'with entries'
'of' :)

[pneubeck (no reviews), 2014-06-12 07:55:46]

lgtm!

[stevenjb, 2014-06-12 16:42:05]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-12 16:48:57]

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

[commit-bot: I haz the power, 2014-06-12 20:31:17]

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...)

[stevenjb, 2014-06-12 21:48:53]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2014-06-12 22:08:55]

Committed patchset #14 manually as r276822 (presubmit successful).