Implement networkingPrivate.getDeviceStates

Implement networkingPrivate.getDeviceStates

This is needed for the internet Settings UI to replace the following
properites passed using chrome.send(). It will also be used in the new
Settings UI to improve the UI for uninitalized and enabling
technologies.

cellularAvailable
cellularEnabled
wifiAvailable
wifiEnabled
wimaxAvailable
wimaxEnabled

BUG=465418
For chrome_browser_main_extra_parts_profiles.cc:
TBR=skuhne@chromium.org

Committed: https://crrev.com/7d6a04929fbc69cd9287cc19a365789a42e0e12b
Cr-Commit-Position: refs/heads/master@{#324643}

[stevenjb, 2015-04-02 21:19:09]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, benchan@chromium.org, pneubeck@chromium.org

[stevenjb, 2015-04-06 21:36:57]

friendly ping

[pneubeck (no reviews), 2015-04-08 10:05:07]

The implementation itself looks good to me with a few nits.

Regarding the nomenclature I'm a bit worried:
Looking at NSH and the DeviceHandler, we use the term 'DeviceState' for the
state of e.g. a specific WiFi device of which you can have multiple.

How do we expect to expose information per WiFi (or in general network) device
which is different from 'per device type' information?

https://codereview.chromium.org/1059033002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/1059033002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:153:
(*result)[ ::onc::network_config::kEthernet] =
nit: remove space after [

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:90: //
No entry for unavaliable network types.
typo: unavaliable -> unavailable

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:368:
InsertDeviceState(::onc::network_type::kEthernet, device_list.get());
for consideration:

would it make sense to expose the list of available (or all known) network
technologies by NSH instead of explicitly listing the technologies to add?

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:41:
using DeviceStateMap = std::map<std::string, std::string>;
please document what the keys and values are.

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (left):

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:48: All, Bluetooth, Cellular,
Ethernet, VPN, Wireless, WiFi, WiMAX
is this related?

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:329: // |callback|: Called with a
dictionary of devices and their state.
dictionary -> list

[stevenjb, 2015-04-08 16:17:50]

On 2015/04/08 10:05:07, pneubeck wrote:
> The implementation itself looks good to me with a few nits.
> 
> Regarding the nomenclature I'm a bit worried:
> Looking at NSH and the DeviceHandler, we use the term 'DeviceState' for the
> state of e.g. a specific WiFi device of which you can have multiple.
> 
> How do we expect to expose information per WiFi (or in general network) device
> which is different from 'per device type' information?
> 

Thanks for pointing this out. I didn't actually implement this the way I had
originally intended.

I added some comments to crbug.com/465418 to (hopefully) help explain my
thinking.

In brief: I was envisioning more than one DeviceState entry if there is more
than one device of the same type.

I will change the implementation to reflect that. (I will also add the
'Scanning' property which we will need in the future and which will make it at
least a little more clear that these entries are associated with a "Device" not
just a "Technology".

[stevenjb, 2015-04-08 20:27:30]

It occurred to me as I added the 'Scanning' property that we really need an
onDeviceStateListChanged event, so I added that as well.

PTAL

https://codereview.chromium.org/1059033002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/1059033002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:153:
(*result)[ ::onc::network_config::kEthernet] =
On 2015/04/08 10:05:07, pneubeck wrote:
> nit: remove space after [

Blame clang :P

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:90: //
No entry for unavaliable network types.
On 2015/04/08 10:05:07, pneubeck wrote:
> typo: unavaliable -> unavailable

Done.

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:368:
InsertDeviceState(::onc::network_type::kEthernet, device_list.get());
On 2015/04/08 10:05:07, pneubeck wrote:
> for consideration:
> 
> would it make sense to expose the list of available (or all known) network
> technologies by NSH instead of explicitly listing the technologies to add?

See meta comment. I'm actually going to need to change this to iterate this
across all Devices + any available technologies that do not have a device.

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:41:
using DeviceStateMap = std::map<std::string, std::string>;
On 2015/04/08 10:05:07, pneubeck wrote:
> please document what the keys and values are.

Documented with GetDeviceStates. I can rename the typedef if that helps.

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (left):

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:48: All, Bluetooth, Cellular,
Ethernet, VPN, Wireless, WiFi, WiMAX
On 2015/04/08 10:05:07, pneubeck wrote:
> is this related?

Kind of. We never supported it, for networking, and now we explicitly don't
support it for getDeviceStates, so I am removing it to eliminate any confusion.

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1059033002/diff/40001/extensions/common/api/n...
extensions/common/api/networking_private.idl:329: // |callback|: Called with a
dictionary of devices and their state.
On 2015/04/08 10:05:07, pneubeck wrote:
> dictionary -> list

Done.

[pneubeck (no reviews), 2015-04-09 09:41:22]

thanks, this addresses my point.
lgtm with minor comments.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:348:
{State: 'Enabled', Type: 'Ethernet'},
you documentation of the API states that this should include a 'Scanning'
property. Probably you want to update the documentation.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:631:
console.log('onDeviceStateListChangedEvent');
nit: remove

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:633:
var listener = function() {
instead of using callbackAdded, you can wrap the listener into callbackPass(),
as it's a onetime listener.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:636:
done();
not sure if that makes sense with the stub implementations: you could
additionally verify whether WiFi is actually disabled

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:357:
LOG(ERROR) << "Get device states.";
debug left over

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:359:
LOG(ERROR) << "No device states.";
probably a warning?

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:367:
LOG(ERROR) << "Device states: " << device_state_list->GetSize();
debug left over

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:115:
DCHECK(state != private_api::DEVICE_STATE_TYPE_NONE);
DCHECK_NE( , )

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:397:
const char* technology_types[] = {::onc::network_type::kEthernet,
NetworkTypePattern is copyable, so you should be able to avoid the
  NetworkTypePattern -> ONC -> NetworkTypePattern
roundtrip by using NetworkTypePattern instead of const char* in this function
then only translate from NetworkTypePattern -> ONC in AppendDeviceState.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:401:
for (size_t i = 0; i < arraysize(technology_types); ++i) {
for (const char* technology : technology_types) {
}

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:403: if
(technologies_found.count(technology) > 0)
from stl_util.h

ContainsValue(technologies_found, technology)

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:16:
#include "extensions/common/api/networking_private.h"
not sure why you have to change this include.
if possible, keep this include in the .cc

https://codereview.chromium.org/1059033002/diff/80001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:89: // Set if the device is
enabled. True if the device is currently scanning.
as mentioned in the test, this is not fully accurate

[stevenjb, 2015-04-09 16:20:23]

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

[stevenjb, 2015-04-09 16:20:25]

+isheramn@ for histograms

pneubeck@ - Thanks a ton for the review!

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:348:
{State: 'Enabled', Type: 'Ethernet'},
On 2015/04/09 09:41:21, pneubeck wrote:
> you documentation of the API states that this should include a 'Scanning'
> property. Probably you want to update the documentation.

Actually, this is a bug in the fake implementation; no Ethernet device was being
added. (Even though Scanning will always be false for Ethernet, it seems simpler
to just provide it). Fixed.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:631:
console.log('onDeviceStateListChangedEvent');
On 2015/04/09 09:41:21, pneubeck wrote:
> nit: remove

Done.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:633:
var listener = function() {
On 2015/04/09 09:41:21, pneubeck wrote:
> instead of using callbackAdded, you can wrap the listener into callbackPass(),
> as it's a onetime listener.

Done.

https://codereview.chromium.org/1059033002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:636:
done();
On 2015/04/09 09:41:21, pneubeck wrote:
> not sure if that makes sense with the stub implementations: you could
> additionally verify whether WiFi is actually disabled

I'd just as soon not test behavior that isn't directly tied to this test, it
adds confusion if disableNetworkType breaks in the fake implementation.

That said, we don't currently test enable/disableNetworkTye, so I added a
separate test for those.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:357:
LOG(ERROR) << "Get device states.";
On 2015/04/09 09:41:22, pneubeck wrote:
> debug left over

Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:359:
LOG(ERROR) << "No device states.";
On 2015/04/09 09:41:22, pneubeck wrote:
> probably a warning?

Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_api.cc:367:
LOG(ERROR) << "Device states: " << device_state_list->GetSize();
On 2015/04/09 09:41:22, pneubeck wrote:
> debug left over

Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:115:
DCHECK(state != private_api::DEVICE_STATE_TYPE_NONE);
On 2015/04/09 09:41:22, pneubeck wrote:
> DCHECK_NE( , )

Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:397:
const char* technology_types[] = {::onc::network_type::kEthernet,
On 2015/04/09 09:41:22, pneubeck wrote:
> NetworkTypePattern is copyable, so you should be able to avoid the
>   NetworkTypePattern -> ONC -> NetworkTypePattern
> roundtrip by using NetworkTypePattern instead of const char* in this function
> then only translate from NetworkTypePattern -> ONC in AppendDeviceState.

I don't follow. In line 390 we do shill -> ONC, here we just use ONC. In
AppendDeviceState we need both ONC and Pattern. I think this is simplest.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:401:
for (size_t i = 0; i < arraysize(technology_types); ++i) {
On 2015/04/09 09:41:22, pneubeck wrote:
> for (const char* technology : technology_types) {
> }

Huh, I didn't know that worked with arrays. Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_chromeos.cc:403: if
(technologies_found.count(technology) > 0)
On 2015/04/09 09:41:22, pneubeck wrote:
> from stl_util.h
> 
> ContainsValue(technologies_found, technology)

Done.

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
File extensions/browser/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/browser/api/...
extensions/browser/api/networking_private/networking_private_delegate.h:16:
#include "extensions/common/api/networking_private.h"
On 2015/04/09 09:41:22, pneubeck wrote:
> not sure why you have to change this include.
> if possible, keep this include in the .cc

Because we use ScopedVector<DeviceStateProperties>, DeviceStateProperties needs
to be a complete type.

https://codereview.chromium.org/1059033002/diff/80001/extensions/common/api/n...
File extensions/common/api/networking_private.idl (right):

https://codereview.chromium.org/1059033002/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:89: // Set if the device is
enabled. True if the device is currently scanning.
On 2015/04/09 09:41:22, pneubeck wrote:
> as mentioned in the test, this is not fully accurate

Acknowledged.

[stevenjb, 2015-04-09 16:43:17]

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

[stevenjb, 2015-04-09 16:43:17]

+asargent@ for neteorking_private.idl

[Ilya Sherman, 2015-04-09 19:47:30]

histograms.xml LGTM.  Lucky number 999!

[stevenjb, 2015-04-09 19:55:57]

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

[stevenjb, 2015-04-09 19:55:58]

or kalman@ for networking_private.idl

[not at google - send to devlin, 2015-04-09 19:57:48]

lgtm

[stevenjb, 2015-04-09 20:44:06]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-04-09 20:44:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org, isherman@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1059033002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-09 20:45:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059033002/140001

[commit-bot: I haz the power, 2015-04-09 21:17:54]

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

[commit-bot: I haz the power, 2015-04-09 21:17:55]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[stevenjb, 2015-04-09 21:38:05]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-04-09 21:38:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pneubeck@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1059033002/#ps160001
(title: "Fix compile errors")

[commit-bot: I haz the power, 2015-04-09 21:38:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059033002/160001

[commit-bot: I haz the power, 2015-04-09 21:51:36]

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

[commit-bot: I haz the power, 2015-04-09 21:51:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-04-09 23:07:21]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2015-04-09 23:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, pneubeck@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1059033002/#ps180001
(title: "Fix Android compile errors")

[commit-bot: I haz the power, 2015-04-09 23:08:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059033002/180001

[commit-bot: I haz the power, 2015-04-10 00:09:38]

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

[commit-bot: I haz the power, 2015-04-10 00:09:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2015-04-10 15:47:59]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2015-04-10 15:48:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1059033002/180001

[commit-bot: I haz the power, 2015-04-10 15:53:45]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-10 15:55:44]

Patchset 10 (id:??) landed as
https://crrev.com/7d6a04929fbc69cd9287cc19a365789a42e0e12b
Cr-Commit-Position: refs/heads/master@{#324643}