CrOS Tether: Create HostScanScheduler, which schedules scans for devices which can serve as tether …

CrOS Tether: Create HostScanScheduler, which schedules scans for devices which can serve as tether hotspots.

Scans can be triggered manually (via a function call) or automatically (when conditions change such that a scan should be performed). These conditions are:
(1) Must be logged in and active.
(2) Must not have a current Internet connection.
(3) Must have synced CryptAuth tether host devices.

Note that this CL does not integrate this code yet. In order to do that, a CryptAuthService must be created to provide the CryptAuthDeviceManager. Once that code is in place, the scheduler will be created an initialized by the Initializer.

BUG=672263
Committed: https://crrev.com/e74a10c8ed2571d066f5901c8d28b3ea89e7eea4
Cr-Commit-Position: refs/heads/master@{#439307}

[Kyle Horimoto, 2016-12-10 00:01:20]

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

[Ryan Hansberry, 2016-12-12 19:18:27]

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

[Ryan Hansberry, 2016-12-12 19:18:28]

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_scheduler.cc:39:
LoginState::Get()->AddObserver(host_scan_scheduler);
RemoveObserver is calling AddObserver methods.

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_scheduler_unittest.cc:148:
TestScheduleScanNowIfPossible(true, false, true, 1);
Shouldn't the expected scan count be 1 iff the params are (true, false, true)?
(authenticated user is logged in, the network is not connected/connecting, and
tether hosts are synced).

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scanner.cc:7: #include "base/location.h"
What is this import used for?

[Kyle Horimoto, 2016-12-12 19:22:13]

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_scheduler.cc:39:
LoginState::Get()->AddObserver(host_scan_scheduler);
On 2016/12/12 19:18:27, Ryan Hansberry wrote:
> RemoveObserver is calling AddObserver methods.

Done.

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scan_scheduler_unittest.cc:148:
TestScheduleScanNowIfPossible(true, false, true, 1);
On 2016/12/12 19:18:27, Ryan Hansberry wrote:
> Shouldn't the expected scan count be 1 iff the params are (true, false, true)?
> (authenticated user is logged in, the network is not connected/connecting, and
> tether hosts are synced).

Yep, and this test confirms that.

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

https://codereview.chromium.org/2564653004/diff/1/chromeos/components/tether/...
chromeos/components/tether/host_scanner.cc:7: #include "base/location.h"
On 2016/12/12 19:18:27, Ryan Hansberry wrote:
> What is this import used for?

Done.

[Ryan Hansberry, 2016-12-17 00:57:11]

lgtm

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.cc:21: namespace {}  // namespace
Is this needed?

[Kyle Horimoto, 2016-12-17 01:18:14]

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.cc:21: namespace {}  // namespace
On 2016/12/17 00:57:11, Ryan Hansberry wrote:
> Is this needed?

Done.

[Kyle Horimoto, 2016-12-17 01:18:19]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2016-12-17 01:18:19]

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

[commit-bot: I haz the power, 2016-12-17 01:19:01]

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, 2016-12-17 01:24:59]

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

[commit-bot: I haz the power, 2016-12-17 01:25:00]

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_...)

[Kyle Horimoto, 2016-12-17 01:39:17]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2016-12-17 01:39:17]

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/2564653004/#ps80001
(title: "Add missing BUILD dependencies.")

[commit-bot: I haz the power, 2016-12-17 01:40:09]

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, 2016-12-17 01:46:33]

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

[commit-bot: I haz the power, 2016-12-17 01:46:34]

Try jobs failed on following builders:
  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, 2016-12-17 01:54:23]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2016-12-17 01:54:24]

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/2564653004/#ps100001
(title: "Add another dependency.")

[commit-bot: I haz the power, 2016-12-17 01:55:06]

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, 2016-12-17 03:29:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481939663830310,
"parent_rev": "63cc4e0da1ec4bda1536321fe184e4e266d1f55c", "commit_rev":
"e0c4745c3d635a68fac425e890256ebd95001b80"}

[commit-bot: I haz the power, 2016-12-17 03:30:32]

Description was changed from

==========
CrOS Tether: Create HostScanScheduler, which schedules scans for devices which
can serve as tether hotspots.

Scans can be triggered manually (via a function call) or automatically (when
conditions change such that a scan should be performed). These conditions are:
(1) Must be logged in and active.
(2) Must not have a current Internet connection.
(3) Must have synced CryptAuth tether host devices.

Note that this CL does not integrate this code yet. In order to do that, a
CryptAuthService must be created to provide the CryptAuthDeviceManager. Once
that code is in place, the scheduler will be created an initialized by the
Initializer.

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

to

==========
CrOS Tether: Create HostScanScheduler, which schedules scans for devices which
can serve as tether hotspots.

Scans can be triggered manually (via a function call) or automatically (when
conditions change such that a scan should be performed). These conditions are:
(1) Must be logged in and active.
(2) Must not have a current Internet connection.
(3) Must have synced CryptAuth tether host devices.

Note that this CL does not integrate this code yet. In order to do that, a
CryptAuthService must be created to provide the CryptAuthDeviceManager. Once
that code is in place, the scheduler will be created an initialized by the
Initializer.

BUG=672263

Review-Url: https://codereview.chromium.org/2564653004
==========

