[CrOS Tether] Add tether network properties (battery percentage, carrier, and signal strength) to t…

[CrOS Tether] Add tether network properties (battery percentage, carrier, and signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Update the settings page's chrome.networkingPrivate.getNetworks() call site to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2818593003
Cr-Commit-Position: refs/heads/master@{#465749}
Committed: https://chromium.googlesource.com/chromium/src/+/e8e89d55e8f02315056a6c524f8fff83fbe1b289

[Kyle Horimoto, 2017-04-13 01:29:17]

Description was changed from

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Add properties to the NetworkState class as instance variables, and update
GetStateProperties() to return these properties when appropriate.
(7) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
==========

to

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Add properties to the NetworkState class as instance variables, and update
GetStateProperties() to return these properties when appropriate.
(7) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Kyle Horimoto, 2017-04-13 20:42:20]

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

[stevenjb, 2017-04-14 21:54:54]

This is kind of huge.

Could you separate out the IDL / ONC changes from the NetworkState changes?

Thanks.

[stevenjb, 2017-04-14 22:04:49]

https://codereview.chromium.org/2818593003/diff/60001/chromeos/network/tether...
File chromeos/network/tether_constants.h (right):

https://codereview.chromium.org/2818593003/diff/60001/chromeos/network/tether...
chromeos/network/tether_constants.h:15: // code, and these values are used
primarily to implement the
The real reason we uses these is that NetworkStateHandler deals with Shill
dictionaries, and the tether component interacts with NetworkStateHandler since
that is the class that maintains network state for all of Chrome OS.

Thinking about this though, there is no particular reason why NetworkState can't
just use the ONC constants. That would make it a little more clear that Tether
networks are not part of Shill.

If we do that, it may still make sense to provide a translation table in the ONC
code, just with a comment that the properties are coming from NSH and still need
to be validated.

(Sorry this did not occur to me sooner).

LMKWYT

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

https://codereview.chromium.org/2818593003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:579: DOMString? Carrier;
Isn't Carrier required? (no '?' for required properties)

https://codereview.chromium.org/2818593003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:584: long? BatteryPercentage;
ManagedLong, etc.

https://codereview.chromium.org/2818593003/diff/60001/third_party/closure_com...
File third_party/closure_compiler/externs/networking_private.js (right):

https://codereview.chromium.org/2818593003/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/networking_private.js:1227: * @deprecated
Use networking.castPrivate API.
Can you put up a CL with just these changes and have me and have tbarzic@ and
myself review this? Or ask tbarzic@ to do so?

[Kyle Horimoto, 2017-04-17 21:04:04]

Description was changed from

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Add properties to the NetworkState class as instance variables, and update
GetStateProperties() to return these properties when appropriate.
(7) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Kyle Horimoto, 2017-04-17 21:05:11]

Removed NetworkState and NetworkStateHandler changes from this CL as you
requested. Will add them to another CL instead.

https://codereview.chromium.org/2818593003/diff/60001/chromeos/network/tether...
File chromeos/network/tether_constants.h (right):

https://codereview.chromium.org/2818593003/diff/60001/chromeos/network/tether...
chromeos/network/tether_constants.h:15: // code, and these values are used
primarily to implement the
On 2017/04/14 22:04:49, stevenjb wrote:
> The real reason we uses these is that NetworkStateHandler deals with Shill
> dictionaries, and the tether component interacts with NetworkStateHandler
since
> that is the class that maintains network state for all of Chrome OS.
> 
> Thinking about this though, there is no particular reason why NetworkState
can't
> just use the ONC constants. That would make it a little more clear that Tether
> networks are not part of Shill.
> 
> If we do that, it may still make sense to provide a translation table in the
ONC
> code, just with a comment that the properties are coming from NSH and still
need
> to be validated.
> 
> (Sorry this did not occur to me sooner).
> 
> LMKWYT

While this solution could also work, Ryan and I agreed that it is easier to
understand by using fill-in Shill types. Using this strategy treats all
Shill/ONC conversions the same way, and it also uses the same style of
constants. Shill constants all use the same property naming scheme:
"<NetworkType>.<PropertyName>", so we think it makes more sense to treat tether
networks the same way. This also is consistent with the JSON-ified Shill
networks in 	chromeos/test/data/network.

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

https://codereview.chromium.org/2818593003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:579: DOMString? Carrier;
On 2017/04/14 22:04:49, stevenjb wrote:
> Isn't Carrier required? (no '?' for required properties)

Done.

https://codereview.chromium.org/2818593003/diff/60001/extensions/common/api/n...
extensions/common/api/networking_private.idl:584: long? BatteryPercentage;
On 2017/04/14 22:04:49, stevenjb wrote:
> ManagedLong, etc.

Done.

https://codereview.chromium.org/2818593003/diff/60001/third_party/closure_com...
File third_party/closure_compiler/externs/networking_private.js (right):

