[CrOS Tether] Create HostScanDevicePrioritizer, a class which prioritizes the order of connection a…

[CrOS Tether] Create HostScanDevicePrioritizer, a class which prioritizes the order of connection attempts to tether hosts.

This change ensures that CrOS devices attempt to connect to devices which have worked in the past before attempting to connect to devices which have not been used before.

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

[Kyle Horimoto, 2017-02-24 02:23:50]

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

[Ryan Hansberry, 2017-02-24 16:00:17]

lgtm with nits. I essentially am rubber-stamping the parts with Prefs -- please
ask for a reviewer more familiar with them, e.g. tengs@

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

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:45:
updated_ids->Remove(*device_id_value, &unused_index);
I believe you can instead pass null as the second argument:
https://cs.chromium.org/chromium/src/base/values.h?type=cs&l=421

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:58:
base::Value(remote_device.GetDeviceId()));
Sanity check: this is UTF-8 right? i.e., safe to put into prefs?

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:80: if
(connect_tethering_id != "") {
Use empty() instead.

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:88: for (size_t i =
prioritized_ids->GetSize(); i-- > 0;) {
Rearrange to '...; i >= 0; i--)'

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:90: if
(prioritized_ids->Get(i, &stored_id_value)) {
Instead of all these nested if statements, use continue when if conditions are
false.

[Kyle Horimoto, 2017-02-24 18:02:59]

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

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:45:
updated_ids->Remove(*device_id_value, &unused_index);
On 2017/02/24 16:00:17, Ryan Hansberry wrote:
> I believe you can instead pass null as the second argument:
> https://cs.chromium.org/chromium/src/base/values.h?type=cs&l=421

Done.

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:58:
base::Value(remote_device.GetDeviceId()));
On 2017/02/24 16:00:17, Ryan Hansberry wrote:
> Sanity check: this is UTF-8 right? i.e., safe to put into prefs?

Yep - it's a base-64 string.

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:80: if
(connect_tethering_id != "") {
On 2017/02/24 16:00:17, Ryan Hansberry wrote:
> Use empty() instead.

Done.

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:88: for (size_t i =
prioritized_ids->GetSize(); i-- > 0;) {
On 2017/02/24 16:00:17, Ryan Hansberry wrote:
> Rearrange to '...; i >= 0; i--)'

That doesn't work since size_t is an unsigned value. Any size_t is >=0.

https://codereview.chromium.org/2713073002/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_device_prioritizer.cc:90: if
(prioritized_ids->Get(i, &stored_id_value)) {
On 2017/02/24 16:00:17, Ryan Hansberry wrote:
> Instead of all these nested if statements, use continue when if conditions are
> false.

Done.

[kkhorimoto, 2017-02-24 18:49:05]

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

[kkhorimoto, 2017-02-24 18:49:05]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-24 18:49:05]

lgtm

[kkhorimoto, 2017-02-24 18:49:05]

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/2713073002/#ps20001
(title: "hansberry@ comments.")

[commit-bot: I haz the power, 2017-02-24 18:49:39]

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

[Kyle Horimoto, 2017-02-24 18:49:57]

The CQ bit was unchecked by khorimoto@chromium.org

[Tim Song, 2017-02-24 18:57:39]

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

[Tim Song, 2017-02-24 18:57:40]

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

https://codereview.chromium.org/2713073002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.cc:87: for (size_t i =
prioritized_ids->GetSize(); i-- > 0;) {
For readability. Move the decrement outside of the comparison.

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

https://codereview.chromium.org/2713073002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:50:
std::vector<cryptauth::RemoteDevice>& remote_devices) const;
According to the Google style guide, all arguments passed by reference must be
const:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

I would make this a pointer instead, and perhaps name the function something
like SortByHostScanOrder() to really emphasize that the vector will be mutated.

[Kyle Horimoto, 2017-02-24 19:21:30]

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

https://codereview.chromium.org/2713073002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.cc:87: for (size_t i =
prioritized_ids->GetSize(); i-- > 0;) {
On 2017/02/24 18:57:40, Tim Song wrote:
> For readability. Move the decrement outside of the comparison.

Unfortunately, that doesn't work since the loop would never run for i == 0. This
seems to be a conventional way to do this:

http://stackoverflow.com/questions/3623263/reverse-iteration-with-an-unsigned...

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

https://codereview.chromium.org/2713073002/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_device_prioritizer.h:50:
std::vector<cryptauth::RemoteDevice>& remote_devices) const;
On 2017/02/24 18:57:40, Tim Song wrote:
> According to the Google style guide, all arguments passed by reference must be
> const:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
> 
> I would make this a pointer instead, and perhaps name the function something
> like SortByHostScanOrder() to really emphasize that the vector will be
mutated.

Done.

[Kyle Horimoto, 2017-02-24 19:21:34]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-02-24 19:21:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, hansberry@chromium.org
Link to the patchset: https://codereview.chromium.org/2713073002/#ps40001
(title: "tengs@ comments.")

[commit-bot: I haz the power, 2017-02-24 19:22:03]

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-02-24 19:29:42]

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

[commit-bot: I haz the power, 2017-02-24 19:29:43]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Kyle Horimoto, 2017-02-24 19:35:05]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 19:35: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-02-24 19:41:53]

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

