[CrOS Tether] Add HasConnectedToHost property for Tether networks.

[CrOS Tether] Add HasConnectedToHost property for Tether networks.

This property is necessary because the networking stack must show a dialog to users the first time they connect to a given host device which alerts the user that tethering will use mobile data and battery.

This change adds the new property to the networkingPrivate IDL file and pipes it through from NetworkState. Currently, the value is hard-coded to "false", but this value will be propagated through the network stack from the Tether component in a follow-up CL.

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

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

[Kyle Horimoto, 2017-04-27 18:34:44]

Description was changed from

==========
[CrOS Tether] Add HasConnectedToHost property for Tether networks.

This property is necessary because the networking stack must show a dialog to
users the first time they connect to a given host device which alerts the user
that tethering will use mobile data and battery.

This change adds the new property to the networkingPrivate IDL file and pipes it
through from NetworkState. Currently, the value is hard-coded to "false", but
this value will be propagated through the network stack from the Tether
component in a follow-up CL.

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

to

==========
[CrOS Tether] Add HasConnectedToHost property for Tether networks.

This property is necessary because the networking stack must show a dialog to
users the first time they connect to a given host device which alerts the user
that tethering will use mobile data and battery.

This change adds the new property to the networkingPrivate IDL file and pipes it
through from NetworkState. Currently, the value is hard-coded to "false", but
this value will be propagated through the network stack from the Tether
component in a follow-up CL.

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

[Kyle Horimoto, 2017-04-27 18:35:57]

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

[Kyle Horimoto, 2017-04-27 18:35:58]

rockot@: For networking_private.idl.
stevenjb@: For everything else.

[stevenjb, 2017-04-27 19:28:53]

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:238: bool has_connected_as_tether_client_;
This should be documented because the name is kind of confusing. Also, it's
unclear to me why this is different from the ONC property name
(has_connected_to_host)?

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:535: // accordingly from the Tether
component.
Are we really going to want/need to pass this as a parameter? It seems like
something that would generally get set after creation.

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.md:1459: * **HasConnectedToHost**
I don't love this name. I'm not sure I have a great alternative, maybe something
like 'Authenticated', although that may not quite be right? If you and Ryan are
in agreement then I'm fine with this, just a heads up that it's a bit confusing,
at least to me.

[Kyle Horimoto, 2017-04-27 19:40:23]

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:238: bool has_connected_as_tether_client_;
On 2017/04/27 19:28:53, stevenjb wrote:
> This should be documented because the name is kind of confusing. Also, it's
> unclear to me why this is different from the ONC property name
> (has_connected_to_host)?

Added explanation. The reason why I chose a slightly different name is because
it is not obvious that a has_connected_to_host() function does not apply to
other network types. For example, a future reader of this class may assume that
has_connected_to_host() would return true for a Wi-Fi NetworkState if the device
has already connected to that Wi-Fi network before. To eliminate this ambiguity,
I made sure to include the word "tether" in the name of the function.

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:535: // accordingly from the Tether
component.
On 2017/04/27 19:28:53, stevenjb wrote:
> Are we really going to want/need to pass this as a parameter? It seems like
> something that would generally get set after creation.

I think that it makes sense to send it at the same time as the other parameters
to this function. The reason is that once we call NotifyNetworkListChanged()
below, an Observer could inspect this network and call
has_connected_as_tether_client(), receiving an incorrect value. I want the
NetworkState to be fully up-to-date before invoking any Observer callbacks.

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.md:1459: * **HasConnectedToHost**
On 2017/04/27 19:28:53, stevenjb wrote:
> I don't love this name. I'm not sure I have a great alternative, maybe
something
> like 'Authenticated', although that may not quite be right? If you and Ryan
are
> in agreement then I'm fine with this, just a heads up that it's a bit
confusing,
> at least to me.

Unfortunately, "Authenticated" doesn't apply to this situation. I think
"HasConnectedToHost" is the best choice.

[stevenjb, 2017-04-27 19:45:52]

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:238: bool has_connected_as_tether_client_;
On 2017/04/27 19:40:23, Kyle Horimoto wrote:
> On 2017/04/27 19:28:53, stevenjb wrote:
> > This should be documented because the name is kind of confusing. Also, it's
> > unclear to me why this is different from the ONC property name
> > (has_connected_to_host)?
> 
> Added explanation. The reason why I chose a slightly different name is because
> it is not obvious that a has_connected_to_host() function does not apply to
> other network types. For example, a future reader of this class may assume
that
> has_connected_to_host() would return true for a Wi-Fi NetworkState if the
device
> has already connected to that Wi-Fi network before. To eliminate this
ambiguity,
> I made sure to include the word "tether" in the name of the function.

