[CrOS Tether] Create TetherHostResponseRecorder, which records ConnectTetheringResponses and Tether…

[CrOS Tether] Create TetherHostResponseRecorder, which records ConnectTetheringResponses and TetherAvailabilityResponses from tether hosts.

This takes some existing functionality out of HostScanDevicePrioritizer and extends it to record the IDs of all ConnectTetheringResponses received instead of only the most recent one.

This refactor is needed because the networking stack needs to know whether to show a warning dialog (stating that tethering will use mobile data and battery on the host device) before connecting to a device for the first time. Thus, it is necessary to record all previously received ConnectTetheringResponses instead of only the most recent one. Because this logic is unrelated to the host prioritization logic, I moved it to its own class.

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

[Kyle Horimoto, 2017-04-27 03:08:03]

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

[Ryan Hansberry, 2017-04-27 17:45:55]

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer.h (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:20: //    
ConnectTetheringResponse is always at the front of the queue.
Now that we capture all of the devices we've connected to, should the queue be
[all connected devices] + [all available devices] + [the rest] ?

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.cc (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:129:
scanned_device_list_so_far.at(0).remote_device);
How do you plan to inform the UI layer that the Connect button should or should
not show the dialog? (If you plan to do so by passing an argument to
NotificationPresenter, you should do so in this CL)

[Kyle Horimoto, 2017-04-27 17:51:43]

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer.h (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:20: //    
ConnectTetheringResponse is always at the front of the queue.
On 2017/04/27 17:45:54, Ryan Hansberry wrote:
> Now that we capture all of the devices we've connected to, should the queue be
> [all connected devices] + [all available devices] + [the rest] ?

No, I don't think this should be the case. Here's my reasoning:

The first one should always be the most-recently connected device. Having
connected most recently is a strong indication that you want to connect to the
same device again.

After that, we should prioritize the devices which have responded most recently.
Prioritizing by last connection over last response means that if you connected
to a device a really long time ago but you don't use that device anymore
(perhaps you don't even own that device anymore), you'd be prioritizing that
over a device which has recently responded. A recent response is a stronger
indication that that device is nearby and can provide a tether hotspot than
having connected to that device long ago.

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.cc (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:129:
scanned_device_list_so_far.at(0).remote_device);
On 2017/04/27 17:45:54, Ryan Hansberry wrote:
> How do you plan to inform the UI layer that the Connect button should or
should
> not show the dialog? (If you plan to do so by passing an argument to
> NotificationPresenter, you should do so in this CL)

I'm adding a new HasConnectedToHost network property to the network stack (as I
did with battery percentage, carrier, and signal strength). When
NetworkConnect::ConnectToNetworkId() is called, NetworkConnect will grab the
corresponding NetworkState from NetworkStateHandler and check whether that
device has connected already. If it has not yet connected, it will bring up the
settings page instead of starting the connection.

[Ryan Hansberry, 2017-04-27 18:08:26]

lgtm

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scan_device_prioritizer.h (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:20: //    
ConnectTetheringResponse is always at the front of the queue.
On 2017/04/27 17:51:42, Kyle Horimoto wrote:
> On 2017/04/27 17:45:54, Ryan Hansberry wrote:
> > Now that we capture all of the devices we've connected to, should the queue
be
> > [all connected devices] + [all available devices] + [the rest] ?
> 
> No, I don't think this should be the case. Here's my reasoning:
> 
> The first one should always be the most-recently connected device. Having
> connected most recently is a strong indication that you want to connect to the
> same device again.
> 
> After that, we should prioritize the devices which have responded most
recently.
> Prioritizing by last connection over last response means that if you connected
> to a device a really long time ago but you don't use that device anymore
> (perhaps you don't even own that device anymore), you'd be prioritizing that
> over a device which has recently responded. A recent response is a stronger
> indication that that device is nearby and can provide a tether hotspot than
> having connected to that device long ago.

Alright, I agree. Thanks.

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:23: //     violate the
first rule.
Re-reading this is kind of odd. How can the first rule be "violated"? This last
phrase just seems unnecessary.

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.cc (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:129:
scanned_device_list_so_far.at(0).remote_device);
On 2017/04/27 17:51:42, Kyle Horimoto wrote:
> On 2017/04/27 17:45:54, Ryan Hansberry wrote:
> > How do you plan to inform the UI layer that the Connect button should or
> should
> > not show the dialog? (If you plan to do so by passing an argument to
> > NotificationPresenter, you should do so in this CL)
> 
> I'm adding a new HasConnectedToHost network property to the network stack (as
I
> did with battery percentage, carrier, and signal strength). When
> NetworkConnect::ConnectToNetworkId() is called, NetworkConnect will grab the
> corresponding NetworkState from NetworkStateHandler and check whether that
> device has connected already. If it has not yet connected, it will bring up
the
> settings page instead of starting the connection.

Got it. I assume that will be passed as a new argument of AddTetherNetworkState,
so please add a TODO above the invocation of it in this file indicating that
that will be done.

[Kyle Horimoto, 2017-04-27 18:10:55]

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
File chromeos/components/tether/host_scanner.cc (right):

https://codereview.chromium.org/2844973002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:129:
scanned_device_list_so_far.at(0).remote_device);
On 2017/04/27 18:08:25, Ryan Hansberry wrote:
> On 2017/04/27 17:51:42, Kyle Horimoto wrote:
> > On 2017/04/27 17:45:54, Ryan Hansberry wrote:
> > > How do you plan to inform the UI layer that the Connect button should or
> > should
> > > not show the dialog? (If you plan to do so by passing an argument to
> > > NotificationPresenter, you should do so in this CL)
> > 
> > I'm adding a new HasConnectedToHost network property to the network stack
(as
> I
> > did with battery percentage, carrier, and signal strength). When
> > NetworkConnect::ConnectToNetworkId() is called, NetworkConnect will grab the
> > corresponding NetworkState from NetworkStateHandler and check whether that
> > device has connected already. If it has not yet connected, it will bring up
> the
> > settings page instead of starting the connection.
> 
> Got it. I assume that will be passed as a new argument of
AddTetherNetworkState,
> so please add a TODO above the invocation of it in this file indicating that
> that will be done.

Done.

[Kyle Horimoto, 2017-04-27 18:11:02]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-27 18:11:02]

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/2844973002/#ps40001
(title: "hansberry@ comment.")

[commit-bot: I haz the power, 2017-04-27 18:11:35]

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-27 18:29:07]

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

