Use GUID instead of ServicePath in networkingPrivate API

Use GUID instead of ServicePath in networkingPrivate API

Currently we pass the service path of networks as the "GUID" in the
networkingPrivate implementation on Chrome OS. We should be using the
actual GUID (if one exists) or create a GUID if one does not.

Note: This change should not break any existing usage since the GUID
value should be transparant to the implementation. (This will have the
positive side effect of making the GUID consistent across sessions).

BUG=284827
For fake_wifi_service.cc:

R=asargent@chromium.org, pneubeck@chromium.org
TBR=mef@chromium.org

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

[stevenjb, 2014-05-08 02:35:46]

OK, at long last, I think this will work. It's not quite as bad as it looks, but
some of it is tricky / subtle code, so take your time. I'll do a self review of
my own tomorrow when I am fresh if you want to wait a little.

[pneubeck (no reviews), 2014-05-08 13:25:12]

did only a quick look at it and looks already rather good to me.

We should have a test like (networkingPrivate API test):
- Wifi with GUID XY
- extension changes security type of wifi with GUID XY
- extension verifies that security type was correctly changed by looking up wifi
with GUID XY
That's to validate that GUID survives security changes.

There should also be some documentation about the GUIDs properties, like: stable
until session end; persistent if network is configured / IsInProfile()==true;
random identifier; any service has a GUID assigned from the very beginning (once
network->update_received()==true); ...

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:335: if (!guid.empty()) {
if the assumption is from now on that a service always has a guid, then this
maybe should better be DCHECK'ed.

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/network...
chromeos/network/network_state.cc:143: // By convention, all visible WiFi
networks have a SignalStrength > 0.
unrelated?

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:462:
scoped_ptr<base::ListValue> TranslateShillNetworkListToONC(
see my comment in the other CL. Please move out of network/onc/ into e.g.
network/network_util

[stevenjb, 2014-05-08 22:32:26]

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:335: if (!guid.empty()) {
On 2014/05/08 13:25:12, pneubeck wrote:
> if the assumption is from now on that a service always has a guid, then this
> maybe should better be DCHECK'ed.

The NetworkHandler layer enfoces that a service always has a guid. Shill and the
Shill*Client layer do not make that assumption. I will add a comment.

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/network...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/network...
chromeos/network/network_state.cc:143: // By convention, all visible WiFi
networks have a SignalStrength > 0.
On 2014/05/08 13:25:12, pneubeck wrote:
> unrelated?
Yeah, I guess it is. I added this while working on
networkingPrivate.getNetworks, I'll save it for that CL.

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:462:
scoped_ptr<base::ListValue> TranslateShillNetworkListToONC(
On 2014/05/08 13:25:12, pneubeck wrote:
> see my comment in the other CL. Please move out of network/onc/ into e.g.
> network/network_util

Done, but note: there is a dependency on NetworkState::StateIsConnected()

[stevenjb, 2014-05-09 19:31:13]

PTAL

It turns out that the setProperties() already effectively tests GUID lookup
after changing the Security property, so I just added an additional explicit
GUID check rather than introducing the overhead of an additional test.

I also added a bunch of comment about Network/FavoriteState and GUIDS in
network_state_handler.h.

+asargent@ for networking_private.json (the changes should not affect any
existing implementations, just clarify documentation and argument names).

[asargent_no_longer_on_chrome, 2014-05-10 14:24:28]

extensions lgtm

[pneubeck (no reviews), 2014-05-12 13:37:06]

Mostly nits except one major concern:

Currently you never remove from the SpecifierGuidMap.
Assume, a network is not stored but visible.
-> it's put into SpecifierGuidMap (with specifier X)
Then the network is configured and stored
-> it's stays in SpecifierGuidMap.
Then the name/security type is changed (specifier is now Y with Y!=X)
-> the guid is still in the Map _and_ assigned to the network, but these have
different specifiers X!=Y.

Compare this to a network that was initially stored.
Then it is never put into the SpecifierGuidMap.

To resolve this, you should remove an entry from the map when the respective
network is stored into a profile.

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:335: if (!guid.empty()) {
On 2014/05/08 22:32:26, stevenjb wrote:
> On 2014/05/08 13:25:12, pneubeck wrote:
> > if the assumption is from now on that a service always has a guid, then this
> > maybe should better be DCHECK'ed.
> 
> The NetworkHandler layer enfoces that a service always has a guid. Shill and
the
> Shill*Client layer do not make that assumption. I will add a comment.

oops. yes, you're right.

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/275543005/diff/10001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:462:
scoped_ptr<base::ListValue> TranslateShillNetworkListToONC(
On 2014/05/08 22:32:26, stevenjb wrote:
> On 2014/05/08 13:25:12, pneubeck wrote:
> > see my comment in the other CL. Please move out of network/onc/ into e.g.
> > network/network_util
> 
> Done, but note: there is a dependency on NetworkState::StateIsConnected()
Yes, that one is only comparing to Shill constants and could be part of
shill_property_utils.
At least it's only a static function.

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
(right):

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:146:
std::string user_id_hash;
?? how has this ever worked?
   ';' -> '='

please verify also in the other functions

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:191:
error_ = "Error.NetworkUnavailable";
I think, GetState should also return successfully for not in-range favorite
networks; likely with a reduced set of properties. At a minimum, it must be
documented in the API description.

However better not in this CL.

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:89:
callbackFail("Error.InvalidNetworkGuid"));
optional nit: the 'Error.' prefix seems superfluous.
We might have to do a review of all error strings at some time anyways and
document them as well in the API description.

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:247: "Active":
"stub_wifi2",
hm. This should be changed to 'Unmanaged' or to the old version.
I opened a bug to fix this also for SSID: crbug.com/372337

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:302: var guid =
result.GUID;
isn't "stub_wifi2" two lines above already the expected guid?
Could you make a variable after line 298 named 'expected_guid' and check it in
all callbacks?

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:253:
service_client->AddServiceWithIPConfig(service_path, guid, guid, type,
nit: add inline comments to clarify the two 'guid' arguments.
note: there is also already 'service_path = guid' above.

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:340:
base::Value::CreateStringValue(guid));
optional nit: CreateStringValue is deprecated, use 'new base::StringValue(..)'
instead.

(optional since we use it all over the place here)

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
File chromeos/network/favorite_state.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.cc:67: // Only used for GetSpecifier()
this comment should be at the declaration of security_

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.cc:85: std::string FavoriteState::GetSpecifier()
const {
I'd prefer to return "" as long as !update_received() to reduce 'flakiness'.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:22: // saved. This is necessary to avoid
unnecessarily re-requesting entries,
not sure about the English here, but is 'stored' maybe more applicable than
'saved'?

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:49: // Returns a specifier for identifying
this network in the absence of a GUID.
maybe mention that this depends on other properties and thus is only valid once
update_received()==true.

better also mention when to (or who should) use this compared to path() and
guid().

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:60: // Keep track of the secuirity for
purposes of specifying wifi networks.
secuirity -> security

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:245:
InvokeErrorCallback(service_path, error_callback, kUnknownServicePath);
nit: the error is not fully correct anymore. more correct would be something
like 'UnknownFavoriteState'

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:337: const FavoriteState* favorite =
(*iter)->AsFavoriteState();
skip if update_received()==false (might not be necessary if guid() return '' if
the required properties are not received)

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:504: managed =
ManagedState::Create(type, path);
optional:
since |type| is constant, IMO the more specific 'new FavoriteState' is
preferable.

up to you.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:746: // Ensure that a FavoriteState
has a valid GUID.
nit: a -> the
(or even remove the comment)

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:764: // If the GUID is not set and a
corresponding FavoriteState exists, get the
hm.
There's a again a timing subtlety here, I think.
Usually, a NetworkState is 'valid' once the first full set of properties was
received and this was indicated by 'update_received()==true'.

With this change, could we have 'update_received()==true' and the guid still be
unset?
_Maybe_ this can't happen because you create the FavoriteState in
UpdateManagedStateProperties(
          ManagedState::MANAGED_TYPE_FAVORITE) before the NetworkState, but
that's at least not that obvious.

Could you make this independent of the order FavoriteState compared to
NetworkState? That would reduce the implicit dependencies on other code.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.h:194: // may or may not be in a profile
(IsProfile() may be false), and may or may
it's a bit unlucky that the name is so similar to the above GetFavorite*
functions but the behavior is rather different (IsProfile() may be false).

to reduce mistakes, maybe add a comment at the other GetFavorite* functions that
it 'ensures that IsProfile() is true';
or rename this one to 'GetStateFromGuid'.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.h:387: // Map of network specifiers to
guids.
According to your class comment, you should ensure the invariant:
'Contains a key /exactly/ for each specifier of every visible network with
IsProfile()==false'
(i.e. no entry if IsProfile()==true)

please comment it here.

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:486:
TEST_F(NetworkStateHandlerTest, NetworkGuid) {
you need a second test that does more or less the same but with
profile_path()=="" (and where the network goes out-of-range and back)

a third test could be: a visible wifi with InProfile()==false which is then
stored; the GUID should then be present in the profile.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:492:
ShillProfileClient::TestInterface* profile_test =
nit: better move this to the test class to make the test more readable.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:496: // Verify that a
FavoriteState existis with a valid GUID.
existis -> exists

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:498:
network_state_handler_->GetFavoriteState(wifi_path);
you could also test GetFavoriteStateFromGuid() here.

To not make test longer, you could at first do GetNetworkState and then
GetFavoriteSTateFromGuid.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:502: // Verify that a
NetworkState existis with the same GUID.
existis -> exists

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:508:
service_test_->RemoveService(wifi_path);
I think you're not testing the right thing anymore from this point.

service_test_->RemoveService would mean, that the network goes out-of-range
_and_ the user is removing its configuration.
See my toplevel comment what the behavior in that case could be.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
File chromeos/network/network_util.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_util.h:17: #include "base/callback.h"
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_util.h:89: // ONC dictionaries using
TranslateShillServiceToONCPart.
reference NetworkStateHandler::NetworkList (so that one knows that this returns
only the list of visible networks)

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/shill_p...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/shill_p...
chromeos/network/shill_property_handler.cc:487: // for all networks, regardles
of whether or not Profile is set, because
regardles -> regardless

[pneubeck (no reviews), 2014-05-12 13:39:19]

On 2014/05/12 13:37:06, pneubeck wrote:
> Mostly nits except one major concern:
> 
> Currently you never remove from the SpecifierGuidMap.
> Assume, a network is not stored but visible.
> -> it's put into SpecifierGuidMap (with specifier X)
> Then the network is configured and stored
> -> it's stays in SpecifierGuidMap.
> Then the name/security type is changed (specifier is now Y with Y!=X)
> -> the guid is still in the Map _and_ assigned to the network, but these have
> different specifiers X!=Y.
> 
> Compare this to a network that was initially stored.
> Then it is never put into the SpecifierGuidMap.
> 
> To resolve this, you should remove an entry from the map when the respective
> network is stored into a profile.

Alternatively, you could also add an entry to the map for _every_ stored network
and update the entry if name/ssid/security changes.

[stevenjb, 2014-05-13 01:18:59]

I addressed the mapping issue by removing saved entries from the map, I think
that is better than including all entries.

I also went ahead and separated out the FakeShill* changes, please review those
also since this CL is dependent on that one now.
https://codereview.chromium.org/284673004/

Thanks!

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
(right):

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:146:
std::string user_id_hash;
On 2014/05/12 13:37:07, pneubeck wrote:
> ?? how has this ever worked?
>    ';' -> '='
> 
> please verify also in the other functions
Yeah, the 'user_id_hash' code is completely broken and untested. I started a fix
in https://codereview.chromium.org/242983004/ if you recall, but have not gotten
back to it (in part because the desired behavior was uncertain).

The latest decision is that networkingPrivate should always affect the default
user profile in a multi-profile environment (any thing else is just too
confusing) so we may need to change the API or at least the documentation.

Regardless, I will deal with this in that CL.

https://codereview.chromium.org/275543005/diff/70001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:191:
error_ = "Error.NetworkUnavailable";
On 2014/05/12 13:37:07, pneubeck wrote:
> I think, GetState should also return successfully for not in-range favorite
> networks; likely with a reduced set of properties. At a minimum, it must be
> documented in the API description.
> 
> However better not in this CL.

In the "getNetworks" CL I fix this for getNetworks and getVisibleNetworks, I
will make a note to fix this for getState also.

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:89:
callbackFail("Error.InvalidNetworkGuid"));
On 2014/05/12 13:37:07, pneubeck wrote:
> optional nit: the 'Error.' prefix seems superfluous.
> We might have to do a review of all error strings at some time anyways and
> document them as well in the API description.

Agreed that we should audit these, but in a separate CL. Need more Dev
resources...

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:247: "Active":
"stub_wifi2",
On 2014/05/12 13:37:07, pneubeck wrote:
> hm. This should be changed to 'Unmanaged' or to the old version.
> I opened a bug to fix this also for SSID: crbug.com/372337
Awesome.

https://codereview.chromium.org/275543005/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:302: var guid =
result.GUID;
On 2014/05/12 13:37:07, pneubeck wrote:
> isn't "stub_wifi2" two lines above already the expected guid?
> Could you make a variable after line 298 named 'expected_guid' and check it in
> all callbacks?

Done.

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:253:
service_client->AddServiceWithIPConfig(service_path, guid, guid, type,
On 2014/05/12 13:37:07, pneubeck wrote:
> nit: add inline comments to clarify the two 'guid' arguments.
> note: there is also already 'service_path = guid' above.

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_service_client.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_service_client.cc:340:
base::Value::CreateStringValue(guid));
On 2014/05/12 13:37:07, pneubeck wrote:
> optional nit: CreateStringValue is deprecated, use 'new base::StringValue(..)'
> instead.
> 
> (optional since we use it all over the place here)
Fixed throughout this file (not too many).

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
File chromeos/network/favorite_state.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.cc:67: // Only used for GetSpecifier()
On 2014/05/12 13:37:07, pneubeck wrote:
> this comment should be at the declaration of security_

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.cc:85: std::string FavoriteState::GetSpecifier()
const {
On 2014/05/12 13:37:07, pneubeck wrote:
> I'd prefer to return "" as long as !update_received() to reduce 'flakiness'.

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:22: // saved. This is necessary to avoid
unnecessarily re-requesting entries,
On 2014/05/12 13:37:07, pneubeck wrote:
> not sure about the English here, but is 'stored' maybe more applicable than
> 'saved'?
'saved' is more commonly used in this context (e.g. 'save password') even though
'stored' is more precise. I think 'stored' would cause the reader to think about
'where and how', whereas 'saved' is a more generic hand-wavy concept. If this
were in the Shill*Client code, 'stored' might be better, but here I think
'saved' is better.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:49: // Returns a specifier for identifying
this network in the absence of a GUID.
On 2014/05/12 13:37:07, pneubeck wrote:
> maybe mention that this depends on other properties and thus is only valid
once
> update_received()==true.
> 
> better also mention when to (or who should) use this compared to path() and
> guid().
Comment updated.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/favorit...
chromeos/network/favorite_state.h:60: // Keep track of the secuirity for
purposes of specifying wifi networks.
On 2014/05/12 13:37:07, pneubeck wrote:
> secuirity -> security

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:245:
InvokeErrorCallback(service_path, error_callback, kUnknownServicePath);
On 2014/05/12 13:37:07, pneubeck wrote:
> nit: the error is not fully correct anymore. more correct would be something
> like 'UnknownFavoriteState'
-> 'UnknownNetwork'

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:337: const FavoriteState* favorite =
(*iter)->AsFavoriteState();
On 2014/05/12 13:37:07, pneubeck wrote:
> skip if update_received()==false (might not be necessary if guid() return ''
if
> the required properties are not received)
guid() will be "" until update_received() is true. I'll DCHECK that 'guid' is
not empty.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:504: managed =
ManagedState::Create(type, path);
On 2014/05/12 13:37:07, pneubeck wrote:
> optional:
> since |type| is constant, IMO the more specific 'new FavoriteState' is
> preferable.
> 
> up to you.

I didn't like that NSH sometimes uses one and not the other. I was going to add
a Create wrapper and do some logic in there but I reverted that, but decided I
preferred keeping this. (I considered protecting the constructors but enough
tests and other code construct local *State members that I decided not to).

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:746: // Ensure that a FavoriteState
has a valid GUID.
On 2014/05/12 13:37:07, pneubeck wrote:
> nit: a -> the
> (or even remove the comment)

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.cc:764: // If the GUID is not set and a
corresponding FavoriteState exists, get the
On 2014/05/12 13:37:07, pneubeck wrote:
> hm.
> There's a again a timing subtlety here, I think.
> Usually, a NetworkState is 'valid' once the first full set of properties was
> received and this was indicated by 'update_received()==true'.
> 
> With this change, could we have 'update_received()==true' and the guid still
be
> unset?
> _Maybe_ this can't happen because you create the FavoriteState in
> UpdateManagedStateProperties(
>           ManagedState::MANAGED_TYPE_FAVORITE) before the NetworkState, but
> that's at least not that obvious.
> 
> Could you make this independent of the order FavoriteState compared to
> NetworkState? That would reduce the implicit dependencies on other code.

I don't think we can ever entirely guarantee that a FavoriteState exists for
every NetworkState, and I'm not sure that we should try.

I think that the correct thing to do is to remove guid() from NetworkState.
There are some places where we really should use FavoriteState instead of
NetworkState anyway. I looked into removing it and it's going to be a PITA, so I
will do it separately.

Meanwhile, if a network was saved, NetworkState will still have a guid as
before. If it wasn't saved, it will eventually have a guid, but might not, so we
can't write code that relies on it. I will add that to the comments.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.h:194: // may or may not be in a profile
(IsProfile() may be false), and may or may
On 2014/05/12 13:37:07, pneubeck wrote:
> it's a bit unlucky that the name is so similar to the above GetFavorite*
> functions but the behavior is rather different (IsProfile() may be false).
> 
> to reduce mistakes, maybe add a comment at the other GetFavorite* functions
that
> it 'ensures that IsProfile() is true';
> or rename this one to 'GetStateFromGuid'.

GetFavoriteState -> GetFravoriteStateFromServicePath with 'is_configured' as an
argument (which I think is a useful clarification to any code that calls it).

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.h:387: // Map of network specifiers to
guids.
On 2014/05/12 13:37:07, pneubeck wrote:
> According to your class comment, you should ensure the invariant:
> 'Contains a key /exactly/ for each specifier of every visible network with
> IsProfile()==false'
> (i.e. no entry if IsProfile()==true)
> 
> please comment it here.

Done.

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:486:
TEST_F(NetworkStateHandlerTest, NetworkGuid) {
On 2014/05/12 13:37:07, pneubeck wrote:
> you need a second test that does more or less the same but with
> profile_path()=="" (and where the network goes out-of-range and back)
> 
> a third test could be: a visible wifi with InProfile()==false which is then
> stored; the GUID should then be present in the profile.

Good point. I added the second test. The "InProfile" test effectively includes
the "third test" because it has to retrieve the saved guid from the Profile. (In
fact, my changes to NSH that update the map caused the InProfile test to fail,
correctly; I had to make a bunch of changes to FakeShillProfileClient to make it
work. I may try to separate those out).

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:492:
ShillProfileClient::TestInterface* profile_test =
On 2014/05/12 13:37:07, pneubeck wrote:
> nit: better move this to the test class to make the test more readable.

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:496: // Verify that a
FavoriteState existis with a valid GUID.
On 2014/05/12 13:37:07, pneubeck wrote:
> existis -> exists

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:498:
network_state_handler_->GetFavoriteState(wifi_path);
On 2014/05/12 13:37:07, pneubeck wrote:
> you could also test GetFavoriteStateFromGuid() here.
> 
> To not make test longer, you could at first do GetNetworkState and then
> GetFavoriteSTateFromGuid.
Added a GetState test

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:502: // Verify that a
NetworkState existis with the same GUID.
On 2014/05/12 13:37:07, pneubeck wrote:
> existis -> exists

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:508:
service_test_->RemoveService(wifi_path);
On 2014/05/12 13:37:07, pneubeck wrote:
> I think you're not testing the right thing anymore from this point.
> 
> service_test_->RemoveService would mean, that the network goes out-of-range
> _and_ the user is removing its configuration.
> See my toplevel comment what the behavior in that case could be.

ServiceClientTest::RemoveService does *NOT* remove the service from the Profile,
so this is testing "going out of range".

That said, the two separate tests should be a little more clear now. Hopefully.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
File chromeos/network/network_util.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_util.h:17: #include "base/callback.h"
On 2014/05/12 13:37:07, pneubeck wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_util.h:89: // ONC dictionaries using
TranslateShillServiceToONCPart.
On 2014/05/12 13:37:07, pneubeck wrote:
> reference NetworkStateHandler::NetworkList (so that one knows that this
returns
> only the list of visible networks)
This actually changes in the getNetworks CL to specify configured, visible, but
I'll go ahead and add a comment in the interim.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/shill_p...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/shill_p...
chromeos/network/shill_property_handler.cc:487: // for all networks, regardles
of whether or not Profile is set, because
On 2014/05/12 13:37:07, pneubeck wrote:
> regardles -> regardless

Done.

[pneubeck (no reviews), 2014-05-13 08:43:54]

lgtm with a few comments/questions.

I will review the other CL tomorrow, no time left today.

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler.h:194: // may or may not be in a profile
(IsProfile() may be false), and may or may
On 2014/05/13 01:19:00, stevenjb wrote:
> On 2014/05/12 13:37:07, pneubeck wrote:
> > it's a bit unlucky that the name is so similar to the above GetFavorite*
> > functions but the behavior is rather different (IsProfile() may be false).
> > 
> > to reduce mistakes, maybe add a comment at the other GetFavorite* functions
> that
> > it 'ensures that IsProfile() is true';
> > or rename this one to 'GetStateFromGuid'.
> 
> GetFavoriteState -> GetFravoriteStateFromServicePath with 'is_configured' as
an
> argument (which I think is a useful clarification to any code that calls it).
> 

yes, that's much better.

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:508:
service_test_->RemoveService(wifi_path);
On 2014/05/13 01:19:00, stevenjb wrote:
> On 2014/05/12 13:37:07, pneubeck wrote:
> > I think you're not testing the right thing anymore from this point.
> > 
> > service_test_->RemoveService would mean, that the network goes out-of-range
> > _and_ the user is removing its configuration.
> > See my toplevel comment what the behavior in that case could be.
> 
> ServiceClientTest::RemoveService does *NOT* remove the service from the
Profile,
> so this is testing "going out of range".

In that case and if the profile isn't removed, shouldn't the FavoriteState
continue to exist?
'out of range' means that the service is only removed from the Manager's
Services property but stays in the Manager's ServiceCompleteList.

So what you're testing is more like a ProfilePop and ProfilePush of Shill (as it
happens on a logout/login).

> 
> That said, the two separate tests should be a little more clear now.
Hopefully.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/favori...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/favori...
chromeos/network/favorite_state.h:62: // Keep track of Service.Sercurity. Only
used for specifying wifi networks.
typo: Sercurity -> Security

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

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:781: FavoriteState* favorite =
GetModifiableFavoriteState(network->path());
at least add a log message here if !favorite

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

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler.h:391: // FavoriteState that is not saved
in a profile, and only for unsaved entries.
last 'entries' -> 'networks'/'FavoriteStates' ?

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:529:
UpdateManagerProperties();
Just to restate what's going on here (for my own clarification):


Here the network handlers see a profile push and a configured wifi (with an
existing GUID) appearing.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:545:
service_test_->RemoveService(wifi_path);
the service is completely disappearing as if the profile were popped

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:548:
EXPECT_FALSE(network_state_handler_->GetFavoriteStateFromServicePath(
this verifies unrelated to GUID that the network/favoriteState is removed. I
think this should be in separate test.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:551: // Add the service and
verify that a FavorteState exists with the same guid.
instead of relying on the FakeShillProfileClient to correctly be updated during
the test, you could just manually set the clients to a fixed state here (maybe
keeping the complexity of the FakeClients lower).

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:552: AddService(wifi_path,
wifi_path, shill::kTypeWifi, shill::kStateOnline);
this makes the service appear again (like a ProfilePush).

This part is required as NetworkStateHandler keeps state in the SpecifierGuidMap
and might mix stuff up.

Good. Maybe drop a rational, why this second part is here.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:589:
EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
as in the previous test, this is unrelated an should be tested separately; you
could remove these two EXPECTs.

[stevenjb, 2014-05-13 18:25:06]

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:508:
service_test_->RemoveService(wifi_path);
On 2014/05/13 08:43:54, pneubeck wrote:
> On 2014/05/13 01:19:00, stevenjb wrote:
> > On 2014/05/12 13:37:07, pneubeck wrote:
> > > I think you're not testing the right thing anymore from this point.
> > > 
> > > service_test_->RemoveService would mean, that the network goes
out-of-range
> > > _and_ the user is removing its configuration.
> > > See my toplevel comment what the behavior in that case could be.
> > 
> > ServiceClientTest::RemoveService does *NOT* remove the service from the
> Profile,
> > so this is testing "going out of range".
> 
> In that case and if the profile isn't removed, shouldn't the FavoriteState
> continue to exist?
> 'out of range' means that the service is only removed from the Manager's
> Services property but stays in the Manager's ServiceCompleteList.
> 
> So what you're testing is more like a ProfilePop and ProfilePush of Shill (as
it
> happens on a logout/login).
> 
> > 
> > That said, the two separate tests should be a little more clear now.
> Hopefully.
> 
Well, the Fake clients are just an approximation. Shill would never have a
Profile entry but no Service. I can add a comment, but the intention here is to
test NSH correctly handling a FavortieState disappearing and re-appearing, e.g.
after a Shill restart, or profile pop/push, or a re-configure, e.g. by policy.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/favori...
File chromeos/network/favorite_state.h (right):

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/favori...
chromeos/network/favorite_state.h:62: // Keep track of Service.Sercurity. Only
used for specifying wifi networks.
On 2014/05/13 08:43:54, pneubeck wrote:
> typo: Sercurity -> Security

Done.

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

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:781: FavoriteState* favorite =
GetModifiableFavoriteState(network->path());
On 2014/05/13 08:43:54, pneubeck wrote:
> at least add a log message here if !favorite
Hmm. I don't want to create log spam here, but in practice it shouldn't happen.
I guess should add a DCHECK until we eliminate NetworkState::guid entirely.

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

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler.h:391: // FavoriteState that is not saved
in a profile, and only for unsaved entries.
On 2014/05/13 08:43:54, pneubeck wrote:
> last 'entries' -> 'networks'/'FavoriteStates' ?

Removed last clause, it was just confusing.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:529:
UpdateManagerProperties();
On 2014/05/13 08:43:54, pneubeck wrote:
> Just to restate what's going on here (for my own clarification):
> 
> 
> Here the network handlers see a profile push and a configured wifi (with an
> existing GUID) appearing.

I'll add comments and break these each into two tests, although the second test
effectively has to duplicate the setup from the first test to be useful.

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:551: // Add the service and
verify that a FavorteState exists with the same guid.
On 2014/05/13 08:43:54, pneubeck wrote:
> instead of relying on the FakeShillProfileClient to correctly be updated
during
> the test, you could just manually set the clients to a fixed state here (maybe
> keeping the complexity of the FakeClients lower).

I have intentionally avoided adding fake behavior to the NSH classes. It is much
cleaner to me to make the FakeShill*Client behavior more closely match Shill
behavior than to maintain two layers of fake behavior.

[pneubeck (no reviews), 2014-05-14 08:46:06]

still lgtm with minor comments.

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

https://codereview.chromium.org/275543005/diff/70001/chromeos/network/network...
chromeos/network/network_state_handler_unittest.cc:508:
service_test_->RemoveService(wifi_path);
On 2014/05/13 18:25:07, stevenjb wrote:
> On 2014/05/13 08:43:54, pneubeck wrote:
> > On 2014/05/13 01:19:00, stevenjb wrote:
> > > On 2014/05/12 13:37:07, pneubeck wrote:
> > > > I think you're not testing the right thing anymore from this point.
> > > > 
> > > > service_test_->RemoveService would mean, that the network goes
> out-of-range
> > > > _and_ the user is removing its configuration.
> > > > See my toplevel comment what the behavior in that case could be.
> > > 
> > > ServiceClientTest::RemoveService does *NOT* remove the service from the
> > Profile,
> > > so this is testing "going out of range".
> > 
> > In that case and if the profile isn't removed, shouldn't the FavoriteState
> > continue to exist?
> > 'out of range' means that the service is only removed from the Manager's
> > Services property but stays in the Manager's ServiceCompleteList.
> > 
> > So what you're testing is more like a ProfilePop and ProfilePush of Shill
(as
> it
> > happens on a logout/login).
> > 
> > > 
> > > That said, the two separate tests should be a little more clear now.
> > Hopefully.
> > 
> Well, the Fake clients are just an approximation. Shill would never have a
> Profile entry but no Service. I can add a comment, but the intention here is
to
> test NSH correctly handling a FavortieState disappearing and re-appearing,
e.g.
> after a Shill restart, or profile pop/push, or a re-configure, e.g. by policy.
> 

so why the misleading 'out-of-range' comment that is wrong?
Just call it 'profile pop/push' or 'Shill restart'.

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

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler.cc:781: FavoriteState* favorite =
GetModifiableFavoriteState(network->path());
On 2014/05/13 18:25:07, stevenjb wrote:
> On 2014/05/13 08:43:54, pneubeck wrote:
> > at least add a log message here if !favorite
> Hmm. I don't want to create log spam here, but in practice it shouldn't
happen.
> I guess should add a DCHECK until we eliminate NetworkState::guid entirely.
> 

DCHECKs shouldn't be handled (next line with if(favorite..) ) ;-)

On non debug builds you will never find out whether this case occurs.
If it shouldn't happen, why would a log message spam the logs?

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/275543005/diff/130001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:551: // Add the service and
verify that a FavorteState exists with the same guid.
On 2014/05/13 18:25:07, stevenjb wrote:
> On 2014/05/13 08:43:54, pneubeck wrote:
> > instead of relying on the FakeShillProfileClient to correctly be updated
> during
> > the test, you could just manually set the clients to a fixed state here
(maybe
> > keeping the complexity of the FakeClients lower).
> 
> I have intentionally avoided adding fake behavior to the NSH classes. It is
much
> cleaner to me to make the FakeShill*Client behavior more closely match Shill
> behavior than to maintain two layers of fake behavior.

Sure, I didn't want to imply that.

What I meant is: The Handlers don't know how the FakeClients were modified until
they are notified. Thus, behind the scenes you can manually put the FakeClients
into whatever state you want, without relying on the FakeClients doing that
themselves.

Effectively, that would mean that the FakeClient doesn't need your latest
modifications, and you would replace line 566 by a copy of 533-541,543.

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

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:381:
TEST_F(NetworkStateHandlerTest, GetState) {
Looking at the names 'GetState' vs. 'FavoriteState' and also the tests
themselves, it's unclear to me what the difference is / what is actually tested.

Can these be merged or alternatively made more distinguished (e.g. GetState ->
NetworkState and remove *Favorite* from it)?

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:532: // And a network to the
default Profile with a specified GUID.
typo: And -> Add

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:562:
EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
this EXPECT should also be moved to test 'FavoriteState' or 'GetState'.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:571: }
optional: at the end of this test and the next one, you could again verify that
the FavoriteState also has the correct GUID.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:577: // And a network without
adding it to a profile.
typo: And -> Add

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:598:
EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
remove

[stevenjb, 2014-05-14 17:31:55]

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

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:381:
TEST_F(NetworkStateHandlerTest, GetState) {
On 2014/05/14 08:46:07, pneubeck wrote:
> Looking at the names 'GetState' vs. 'FavoriteState' and also the tests
> themselves, it's unclear to me what the difference is / what is actually
tested.
> 
> Can these be merged or alternatively made more distinguished (e.g. GetState ->
> NetworkState and remove *Favorite* from it)?

I combined these and cleaned them up a little.
I decided with the previous CL not to explicitly test that a FavoriteState no
longer exists after "RemoveService" because it's not really clear what that
should do. At some point we should make that "RemoveVisibleService" and keep it
in ServiceCompleteList, and add a "RemoveServiceFromProfile". But not today.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:532: // And a network to the
default Profile with a specified GUID.
On 2014/05/14 08:46:07, pneubeck wrote:
> typo: And -> Add

Done.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:562:
EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
On 2014/05/14 08:46:07, pneubeck wrote:
> this EXPECT should also be moved to test 'FavoriteState' or 'GetState'.
See above.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:571: }
On 2014/05/14 08:46:07, pneubeck wrote:
> optional: at the end of this test and the next one, you could again verify
that
> the FavoriteState also has the correct GUID.
Done.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:577: // And a network without
adding it to a profile.
On 2014/05/14 08:46:07, pneubeck wrote:
> typo: And -> Add

Done.

https://codereview.chromium.org/275543005/diff/150001/chromeos/network/networ...
chromeos/network/network_state_handler_unittest.cc:598:
EXPECT_FALSE(network_state_handler_->GetNetworkState(wifi_path));
On 2014/05/14 08:46:07, pneubeck wrote:
> remove
It may be redundant, but I think it makes the test more clear.

[stevenjb, 2014-05-14 23:23:29]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-14 23:24:06]

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

[commit-bot: I haz the power, 2014-05-15 03:00:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-15 03:05:42]

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

[commit-bot: I haz the power, 2014-05-15 03:05:42]

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

[pneubeck (no reviews), 2014-05-15 07:31:55]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-05-15 07:32:06]

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

[commit-bot: I haz the power, 2014-05-15 11:16:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-15 12:21:42]

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

[commit-bot: I haz the power, 2014-05-15 12:21:43]

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

[stevenjb, 2014-05-15 19:04:30]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-15 19:05:57]

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

[commit-bot: I haz the power, 2014-05-15 22:13:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  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-05-15 22:17:27]

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

[commit-bot: I haz the power, 2014-05-15 22:17:27]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[stevenjb, 2014-05-16 01:28:00]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-16 01:28:14]

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

[commit-bot: I haz the power, 2014-05-16 05:33:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  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-05-16 07:29:42]

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

[commit-bot: I haz the power, 2014-05-16 07:29:42]

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

[stevenjb, 2014-05-16 20:26:44]

+mef@ for FakeWifiService

[stevenjb, 2014-05-16 21:48:51]

Committed patchset #15 manually as r271106 (presubmit successful).