https://codereview.chromium.org/2818593003/diff/60001/third_party/closure_com...
third_party/closure_compiler/externs/networking_private.js:1227: * @deprecated
Use networking.castPrivate API.
On 2017/04/14 22:04:49, stevenjb wrote:
> Can you put up a CL with just these changes and have me and have tbarzic@ and
> myself review this? Or ask tbarzic@ to do so?

Removed it as part of this CL; will send another CL later.

[stevenjb, 2017-04-18 20:41:19]

https://codereview.chromium.org/2818593003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2818593003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:285: Tether:
[],
We shouldn't make this change by itself.

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/networ...
File chromeos/network/network_type_pattern.cc (right):

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/networ...
chromeos/network/network_type_pattern.cc:67: kNetworkTypeCellular |
kNetworkTypeTether);
I'd rather not add this here yet. We need to audit the code that uses Wireless()
and ensure that it handles Tether networks properly before we do this. In the
meanwhile we can do this explicitly when we need to.

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/tether...
File chromeos/network/tether_constants.h (right):

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/tether...
chromeos/network/tether_constants.h:16: // chrome.networkingPrivate API.
Strictly speaking these have nothing to do with chrome.networkingPrivate which
uses ONC. These constants are just for integration with NetworkStateHandler
which primarily deals with Shill services.

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

https://codereview.chromium.org/2818593003/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:578: long BatteryPercentage;
This is optional, right? Same with SignalStrength? This should be 'long?'

https://codereview.chromium.org/2818593003/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:584: ManagedLong BatteryPercentage;
ditto

[Kyle Horimoto, 2017-04-18 21:58:39]

https://codereview.chromium.org/2818593003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/internet_page/network_summary.js (right):

https://codereview.chromium.org/2818593003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/internet_page/network_summary.js:285: Tether:
[],
On 2017/04/18 20:41:19, stevenjb wrote:
> We shouldn't make this change by itself.

Done.

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/networ...
File chromeos/network/network_type_pattern.cc (right):

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/networ...
chromeos/network/network_type_pattern.cc:67: kNetworkTypeCellular |
kNetworkTypeTether);
On 2017/04/18 20:41:19, stevenjb wrote:
> I'd rather not add this here yet. We need to audit the code that uses
Wireless()
> and ensure that it handles Tether networks properly before we do this. In the
> meanwhile we can do this explicitly when we need to.

Done.

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/tether...
File chromeos/network/tether_constants.h (right):

https://codereview.chromium.org/2818593003/diff/80001/chromeos/network/tether...
chromeos/network/tether_constants.h:16: // chrome.networkingPrivate API.
On 2017/04/18 20:41:19, stevenjb wrote:
> Strictly speaking these have nothing to do with chrome.networkingPrivate which
> uses ONC. These constants are just for integration with NetworkStateHandler
> which primarily deals with Shill services.

Done.

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

https://codereview.chromium.org/2818593003/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:578: long BatteryPercentage;
On 2017/04/18 20:41:19, stevenjb wrote:
> This is optional, right? Same with SignalStrength? This should be 'long?'

No, these should always be present. They are always sent as the same time that
carrier is sent.

https://codereview.chromium.org/2818593003/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:584: ManagedLong BatteryPercentage;
On 2017/04/18 20:41:19, stevenjb wrote:
> ditto

Same.

[stevenjb, 2017-04-18 22:23:06]

We also need to update src/components/onc/docs/onc_spec.md

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

https://codereview.chromium.org/2818593003/diff/80001/extensions/common/api/n...
extensions/common/api/networking_private.idl:578: long BatteryPercentage;
On 2017/04/18 21:58:39, Kyle Horimoto wrote:
> On 2017/04/18 20:41:19, stevenjb wrote:
> > This is optional, right? Same with SignalStrength? This should be 'long?'
> 
> No, these should always be present. They are always sent as the same time that
> carrier is sent.

In the ONC validation code you treat these as optional.

https://codereview.chromium.org/2818593003/diff/100001/chromeos/network/onc/o...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/2818593003/diff/100001/chromeos/network/onc/o...
chromeos/network/onc/onc_validator.cc:1032: }
This pattern is for optional parameters. Personally I think making these
optional is fine, but we should be consistent.

[Kyle Horimoto, 2017-04-18 23:25:21]

Updated onc_spec.md as well.

https://codereview.chromium.org/2818593003/diff/100001/chromeos/network/onc/o...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/2818593003/diff/100001/chromeos/network/onc/o...
chromeos/network/onc/onc_validator.cc:1032: }
On 2017/04/18 22:23:06, stevenjb wrote:
> This pattern is for optional parameters. Personally I think making these
> optional is fine, but we should be consistent.

Done.

[stevenjb, 2017-04-18 23:34:15]

