Improve Cellular support in networkingPrivate

Improve Cellular support in networkingPrivate

This CL
* Adds support for setting Cellular Device properties (Carrier) in
  networkingPrivate.setProperties
* Adds SupportNetworkScan property to networkingPrivate.getState

BUG=464036

[stevenjb, 2015-03-05 02:22:09]

stevenjb@chromium.org changed reviewers:
+ pneubeck@chromium.org

[stevenjb, 2015-03-05 02:22:09]

When I started this CL, I assumed that there would be other Device properties
that we would want to set this way, e.g. AllowRoaming, but it turns out that
property is blacklisted.

There are a few other Device properties that we may want to set in the future,
e.g. ScanInterval and Wake*, so I am inclined to commit this (since I already
did the work), but I would also understand if you think we should do something
simpler just for Carrier (which actually requires calling Device.SetCarrier
instead of Device.SetProperties, but I don't think that is a detail we should
expose in the networkingPrivate API.

Thoughts?

[stevenjb, 2015-03-20 17:46:47]

Ping

[pneubeck (no reviews), 2015-03-24 16:19:52]

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:217:
profile_test_->AddService(ShillProfileClient::GetSharedProfilePath(),
do you know where cellular services are usually stored in Shill? shared or
private profile?

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:218:
"stub_cellular1");
-> kCellular1ServicePath

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js
(right):

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js:467:
var done = chrome.test.callbackAdded();
not sure why we ended up using this done/callbackAdded construct in so many
tests, but it shouldn't be required here since you use callbackPass to wrap
every callback.

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js:557:
"SupportNetworkScan": false,
this dictionary seems rather huge. Are all of these properties relevant for
(overview) network lists in UI?

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

https://codereview.chromium.org/978923003/diff/60001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:293: // Extract
any Device properties to apply separately, before validation.
any reason to do this before validation?
So far, all details of validation were separated out into the validator.
I'd like to avoid any adhoc translatin/validation code accumulating in this
class (which is already too large).

https://codereview.chromium.org/978923003/diff/60001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:347: void
ManagedNetworkConfigurationHandlerImpl::SetShillDeviceProperties(
this should really, really move to the NetworkDeviceHandler

ideally we could avoid this complexity by also having a SetDeviceProperties
exposed by Shill (at least that should be filed as a bug). This must be covered
by a test.

[stevenjb, 2015-03-25 20:50:10]

I am actually going to abandon this CL in favor of a simper one. Some of the
comments are applied to the next CL.

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:217:
profile_test_->AddService(ShillProfileClient::GetSharedProfilePath(),
On 2015/03/24 16:19:52, pneubeck wrote:
> do you know where cellular services are usually stored in Shill? shared or
> private profile?

Private, but I'm not sure we even have a private profile in the fake
implementation. We don't use it here anyway.

https://codereview.chromium.org/978923003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:218:
"stub_cellular1");
On 2015/03/24 16:19:52, pneubeck wrote:
> -> kCellular1ServicePath

Done.

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js
(right):

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js:467:
var done = chrome.test.callbackAdded();
On 2015/03/24 16:19:52, pneubeck wrote:
> not sure why we ended up using this done/callbackAdded construct in so many
> tests, but it shouldn't be required here since you use callbackPass to wrap
> every callback.

Done.

https://codereview.chromium.org/978923003/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/networking_private/chromeos/networking_private_apitest_chromeos.js:557:
"SupportNetworkScan": false,
On 2015/03/24 16:19:52, pneubeck wrote:
> this dictionary seems rather huge. Are all of these properties relevant for
> (overview) network lists in UI?
All except SupportNetworkScan are used in the badge. I'm not sure why ONC is
providing that; we don't provide it in NetworkState::GetStateProperties as near
as I can tell. Maybe because it is a boolean?