[CrOS Tether] Create the ActiveHost class, which stores metadata related to the current connection …

[CrOS Tether] Create the ActiveHost class, which stores metadata related to the current connection to a tether host.

BUG=672263
Review-Url: https://codereview.chromium.org/2734033002
Cr-Commit-Position: refs/heads/master@{#455904}
Committed: https://chromium.googlesource.com/chromium/src/+/1adf5968f86a276628968dccc2467d91677b1107

[Kyle Horimoto, 2017-03-07 00:38:56]

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

[Ryan Hansberry, 2017-03-09 17:20:48]

Have not looked at tests yet.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host.cc (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.cc:122:
DCHECK(GetTetherNetworkSsid().empty());
It's possible that the network_id is known (non-empty) but the Wi-Fi network is
still connecting (for example, we recently received the ConnectTetheringResponse
and have configured the Wi-Fi network, but are waiting to fully connect). tldr;
this check is incorrect.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.cc:124: std::move(remote_device), "");
add a /* argument comment */

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host.h (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.h:69: const std::string&
tether_network_ssid)>;
We'll be referencing the Wi-Fi network by its network_id, instead of its SSID.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host_unittest.cc (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host_unittest.cc:33: other.remote_device &&
*remote_device == *other.remote_device;
C++ noob question: do you need to dereference shared_ptr for comparison? It's
surprising to me that shared_ptr doesn't do any kind of magic internally to make
== just work.

[Kyle Horimoto, 2017-03-09 18:42:01]

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host.cc (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.cc:122:
DCHECK(GetTetherNetworkSsid().empty());
On 2017/03/09 17:20:48, Ryan Hansberry wrote:
> It's possible that the network_id is known (non-empty) but the Wi-Fi network
is
> still connecting (for example, we recently received the
ConnectTetheringResponse
> and have configured the Wi-Fi network, but are waiting to fully connect).
tldr;
> this check is incorrect.

As discussed offline, this is an implementation detail of the "connecting"
state. Leaving as is.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.cc:124: std::move(remote_device), "");
On 2017/03/09 17:20:48, Ryan Hansberry wrote:
> add a /* argument comment */

Done.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host.h (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host.h:69: const std::string&
tether_network_ssid)>;
On 2017/03/09 17:20:48, Ryan Hansberry wrote:
> We'll be referencing the Wi-Fi network by its network_id, instead of its SSID.

Done.

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
File chromeos/components/tether/active_host_unittest.cc (right):

https://codereview.chromium.org/2734033002/diff/1/chromeos/components/tether/...
chromeos/components/tether/active_host_unittest.cc:33: other.remote_device &&
*remote_device == *other.remote_device;
On 2017/03/09 17:20:48, Ryan Hansberry wrote:
> C++ noob question: do you need to dereference shared_ptr for comparison? It's
> surprising to me that shared_ptr doesn't do any kind of magic internally to
make
> == just work.

operator== will compare the values of the pointers (i.e., the address pointed
to). This won't work if we have two separate objects at different addresses
which have the same value. You need to dereference to perform that comparison.

[Ryan Hansberry, 2017-03-09 19:18:21]

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/active_host_unittest.cc (right):

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:163:
active_host_->SetActiveHostDisconnected();
I don't doubt that this could happen, but when do you foresee it happening?

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/pref_names.h (right):

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:29: // The device ID of the active host.
If there is no active host, the value at
value of

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:33: // The network ID of the active
host. If there is no active host, the value at
value of

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:35: extern const char
kTetherNetworkId[];
In the network stack, there will simultaneously be the tether network (the
visible network in the UI) and the wifi network (the real network that is
hidden). Because this network_id is referring to the wifi network, please
instead name it kWifiNetworkId. Please also apply this to the rest of the CL.

[Kyle Horimoto, 2017-03-09 19:29:28]

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/active_host_unittest.cc (right):

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:163:
active_host_->SetActiveHostDisconnected();
On 2017/03/09 19:18:20, Ryan Hansberry wrote:
> I don't doubt that this could happen, but when do you foresee it happening?

0) Connected to a device.
1) Someone calls GetActiveHost().
2) The connection is lost.
3) The GetActiveHost() command returns.

At this point, the fetched device is no longer the active host.

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/pref_names.h (right):

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:29: // The device ID of the active host.
If there is no active host, the value at
On 2017/03/09 19:18:20, Ryan Hansberry wrote:
> value of