[commit-bot: I haz the power, 2017-04-27 18:29:09]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-04-27 18:38:02]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-27 18:38:11]

The CQ bit was unchecked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-27 18:41:19]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-04-27 18:41:20]

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/2844973002/#ps60001
(title: "Changed "Connectable" wording to "Connected".")

[commit-bot: I haz the power, 2017-04-27 18:41:56]

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-27 19:10:49]

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

[commit-bot: I haz the power, 2017-04-27 19:10:50]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kyle Horimoto, 2017-04-27 19:12:10]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-04-27 19:12:47]

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-27 20:53:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493320330674560,
"parent_rev": "2b5a31c0dc8dcea1ae68da6b000c3622e68b4b9c", "commit_rev":
"bdc215987e3feb39a009ccb797bf751ba42311d6"}

[commit-bot: I haz the power, 2017-04-27 20:53:22]

Description was changed from

==========
[CrOS Tether] Create TetherHostResponseRecorder, which records
ConnectTetheringResponses and TetherAvailabilityResponses from tether hosts.

This takes some existing functionality out of HostScanDevicePrioritizer and
extends it to record the IDs of all ConnectTetheringResponses received instead
of only the most recent one.

This refactor is needed because the networking stack needs to know whether to
show a warning dialog (stating that tethering will use mobile data and battery
on the host device) before connecting to a device for the first time. Thus, it
is necessary to record all previously received ConnectTetheringResponses instead
of only the most recent one. Because this logic is unrelated to the host
prioritization logic, I moved it to its own class.

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

to

==========
[CrOS Tether] Create TetherHostResponseRecorder, which records
ConnectTetheringResponses and TetherAvailabilityResponses from tether hosts.

This takes some existing functionality out of HostScanDevicePrioritizer and
extends it to record the IDs of all ConnectTetheringResponses received instead
of only the most recent one.

This refactor is needed because the networking stack needs to know whether to
show a warning dialog (stating that tethering will use mobile data and battery
on the host device) before connecting to a device for the first time. Thus, it
is necessary to record all previously received ConnectTetheringResponses instead
of only the most recent one. Because this logic is unrelated to the host
prioritization logic, I moved it to its own class.

BUG=672263

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

[commit-bot: I haz the power, 2017-04-27 20:53:24]

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