Implement networkingPrivate.getNetworks

Implement networkingPrivate.getNetworks

Design Doc and Proposal here:
https://docs.google.com/a/google.com/document/d/1QWIzDvf_-iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit

This CL depends on https://codereview.chromium.org/275543005/

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

[stevenjb, 2014-05-10 00:44:17]

OK, this is ready for review, please take a look when you have a chance.

[tbarzic, 2014-05-13 19:02:58]

lgtm

https://codereview.chromium.org/280023003/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:199: "description": "If
true, only include visible (physically connected or in-range) networks. Defaults
to 'false'."
I'd lose '' around false

https://codereview.chromium.org/280023003/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:229: "deprecated": "Please
Use $(ref:networkingPrivate.getNetworks).",
replace leading /t with ws

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

https://codereview.chromium.org/280023003/diff/20001/chromeos/network/network...
chromeos/network/network_util.h:93: CHROMEOS_EXPORT scoped_ptr<base::ListValue>
TranslateNetworkListToONC(
suggestion: GetNetworkListAsONC may be more suitable name

[armansito, 2014-05-13 20:31:36]

lgtm

[stevenjb, 2014-05-15 00:04:48]

Rebased with the GUID changes, Philip PTAL when you have a chance.

[armansito, 2014-05-15 00:25:03]

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

https://codereview.chromium.org/280023003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:347:
chrome.networkingPrivate.setProperties("stub_wifi2", result,
nit: Might as well pass |network_guid| here instead of "stub_wifi2".

https://codereview.chromium.org/280023003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:350: "stub_wifi2",
Same as above.

[stevenjb, 2014-05-15 01:23:34]

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

https://codereview.chromium.org/280023003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:347:
chrome.networkingPrivate.setProperties("stub_wifi2", result,
On 2014/05/15 00:25:03, armansito wrote:
> nit: Might as well pass |network_guid| here instead of "stub_wifi2".

Done.

https://codereview.chromium.org/280023003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:350: "stub_wifi2",
On 2014/05/15 00:25:03, armansito wrote:
> Same as above.

Done.

[pneubeck (no reviews), 2014-05-15 18:28:37]

Awesome! Looks very good.

IMO, commit message should at least contain a short rational.

The difference of reported state properties for visible&not-visible networks is
currently not documented. We should decide what the roadmap for this case is:
1) The difference is intended -> then it should be documented in the API
2) The difference is a current flaw -> will be fixed soon, so that not-visible
networks report as much state as makes sense (e.g. SecurityType,
SignalStrength=0, ...) / needs TODO or bug

I'd strongly suggest doing 2).
Which I think will also make the concept of FavoriteState & NetworkState
clearer: FavoriteState is _the_ primary state object. NetworkState only provides
a few additional properties and could maybe even be removed at some time.

(Of course this can be punted to another CL)

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

https://codereview.chromium.org/280023003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:305:
type);
this assumes that the filter.type is a valid ONC network type (or "All"). Could
be clarified in the API documentation.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:185: "description":
"Returns a list of network objects with the same properties provided by
getState.",
could mention the filter argument

since you allow limiting the number of networks, it might be helpful to mention
the ordering.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:193: "enum": ["All",
"Bluetooth", "Cellular", "Ethernet", "VPN", "Wireless", "WiFi", "Wimax"],
could you move this to the toplevel , and maybe mention that these are types
from ONC?

There is already a toplevel 'NetworkType' used by enabledNetworkType. You could
define another 'NetworkPatternType' consisting of the enum ['All', 'Wireless']
and then use the union/choice of NetworkType and NetworkPatternType for the
filter.type.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:199: "description": "If
true, only include visible (physically connected or in-range) networks. Defaults
to 'false'."
physically connected or in-range

this doesn't explain what happens for VPN :-/ ; maybe add a 'or connectable'.

Potentially, VPN could be left out if there is no host network to route the VPN
traffic. So, maybe it's fine to leave this unspecified so far.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:209: "description":
"Maximum number of networks to return. Default: 1000"
s/Default: /Defaults to /

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:229: "deprecated": "Please
Use $(ref:networkingPrivate.getNetworks).",
Use -> use
or remove 'Please'

could mention what call to getNetworks is equivalent: 'with filter.visible =
true'

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

https://codereview.chromium.org/280023003/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:125: // Test 'type' and
'configured'.
testing 'visible: true' too?