[commit-bot: I haz the power, 2017-02-24 19:41:54]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 19:42:27]

The CQ bit was checked by khorimoto@chromium.org

[Tim Song, 2017-02-24 19:42:46]

lgtm

[commit-bot: I haz the power, 2017-02-24 19:43:18]

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-02-24 19:50:20]

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

[commit-bot: I haz the power, 2017-02-24 19:50:21]

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

[Kyle Horimoto, 2017-02-24 19:51:20]

The CQ bit was checked by khorimoto@chromium.org

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

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-02-24 19:59:18]

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

[commit-bot: I haz the power, 2017-02-24 19:59:20]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 20:08:28]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 20:09:23]

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-02-24 20:19:38]

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

[commit-bot: I haz the power, 2017-02-24 20:19:39]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 20:56:54]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 20:57:46]

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-02-24 21:04:05]

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

[commit-bot: I haz the power, 2017-02-24 21:04:07]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 21:08:54]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:10:12]

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-02-24 21:16:42]

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

[commit-bot: I haz the power, 2017-02-24 21:16:43]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 21:18:56]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:19:41]

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-02-24 21:28:01]

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

[commit-bot: I haz the power, 2017-02-24 21:28:03]

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

[Kyle Horimoto, 2017-02-24 21:28:46]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:30:02]

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-02-24 21:36:46]

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

[commit-bot: I haz the power, 2017-02-24 21:36:47]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 21:37:54]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:38:46]

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-02-24 21:45:01]

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

[commit-bot: I haz the power, 2017-02-24 21:45:03]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Kyle Horimoto, 2017-02-24 21:50:38]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:51: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-02-24 21:58:55]

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

[commit-bot: I haz the power, 2017-02-24 21:58:57]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Kyle Horimoto, 2017-02-24 22:03:50]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-02-24 22:04:27]

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

[Kyle Horimoto, 2017-02-24 22:09:37]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-02-24 22:09:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, tengs@chromium.org, hansberry@chromium.org
Link to the patchset: https://codereview.chromium.org/2713073002/#ps60001
(title: "Add dep.")

[commit-bot: I haz the power, 2017-02-24 22:10:32]

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-02-25 00:47:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487974177298960,
"parent_rev": "d0beea4e2d9c11e6ab4f94eea91c4b6b87104a56", "commit_rev":
"f788c53efa1ace1ef43714293ee37b844fc9e67a"}

[commit-bot: I haz the power, 2017-02-25 00:47:48]

Description was changed from

==========
[CrOS Tether] Create HostScanDevicePrioritizer, a class which prioritizes the
order of connection attempts to tether hosts.

This change ensures that CrOS devices attempt to connect to devices which have
worked in the past before attempting to connect to devices which have not been
used before.

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

to

==========
[CrOS Tether] Create HostScanDevicePrioritizer, a class which prioritizes the
order of connection attempts to tether hosts.

This change ensures that CrOS devices attempt to connect to devices which have
worked in the past before attempting to connect to devices which have not been
used before.

BUG=672263

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

[commit-bot: I haz the power, 2017-02-25 00:47:50]

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