I think you're misreading this. The value of this key is a constant and can
never be "". The value in prefs at this key is the thing that can be "".

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:33: // The network ID of the active
host. If there is no active host, the value at
On 2017/03/09 19:18:20, Ryan Hansberry wrote:
> value of

Same.

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:35: extern const char
kTetherNetworkId[];
On 2017/03/09 19:18:20, Ryan Hansberry wrote:
> In the network stack, there will simultaneously be the tether network (the
> visible network in the UI) and the wifi network (the real network that is
> hidden). Because this network_id is referring to the wifi network, please
> instead name it kWifiNetworkId. Please also apply this to the rest of the CL.

Done.

[Ryan Hansberry, 2017-03-09 19:48:20]

lgtm with nit

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
File chromeos/components/tether/pref_names.h (right):

https://codereview.chromium.org/2734033002/diff/40001/chromeos/components/tet...
chromeos/components/tether/pref_names.h:29: // The device ID of the active host.
If there is no active host, the value at
On 2017/03/09 19:29:28, Kyle Horimoto wrote:
> On 2017/03/09 19:18:20, Ryan Hansberry wrote:
> > value of
> 
> I think you're misreading this. The value of this key is a constant and can
> never be "". The value in prefs at this key is the thing that can be "".

Oops :)

https://codereview.chromium.org/2734033002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/active_host.h (right):

https://codereview.chromium.org/2734033002/diff/60001/chromeos/components/tet...
chromeos/components/tether/active_host.h:56: // with Wi-Fi network ID
|tether_network_id|.
wifi_network_id

[Kyle Horimoto, 2017-03-09 19:56:04]

https://codereview.chromium.org/2734033002/diff/60001/chromeos/components/tet...
File chromeos/components/tether/active_host.h (right):

https://codereview.chromium.org/2734033002/diff/60001/chromeos/components/tet...
chromeos/components/tether/active_host.h:56: // with Wi-Fi network ID
|tether_network_id|.
On 2017/03/09 19:48:20, Ryan Hansberry wrote:
> wifi_network_id

Done.

[Jeremy Klein, 2017-03-09 20:13:01]

jlklein@chromium.org changed reviewers:
+ jlklein@chromium.org

[Jeremy Klein, 2017-03-09 20:13:02]

lgtm

https://codereview.chromium.org/2734033002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/active_host.cc (right):

https://codereview.chromium.org/2734033002/diff/80001/chromeos/components/tet...
chromeos/components/tether/active_host.cc:117: // If the active host has changed
while the tether host was being fetched,
Do we do this in Android? Is it more likely to change here for some reason?

[Kyle Horimoto, 2017-03-09 21:19:38]

https://codereview.chromium.org/2734033002/diff/80001/chromeos/components/tet...
File chromeos/components/tether/active_host.cc (right):

https://codereview.chromium.org/2734033002/diff/80001/chromeos/components/tet...
chromeos/components/tether/active_host.cc:117: // If the active host has changed
while the tether host was being fetched,
On 2017/03/09 20:13:02, Jeremy Klein wrote:
> Do we do this in Android? Is it more likely to change here for some reason?

Nope, we don't. I don't think it's any more likely here vs. there. We should
probably fix this in Android as well.

[Kyle Horimoto, 2017-03-09 21:19:54]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-03-09 21:19:55]

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

[commit-bot: I haz the power, 2017-03-09 21:20:31]

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

[Kyle Horimoto, 2017-03-09 22:27:12]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-03-09 22:27:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
jlklein@chromium.org, hansberry@chromium.org
Link to the patchset: https://codereview.chromium.org/2734033002/#ps100001
(title: "hansberry@ comment.")

[commit-bot: I haz the power, 2017-03-09 22:27:50]

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-03-09 23:48:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489098432263250,
"parent_rev": "a747cae76f59dcea4d83fec77534b35978b8a272", "commit_rev":
"1adf5968f86a276628968dccc2467d91677b1107"}

[commit-bot: I haz the power, 2017-03-09 23:49:28]

Description was changed from

==========
[CrOS Tether] Create the ActiveHost class, which stores metadata related to the
current connection to a tether host.

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

to

==========
[CrOS Tether] Create the ActiveHost class, which stores metadata related to the
current connection to a tether host.

BUG=672263

Review-Url: https://codereview.chromium.org/2734033002
Cr-Commit-Position: refs/heads/master@{#455904}
Committed:
https://chromium.googlesource.com/chromium/src/+/1adf5968f86a276628968dccc246...
==========

[commit-bot: I haz the power, 2017-03-09 23:49:29]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1adf5968f86a276628968dccc246...