https://codereview.chromium.org/280023003/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:131: "ConnectionState":
"Connected",
if such network states are common over several tests, we could move them to
global variables:
 var initialStateWiFi1 = ...
  ...

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:510: &active_services,
&inactive_services);
nit: format

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:547: return;
nit: remove

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.h:108: void SortServiceList(const
std::string& property);
optional nit: add a comment; also that |property| should either be
shill::kServicesProperty or shill::kServiceCompleteListProperty

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state.cc:133: return false;
the return value is ignored by the caller.
in particular this seems not to lead to ignoring this state in the future
(set_update_received() is called unconditionally).

the best that might be feasible currently is to remove this state?

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state.cc:138: // Set the ca_cert_nss_ property.
ca_cert_nss_ -> has_ca_cert_nss_

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:315: for
(ManagedStateList::const_iterator iter = network_list_.begin();
I like this solution if we don't care about best runtime.
Just wanted to mention an alternative:
We could set a pointer to the NetworkState (or only a boolean 'visible') in the
favorite state that we would have to update on creation/destruction of network
states.
Preparing a set wouldn't be required anymore and lookup would be very fast.
Space overhead would be one pointer (or boolean) per FavoriteState only.

This would also make it more clear / ensure that NetworkStates only exist if
there is already a FavoriteState.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.h:187: // and the following properties:
remove one 'and'

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:143:
network_state->GetStateProperties(&shill_network_dictionary);
optional nit: GetStateProperties could merge only omit the initial clear().

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:162: int count = 0;
optional nit: limit could also be forwarded to GetFavoriteListByType (down to
where the filtering happens)

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:171: if (++count >= limit)
I'd find it more logical if the condition would be at the beginning, which would
also work for limit=0 ;-D

(the loop condition is also evaluated at the beginning of each iteration)

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:355: <dt class="field">ConnectionState</dt>
didn't I review this already at some time?

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:378: connectable.
I think, 'connectable' makes only sense to us who know Shill's definition of it.
Maybe something like 'if the system indicates that the network is fully
configured, i.e. no additional configuration and user interaction is required to
connect.' would be clearer?

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
File components/onc/onc_constants.h (right):

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
components/onc/onc_constants.h:87: ONC_EXPORT extern const char kWimax[];
please group these two with kAllTypes and comment them as 'Virtual types; not
part of the ONC spec.'

[stevenjb, 2014-05-15 20:12:41]

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

https://codereview.chromium.org/280023003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:305:
type);
On 2014/05/15 18:28:37, pneubeck wrote:
> this assumes that the filter.type is a valid ONC network type (or "All").
Could
> be clarified in the API documentation.

From the .json file:

              "type": {
		"type": "string",
		"enum": ["All", "Bluetooth", "Cellular", "Ethernet", "VPN", "Wireless",
"WiFi", "Wimax"],
		"description": "The type of networks to return."
              },

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:185: "description":
"Returns a list of network objects with the same properties provided by
getState.",
On 2014/05/15 18:28:37, pneubeck wrote:
> could mention the filter argument
> 
> since you allow limiting the number of networks, it might be helpful to
mention
> the ordering.

Ah, sure, I'll mention the filter at the top level, and the ordering.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:193: "enum": ["All",
"Bluetooth", "Cellular", "Ethernet", "VPN", "Wireless", "WiFi", "Wimax"],
On 2014/05/15 18:28:37, pneubeck wrote:
> could you move this to the toplevel , and maybe mention that these are types
> from ONC?
> 
> There is already a toplevel 'NetworkType' used by enabledNetworkType. You
could
> define another 'NetworkPatternType' consisting of the enum ['All', 'Wireless']
> and then use the union/choice of NetworkType and NetworkPatternType for the
> filter.type.

Ah, so there is, even better. I will unify these, I don't think we should need
to distinguish between 'NetworkType' and 'NetworkPatternType' at the API level.
Enable/disable technology can already support patterns in NetworkStateHandler, I
will fix Enable/DisableNetworkType to support patterns also. I will also add a
comment to getEnabledNetworkTypes which is the only place where non-discrete
types don't make sense.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:199: "description": "If
true, only include visible (physically connected or in-range) networks. Defaults
to 'false'."
On 2014/05/15 18:28:37, pneubeck wrote:
> physically connected or in-range
> 
> this doesn't explain what happens for VPN :-/ ; maybe add a 'or connectable'.
> 
> Potentially, VPN could be left out if there is no host network to route the
VPN
> traffic. So, maybe it's fine to leave this unspecified so far.