[commit-bot: I haz the power, 2016-12-17 03:30:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-17 03:32:26]

Description was changed from

==========
CrOS Tether: Create HostScanScheduler, which schedules scans for devices which
can serve as tether hotspots.

Scans can be triggered manually (via a function call) or automatically (when
conditions change such that a scan should be performed). These conditions are:
(1) Must be logged in and active.
(2) Must not have a current Internet connection.
(3) Must have synced CryptAuth tether host devices.

Note that this CL does not integrate this code yet. In order to do that, a
CryptAuthService must be created to provide the CryptAuthDeviceManager. Once
that code is in place, the scheduler will be created an initialized by the
Initializer.

BUG=672263

Review-Url: https://codereview.chromium.org/2564653004
==========

to

==========
CrOS Tether: Create HostScanScheduler, which schedules scans for devices which
can serve as tether hotspots.

Scans can be triggered manually (via a function call) or automatically (when
conditions change such that a scan should be performed). These conditions are:
(1) Must be logged in and active.
(2) Must not have a current Internet connection.
(3) Must have synced CryptAuth tether host devices.

Note that this CL does not integrate this code yet. In order to do that, a
CryptAuthService must be created to provide the CryptAuthDeviceManager. Once
that code is in place, the scheduler will be created an initialized by the
Initializer.

BUG=672263

Committed: https://crrev.com/e74a10c8ed2571d066f5901c8d28b3ea89e7eea4
Cr-Commit-Position: refs/heads/master@{#439307}
==========

[commit-bot: I haz the power, 2016-12-17 03:32:27]

Patchset 6 (id:??) landed as
https://crrev.com/e74a10c8ed2571d066f5901c8d28b3ea89e7eea4
Cr-Commit-Position: refs/heads/master@{#439307}

[Tim Song, 2016-12-17 05:13:42]

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:33: //   (1) The user has just
started using the device; specifically, the user has
"started using the device" is a bit confusing, due to multi-profile use cases. I
would just delete this sentence.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:73: device_change_result)
override;
indenting issue

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:78: class Context {
A more conventional name for this would be Delegate.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:80: virtual void
AddObserver(HostScanScheduler* host_scan_scheduler) = 0;
Maybe a better name would be observeSystemEvents(). AddObserver() is a bit
confusing.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:87: class ContextImpl : public
Context {
You can move this to the .cc file.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:98:
HostScanScheduler(std::unique_ptr<Context> context_,
nit: no trailing underscore.

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:60: observer_ ==
test_->host_scan_scheduler_.get();
Assert this condition instead.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:71: void
AreTetherHostsSynced(bool value) { are_tether_hosts_synced = value; }
rename to set_are_thether_hosts_synced()

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:82: class
MockHostScanner : public HostScanner {
This should be FakeHostScanner, as there are no mock functions.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:158:
test_context_->AreTetherHostsSynced(true);
Maybe initialize |test_context_| to these values in setUp()? I noticed it's
duplicated a few times in these tests.

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2016

[Kyle Horimoto, 2016-12-19 21:10:48]

The requested changes have been implemented in
https://codereview.chromium.org/2587783003. Thanks!

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:33: //   (1) The user has just
started using the device; specifically, the user has
On 2016/12/17 05:13:41, Tim Song wrote:
> "started using the device" is a bit confusing, due to multi-profile use cases.
I
> would just delete this sentence.

Done.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:73: device_change_result)
override;
On 2016/12/17 05:13:41, Tim Song wrote:
> indenting issue

This was intentional since it doesn't fit on the previous line.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:78: class Context {
On 2016/12/17 05:13:41, Tim Song wrote:
> A more conventional name for this would be Delegate.

Done.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:80: virtual void
AddObserver(HostScanScheduler* host_scan_scheduler) = 0;
On 2016/12/17 05:13:41, Tim Song wrote:
> Maybe a better name would be observeSystemEvents(). AddObserver() is a bit
> confusing.

I'd like to keep Add/Remove since we really are just observing events from the
delegate.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:87: class ContextImpl : public
Context {
On 2016/12/17 05:13:41, Tim Song wrote:
> You can move this to the .cc file.

This isn't possible since it extends Context, which is a private class.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler.h:98:
HostScanScheduler(std::unique_ptr<Context> context_,
On 2016/12/17 05:13:41, Tim Song wrote:
> nit: no trailing underscore.

Done.

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:60: observer_ ==
test_->host_scan_scheduler_.get();
On 2016/12/17 05:13:41, Tim Song wrote:
> Assert this condition instead.

This function is called in both EXPECT_FALSE() and EXPECT_TRUE(), so I'm leaving
as-is.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:71: void
AreTetherHostsSynced(bool value) { are_tether_hosts_synced = value; }
On 2016/12/17 05:13:41, Tim Song wrote:
> rename to set_are_thether_hosts_synced()

Done.

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scan_scheduler_unittest.cc:82: class
MockHostScanner : public HostScanner {
On 2016/12/17 05:13:41, Tim Song wrote:
> This should be FakeHostScanner, as there are no mock functions.

Done.

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

https://codereview.chromium.org/2564653004/diff/40001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2016/12/17 05:13:41, Tim Song wrote:
> 2016

Done.