How about tether_has_connected_to_host?

https://codereview.chromium.org/2844363003/diff/20001/chromeos/network/networ...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/20001/chromeos/network/networ...
chromeos/network/network_state.h:121: // connected to the Tether network
corresponding to this NetworkState.
Comment should be above the member.

[stevenjb, 2017-04-27 19:47:33]

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:535: // accordingly from the Tether
component.
On 2017/04/27 19:40:23, Kyle Horimoto wrote:
> On 2017/04/27 19:28:53, stevenjb wrote:
> > Are we really going to want/need to pass this as a parameter? It seems like
> > something that would generally get set after creation.
> 
> I think that it makes sense to send it at the same time as the other
parameters
> to this function. The reason is that once we call NotifyNetworkListChanged()
> below, an Observer could inspect this network and call
> has_connected_as_tether_client(), receiving an incorrect value. I want the
> NetworkState to be fully up-to-date before invoking any Observer callbacks.

Acknowledged.

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
File components/onc/docs/onc_spec.md (right):

https://codereview.chromium.org/2844363003/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.md:1459: * **HasConnectedToHost**
On 2017/04/27 19:40:23, Kyle Horimoto wrote:
> On 2017/04/27 19:28:53, stevenjb wrote:
> > I don't love this name. I'm not sure I have a great alternative, maybe
> something
> > like 'Authenticated', although that may not quite be right? If you and Ryan
> are
> > in agreement then I'm fine with this, just a heads up that it's a bit
> confusing,
> > at least to me.
> 
> Unfortunately, "Authenticated" doesn't apply to this situation. I think
> "HasConnectedToHost" is the best choice.

As long as you and Ryan and/or Jeremy are in agreement.

[Kyle Horimoto, 2017-04-27 19:51:50]

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/1/chromeos/network/network_st...
chromeos/network/network_state.h:238: bool has_connected_as_tether_client_;
On 2017/04/27 19:45:52, stevenjb wrote:
> On 2017/04/27 19:40:23, Kyle Horimoto wrote:
> > On 2017/04/27 19:28:53, stevenjb wrote:
> > > This should be documented because the name is kind of confusing. Also,
it's
> > > unclear to me why this is different from the ONC property name
> > > (has_connected_to_host)?
> > 
> > Added explanation. The reason why I chose a slightly different name is
because
> > it is not obvious that a has_connected_to_host() function does not apply to
> > other network types. For example, a future reader of this class may assume
> that
> > has_connected_to_host() would return true for a Wi-Fi NetworkState if the
> device
> > has already connected to that Wi-Fi network before. To eliminate this
> ambiguity,
> > I made sure to include the word "tether" in the name of the function.
> 
> How about tether_has_connected_to_host?

Done.

https://codereview.chromium.org/2844363003/diff/20001/chromeos/network/networ...
File chromeos/network/network_state.h (right):

https://codereview.chromium.org/2844363003/diff/20001/chromeos/network/networ...
chromeos/network/network_state.h:121: // connected to the Tether network
corresponding to this NetworkState.
On 2017/04/27 19:45:52, stevenjb wrote:
> Comment should be above the member.

Done.

[Ken Rockot(use gerrit already), 2017-04-27 19:54:26]

I D LGTM

[stevenjb, 2017-04-27 19:55:00]

lgtm

[Kyle Horimoto, 2017-04-28 02:01:25]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-28 02:01:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2844363003/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-04-28 02:02:52]

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-28 03:14:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493344885216510,
"parent_rev": "3b669c732833d94c9b84974af7cf1b00eaeb0f32", "commit_rev":
"f7b4872ff079f43bcb448dcf32d38d1901b952c6"}

[commit-bot: I haz the power, 2017-04-28 03:15:38]

Description was changed from

==========
[CrOS Tether] Add HasConnectedToHost property for Tether networks.

This property is necessary because the networking stack must show a dialog to
users the first time they connect to a given host device which alerts the user
that tethering will use mobile data and battery.

This change adds the new property to the networkingPrivate IDL file and pipes it
through from NetworkState. Currently, the value is hard-coded to "false", but
this value will be propagated through the network stack from the Tether
component in a follow-up CL.

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

to

==========
[CrOS Tether] Add HasConnectedToHost property for Tether networks.

This property is necessary because the networking stack must show a dialog to
users the first time they connect to a given host device which alerts the user
that tethering will use mobile data and battery.

This change adds the new property to the networkingPrivate IDL file and pipes it
through from NetworkState. Currently, the value is hard-coded to "false", but
this value will be propagated through the network stack from the Tether
component in a follow-up CL.

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

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

[commit-bot: I haz the power, 2017-04-28 03:15:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f7b4872ff079f43bcb448dcf32d3...