I'm not entirely certain what the VPN behavior is. I think I'll leave it
undocumented until/unless it becomes an issue (we may need to add a NonVirtual
type to NetworkType).

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:209: "description":
"Maximum number of networks to return. Default: 1000"
On 2014/05/15 18:28:37, pneubeck wrote:
> s/Default: /Defaults to /

Done.

https://codereview.chromium.org/280023003/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/networking_private.json:229: "deprecated": "Please
Use $(ref:networkingPrivate.getNetworks).",
On 2014/05/15 18:28:37, pneubeck wrote:
> Use -> use
> or remove 'Please'
> 
> could mention what call to getNetworks is equivalent: 'with filter.visible =
> true'

Done.

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

https://codereview.chromium.org/280023003/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:125: // Test 'type' and
'configured'.
On 2014/05/15 18:28:37, pneubeck wrote:
> testing 'visible: true' too?

Hmm, we're not actually testing that and should, I will add that test.

https://codereview.chromium.org/280023003/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking/test.js:131: "ConnectionState":
"Connected",
On 2014/05/15 18:28:37, pneubeck wrote:
> if such network states are common over several tests, we could move them to
> global variables:
>  var initialStateWiFi1 = ...
>   ...

If I did that I'd want to move the entire default states to globals. TODO.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:510: &active_services,
&inactive_services);
On 2014/05/15 18:28:37, pneubeck wrote:
> nit: format

Done.

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:547: return;
On 2014/05/15 18:28:37, pneubeck wrote:
> nit: remove

Done.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.h:108: void SortServiceList(const
std::string& property);
On 2014/05/15 18:28:37, pneubeck wrote:
> optional nit: add a comment; also that |property| should either be
> shill::kServicesProperty or shill::kServiceCompleteListProperty

Done.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state.cc:133: return false;
On 2014/05/15 18:28:37, pneubeck wrote:
> the return value is ignored by the caller.
> in particular this seems not to lead to ignoring this state in the future
> (set_update_received() is called unconditionally).
> 
> the best that might be feasible currently is to remove this state?

It is used in one of the places it gets called from, but not in the way that
might be expected (but it is documented). It returns whether or not a property
is changed.

On the one hand this shouldn't ever happen, but if it did I can't think off the
top of my head of anything that would break catastrophically, and I would just
as soon have the broken state available (in logs, etc) for debugging if it ever
did happen.

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state.cc:138: // Set the ca_cert_nss_ property.
On 2014/05/15 18:28:38, pneubeck wrote:
> ca_cert_nss_ -> has_ca_cert_nss_

Done.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:315: for
(ManagedStateList::const_iterator iter = network_list_.begin();
On 2014/05/15 18:28:38, pneubeck wrote:
> I like this solution if we don't care about best runtime.
> Just wanted to mention an alternative:
> We could set a pointer to the NetworkState (or only a boolean 'visible') in
the
> favorite state that we would have to update on creation/destruction of network
> states.
> Preparing a set wouldn't be required anymore and lookup would be very fast.
> Space overhead would be one pointer (or boolean) per FavoriteState only.
> 
> This would also make it more clear / ensure that NetworkStates only exist if
> there is already a FavoriteState.

I thought about that, but honestly if performance becomes a concern we would get
a bigger win from maintaining a map of NetworkState and FavoriteState by path
and a map of guid -> path. 

Alternately/additionally we could probably combine NetworkState and
FavoriteState at this point, but... definitely a task for another day.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.h:187: // and the following properties:
On 2014/05/15 18:28:38, pneubeck wrote:
> remove one 'and'

Done.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:143:
network_state->GetStateProperties(&shill_network_dictionary);
On 2014/05/15 18:28:38, pneubeck wrote:
> optional nit: GetStateProperties could merge only omit the initial clear().

While that is probably safe currently, and I could document the behavior, I
still worry that it would potentially lead to a re-used local unintentionally
combining states. (I'd want to rename it MergeStateProperties at that point,
similar to DictionaryValue).

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:162: int count = 0;
On 2014/05/15 18:28:38, pneubeck wrote:
> optional nit: limit could also be forwarded to GetFavoriteListByType (down to
> where the filtering happens)

Moved limit to GetFavoriteListByType (and changed + documented that 0 == no
limit)

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_util.cc:171: if (++count >= limit)
On 2014/05/15 18:28:38, pneubeck wrote:
> I'd find it more logical if the condition would be at the beginning, which
would
> also work for limit=0 ;-D
> 
> (the loop condition is also evaluated at the beginning of each iteration)
meh

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:355: <dt class="field">ConnectionState</dt>
On 2014/05/15 18:28:38, pneubeck wrote:
> didn't I review this already at some time?

Yes, it was in an early chrome://network re-factor CL that has since been
re-re-factored on top of this one. This change makes more sense here I think.

https://codereview.chromium.org/280023003/diff/80001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:378: connectable.
On 2014/05/15 18:28:38, pneubeck wrote:
> I think, 'connectable' makes only sense to us who know Shill's definition of
it.
> Maybe something like 'if the system indicates that the network is fully
> configured, i.e. no additional configuration and user interaction is required
to
> connect.' would be clearer?

Done.

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
File components/onc/onc_constants.h (right):

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
components/onc/onc_constants.h:87: ONC_EXPORT extern const char kWimax[];
On 2014/05/15 18:28:38, pneubeck wrote:
> please group these two with kAllTypes and comment them as 'Virtual types; not
> part of the ONC spec.'
Wimax should be part of the spec, shouldn't it? At the Shill layer it is, for
all intensive purposes, a discrete type.

Moved All above Wireless and commented.

[pneubeck (no reviews), 2014-05-16 13:18:54]

lgtm

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:510: &active_services,
&inactive_services);
On 2014/05/15 20:12:41, stevenjb wrote:
> On 2014/05/15 18:28:37, pneubeck wrote:
> > nit: format
> 
> Done.