https://codereview.chromium.org/2818593003/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/2818593003/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_validator.cc:1046: LOG(ERROR) << "here";
?

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1439: Used for representing a tether hotspot
produced by a device with a cellular
s/produced by a device with a cellular chip/provided by an external device, e.g.
a phone/ ?

I don't think the technology used to provide the hotspot matters from an API
perspective.

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1452: an Internet connection.
'The name of the cellular carrier when the hotspot is provided by a cellular
connection'?

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1456: * The current signal strength for this
network in the range [0, 100].
s/for this network/for the connection to the hotspot/? Or is it the hotspot's
signal strength? We should be clear either way.

[Kyle Horimoto, 2017-04-18 23:41:13]

https://codereview.chromium.org/2818593003/diff/120001/chromeos/network/onc/o...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/2818593003/diff/120001/chromeos/network/onc/o...
chromeos/network/onc/onc_validator.cc:1046: LOG(ERROR) << "here";
On 2017/04/18 23:34:14, stevenjb wrote:
> ?

Sorry - debug log. Removed it.

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1439: Used for representing a tether hotspot
produced by a device with a cellular
On 2017/04/18 23:34:14, stevenjb wrote:
> s/produced by a device with a cellular chip/provided by an external device,
e.g.
> a phone/ ?
> 
> I don't think the technology used to provide the hotspot matters from an API
> perspective.

Done.

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1452: an Internet connection.
On 2017/04/18 23:34:14, stevenjb wrote:
> 'The name of the cellular carrier when the hotspot is provided by a cellular
> connection'?

Done.

https://codereview.chromium.org/2818593003/diff/120001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1456: * The current signal strength for this
network in the range [0, 100].
On 2017/04/18 23:34:15, stevenjb wrote:
> s/for this network/for the connection to the hotspot/? Or is it the hotspot's
> signal strength? We should be clear either way.

Done, and added some extra clarification.

[stevenjb, 2017-04-18 23:46:10]

lgtm w/ clarification request

https://codereview.chromium.org/2818593003/diff/140001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2818593003/diff/140001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1456: * The current signal strength for the
hotspot's signal strength in the
'current value for the hotspot's signal strength' ? (Current phrasing is awkward
/ confusing).

[Kyle Horimoto, 2017-04-18 23:49:22]

https://codereview.chromium.org/2818593003/diff/140001/components/onc/docs/on...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2818593003/diff/140001/components/onc/docs/on...
components/onc/docs/onc_spec.md:1456: * The current signal strength for the
hotspot's signal strength in the
On 2017/04/18 23:46:10, stevenjb wrote:
> 'current value for the hotspot's signal strength' ? (Current phrasing is
awkward
> / confusing).

Done.

[Kyle Horimoto, 2017-04-18 23:49:27]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-18 23:49:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2818593003/#ps160001
(title: "stevenjb@ comment.")

[commit-bot: I haz the power, 2017-04-18 23:49:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 23:58:21]

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

[commit-bot: I haz the power, 2017-04-18 23:58:22]

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

[Kyle Horimoto, 2017-04-19 00:17:01]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-04-19 00:17:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 00:31:22]

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

[commit-bot: I haz the power, 2017-04-19 00:31:23]

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

[Kyle Horimoto, 2017-04-19 00:34:19]

khorimoto@chromium.org changed reviewers:
+ rockot@chromium.org

[Kyle Horimoto, 2017-04-19 00:34:20]

+rockot for networking_private.idl change.

[Ken Rockot(use gerrit already), 2017-04-19 19:59:38]

lgtm

[Kyle Horimoto, 2017-04-19 20:03:40]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-19 20:03:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2818593003/#ps180001
(title: "Ran git cl format.")

[commit-bot: I haz the power, 2017-04-19 20:04:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 21:16:07]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492632220611630,
"parent_rev": "a4be5c7090175b7410f2a9adcb447a111d152c94", "commit_rev":
"e8e89d55e8f02315056a6c524f8fff83fbe1b289"}

[commit-bot: I haz the power, 2017-04-19 21:17:02]

Description was changed from

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[CrOS Tether] Add tether network properties (battery percentage, carrier, and
signal strength) to the Chrome OS networking stack.

This change is composed of several related parts:
(1) Update the chrome.networkingPrivate API to include tethering properties as
part of NetworkProperties, ManagedNetworkProperties, and NetworkStateProperties.
(2) Add ONC properties for the new tethering properties.
(3) Add ONC validation for tether networks.
(4) Add "fake" Shill tethering properties (since tethering networks are really
normal Wi-Fi networks, Shill does not actually ever use the properties).
(5) Add translation from Shill tethering properties to ONC tethering properties.
(6) Update the settings page's chrome.networkingPrivate.getNetworks() call site
to handle tether networks properly.

BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2818593003
Cr-Commit-Position: refs/heads/master@{#465749}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8e89d55e8f02315056a6c524f8f...
==========

[commit-bot: I haz the power, 2017-04-19 21:17:03]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/e8e89d55e8f02315056a6c524f8f...