not done? (I meant the indentation of the second argument line)

https://codereview.chromium.org/280023003/diff/80001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:547: return;
On 2014/05/15 20:12:41, stevenjb wrote:
> On 2014/05/15 18:28:37, pneubeck wrote:
> > nit: remove
> 
> Done.

Not done?

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state.cc:133: return false;
On 2014/05/15 20:12:41, stevenjb wrote:
> On 2014/05/15 18:28:37, pneubeck wrote:
> > the return value is ignored by the caller.
> > in particular this seems not to lead to ignoring this state in the future
> > (set_update_received() is called unconditionally).
> > 
> > the best that might be feasible currently is to remove this state?
> 
> It is used in one of the places it gets called from, but not in the way that
> might be expected (but it is documented). It returns whether or not a property
> is changed.
Ops. My search was not exhaustive, sorry.

> On the one hand this shouldn't ever happen, but if it did I can't think off
the
> top of my head of anything that would break catastrophically, and I would just
> as soon have the broken state available (in logs, etc) for debugging if it
ever
> did happen.

Yeah, let's wait for the error to appear.

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

https://codereview.chromium.org/280023003/diff/80001/chromeos/network/network...
chromeos/network/network_state_handler.cc:315: for
(ManagedStateList::const_iterator iter = network_list_.begin();
On 2014/05/15 20:12:41, stevenjb wrote:
> On 2014/05/15 18:28:38, pneubeck wrote:
> > I like this solution if we don't care about best runtime.
> > Just wanted to mention an alternative:
> > We could set a pointer to the NetworkState (or only a boolean 'visible') in
> the
> > favorite state that we would have to update on creation/destruction of
network
> > states.
> > Preparing a set wouldn't be required anymore and lookup would be very fast.
> > Space overhead would be one pointer (or boolean) per FavoriteState only.
> > 
> > This would also make it more clear / ensure that NetworkStates only exist
if
> > there is already a FavoriteState.
> 
> I thought about that, but honestly if performance becomes a concern we would
get
> a bigger win from maintaining a map of NetworkState and FavoriteState by path
> and a map of guid -> path. 
> 
> Alternately/additionally we could probably combine NetworkState and
> FavoriteState at this point, but... definitely a task for another day.
> 

Yes, totally reasonable.

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
File components/onc/onc_constants.h (right):

https://codereview.chromium.org/280023003/diff/80001/components/onc/onc_const...
components/onc/onc_constants.h:87: ONC_EXPORT extern const char kWimax[];
On 2014/05/15 20:12:41, stevenjb wrote:
> On 2014/05/15 18:28:38, pneubeck wrote:
> > please group these two with kAllTypes and comment them as 'Virtual types;
not
> > part of the ONC spec.'
> Wimax should be part of the spec, shouldn't it? At the Shill layer it is, for
> all intensive purposes, a discrete type.
Not yet, but this is fine.

> 
> Moved All above Wireless and commented.

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:192: "$ref": "NetworkType",
replace tabs (\t) by spaces (everywhere in this file)

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:228: "deprecated": "Please
use $(ref:networkingPrivate.getNetworks) with filter.visble = true",
nit: '.' at the end.

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:270: "description": "Enable
the specified network type. Note, the type might represent multiple",
nit: '.' at the end.

https://codereview.chromium.org/280023003/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/280023003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:166: { "networkType":
"All", "configured": true, "limit": 1 },
you should also have a test for 'configured:false'.
Maybe you can make ethernet unconfigured and change this test to
'configured:false' ?

Then you have all "interesting" values of the individual filter options covered.
(sorry that I didn't mention it on the first pass)

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

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:462:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
I think the reason for this notification missing was that Shill doesn't send
notifications for it.

https://code.google.com/p/chromium/codesearch#chromium-os/src/platform/shill/...

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:478:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
ditto

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

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/shill_man...
chromeos/dbus/shill_manager_client.h:68: virtual void RemoveManagerService(const
std::string& service_path,
comment above doesn't apply anymore.

[stevenjb, 2014-05-16 17:35:23]

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:192: "$ref": "NetworkType",
On 2014/05/16 13:18:55, pneubeck wrote:
> replace tabs (\t) by spaces (everywhere in this file)

Done.

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:228: "deprecated": "Please
use $(ref:networkingPrivate.getNetworks) with filter.visble = true",
On 2014/05/16 13:18:55, pneubeck wrote:
> nit: '.' at the end.

Done.

https://codereview.chromium.org/280023003/diff/100001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:270: "description": "Enable
the specified network type. Note, the type might represent multiple",
On 2014/05/16 13:18:55, pneubeck wrote:
> nit: '.' at the end.

Done.

https://codereview.chromium.org/280023003/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking/test.js (right):

https://codereview.chromium.org/280023003/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking/test.js:166: { "networkType":
"All", "configured": true, "limit": 1 },
On 2014/05/16 13:18:55, pneubeck wrote:
> you should also have a test for 'configured:false'.
> Maybe you can make ethernet unconfigured and change this test to
> 'configured:false' ?
> 
> Then you have all "interesting" values of the individual filter options
covered.
> (sorry that I didn't mention it on the first pass)

So, configured='false' is not the same as !configured, it means 'don't filter by
configured'. I removed 'configured' from this test for now, which doesn't affect
the result.

We really need a better way to add and maintain these tests. It's on my "TODO"
list for 37.

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

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:462:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
On 2014/05/16 13:18:55, pneubeck wrote:
> I think the reason for this notification missing was that Shill doesn't send
> notifications for it.
> 
>
https://code.google.com/p/chromium/codesearch#chromium-os/src/platform/shill/...

Ah, you are correct, I'll remove these. Done.

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:478:
CallNotifyObserversPropertyChanged(shill::kServiceCompleteListProperty);
On 2014/05/16 13:18:55, pneubeck wrote:
> ditto

Done.

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

https://codereview.chromium.org/280023003/diff/100001/chromeos/dbus/shill_man...
chromeos/dbus/shill_manager_client.h:68: virtual void RemoveManagerService(const
std::string& service_path,
On 2014/05/16 13:18:55, pneubeck wrote:
> comment above doesn't apply anymore.

Done.

[stevenjb, 2014-05-16 17:38:15]

+jar@ for metrics, +asargent@ for extensions/

[asargent_no_longer_on_chrome, 2014-05-16 19:32:57]

lgtm with a few optional suggestions for you to consider (could be done as
follow-up CLs too)

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:116: "description": "Gets
the cached read-only properties of the network with id networkGuid. This is
meant to be a higher performance function than getProperties, which requires a
round trip to query the networking subsystem.  The following properties are
returned for all networks: GUID, Type, Name, WiFi.Security. Additional
properties are provided for visible networks: ConnectionState, ErrorState,
WiFi.SignalStrength, Cellular.NetworkTechnology, Cellular.ActivationState,
Cellular.RoamingState, Cellular.OutOfCredits.",
Instead of having one big bag o' properties that the schema validation system
doesn't know about with some degree of scoping provded by dot-seprated string
prefix on property names, had you considered using actual objects with named
fields? E.g. instead of just:

------------------------------------------
{
  "id": "NetworkProperties",
  "type": "object",
  "additionalProperties": { "type": "any" }
},
{
  "id": "NetworkType",
  "type": "string",
  "enum": ["Ethernet", "WiFi", "Cellular"]
},
------------------------------------------

you'd have:

------------------------------------------
{
  "id": "NetworkType",
  "type": "string",
  "enum": ["Ethernet", "WiFi", "Cellular"]
},

{
  "id": "WifiProperties",
  "type": "object",
  "properties": {
    "Security": { "type": "string", "enum": ["None", "WPA2"] },
    "SignalStrength": { "type": "integer" }
    ... etc. ...
  }
}

{
  "id": "CellularProperties",
  "type": "object",
  "properties": {
    "NetworkTechnology": { "type": "string", "enum": [ <whatever values> ]  },
    "ActivationState": { "type": "string", "enum": [ <whatever values> ] }
    ... etc. ...
  }
}

{
  "id": "NetworkProperties",
  "type": "object",
  "properties": {
    "Type" : {
      "$ref": "NetworkType"
    },
    "Wifi": {
      "$ref": "WifiProperties",
      "optional": true,
      "description": "Wifi properties, set if Type is 'Wifi'"
    },
    "Cellular": {
      "$ref": "CellularProperties",
      "optional": true,
      "description": "Cellular properties, set if Type is 'Cellular'"
    }
  }
  "additionalProperties": { "type": "any" }
},
------------------------------------------


Then when dealing with this in the callback, instead of doing:


chrome.networkingPrivate.getState(<id>, function(props) {
  if (props.Type == "Wifi") {
    console.log(props["Wifi.Security"]);
  }
});

you could do:

chrome.networkingPrivate.getState(<id>, function(props) {
  if (props.Type == "Wifi") {
    console.log(props.Wifi.Security);
  }
});

The big advantage though is actually that then you get schema validation of the
results.

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:193: "description": "The
type of networks to return."
Instead of making callers pass "All" here, you could just skip having the "All"
enum value altogether and instead let this be an optional property, and the
absence of it implies all.

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:270: "description": "Enable
the specified network type. Note, the type might represent multiple.",
nit: did you mean "multiple types"? "multiple networks"?

[stevenjb, 2014-05-16 19:44:44]

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:116: "description": "Gets
the cached read-only properties of the network with id networkGuid. This is
meant to be a higher performance function than getProperties, which requires a
round trip to query the networking subsystem.  The following properties are
returned for all networks: GUID, Type, Name, WiFi.Security. Additional
properties are provided for visible networks: ConnectionState, ErrorState,
WiFi.SignalStrength, Cellular.NetworkTechnology, Cellular.ActivationState,
Cellular.RoamingState, Cellular.OutOfCredits.",
The result is an ONC (Open Network Configuration) object, and we're changing it
pretty rapidly at this point, but there would definitely be value to validating
the results. I'll discuss this with Philip and consider doing this (in a
separate CL).

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:193: "description": "The
type of networks to return."
On 2014/05/16 19:32:58, Antony Sargent wrote:
> Instead of making callers pass "All" here, you could just skip having the
"All"
> enum value altogether and instead let this be an optional property, and the
> absence of it implies all. 

I think I prefer requiring an explicit type, but could be convinced otherwise.
Philip, thoughts?

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:270: "description": "Enable
the specified network type. Note, the type might represent multiple.",
On 2014/05/16 19:32:58, Antony Sargent wrote:
> nit: did you mean "multiple types"? "multiple networks"? 
I meant 'multiple network types', thanks! Done.

[stevenjb, 2014-05-16 22:48:12]

+ isherman@chromium.org, mpearson@chromium.org, asvitkine@chromium.org
Can I get a quick OWNERS review on the histograms changes? Thanks!

[Ilya Sherman, 2014-05-16 22:58:23]

Histogram changes lgtm

[stevenjb, 2014-05-16 23:30:03]

The CQ bit was checked by stevenjb@chromium.org

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

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

[commit-bot: I haz the power, 2014-05-17 06:39:35]

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_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-17 07:42:46]

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

[commit-bot: I haz the power, 2014-05-17 07:42:47]

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

[pneubeck (no reviews), 2014-05-18 18:36:54]

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/280023003/diff/140001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:193: "description": "The
type of networks to return."
On 2014/05/16 19:44:44, stevenjb wrote:
> On 2014/05/16 19:32:58, Antony Sargent wrote:
> > Instead of making callers pass "All" here, you could just skip having the
> "All"
> > enum value altogether and instead let this be an optional property, and the
> > absence of it implies all. 
> 
> I think I prefer requiring an explicit type, but could be convinced otherwise.
> Philip, thoughts?

Similar to the other filter options, I think it's logical that omitting
NetworkType means 'any network type'.
Anyways, I'm fine with both.

[stevenjb, 2014-05-19 18:06:15]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-05-19 18:07:37]

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

[commit-bot: I haz the power, 2014-05-19 21:36:05]

Change committed as 271490