[CrOS Tether] Create KeepAliveScheduler, a class which schedules keep-alive tickles to be sent to a…

[CrOS Tether] Create KeepAliveScheduler, a class which schedules keep-alive tickles to be sent to a connected host every 4 minutes until the device disconnects.

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

[Kyle Horimoto, 2017-03-10 23:38:15]

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

[Jeremy Klein, 2017-03-10 23:58:08]

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

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:197: TEST_F(ActiveHostTest,
TestObserverCalls) {
Can we also add a test which goes through the early return codepath?

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:199:
EXPECT_FALSE(test_observer_->host_changed_updates().size());
Is EXPECT_FALSE the standard chrome practice for "equals 0"? Seems not as
readable imo.

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

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/keep_alive_scheduler.cc:24:
std::unique_ptr<base::Timer> timer)
Should this also just be a RepeatingTimer? I know it doesn't matter in practice,
but think this class isn't really valid with a OneShotTimer.

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/keep_alive_scheduler.cc:29: if (active_host_) {
When would active_host_ be null here? Is that a legit use case?

[Kyle Horimoto, 2017-03-11 00:03:09]

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

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:197: TEST_F(ActiveHostTest,
TestObserverCalls) {
On 2017/03/10 23:58:08, Jeremy Klein wrote:
> Can we also add a test which goes through the early return codepath?

Done.

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/active_host_unittest.cc:199:
EXPECT_FALSE(test_observer_->host_changed_updates().size());
On 2017/03/10 23:58:08, Jeremy Klein wrote:
> Is EXPECT_FALSE the standard chrome practice for "equals 0"? Seems not as
> readable imo.

Yeah - I don't really like it either :(

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

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/keep_alive_scheduler.cc:24:
std::unique_ptr<base::Timer> timer)
On 2017/03/10 23:58:08, Jeremy Klein wrote:
> Should this also just be a RepeatingTimer? I know it doesn't matter in
practice,
> but think this class isn't really valid with a OneShotTimer.

Yeah, but unfortunately MockTimer doesn't derive from RepeatingTimer. Do you
have any suggestions for how to make this work?

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/keep_alive_scheduler.cc:29: if (active_host_) {
On 2017/03/10 23:58:08, Jeremy Klein wrote:
> When would active_host_ be null here? Is that a legit use case?

Done.

[Jeremy Klein, 2017-03-11 00:09:18]

lgtm

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

https://codereview.chromium.org/2741253002/diff/40001/chromeos/components/tet...
chromeos/components/tether/keep_alive_scheduler.cc:24:
std::unique_ptr<base::Timer> timer)
On 2017/03/11 00:03:09, Kyle Horimoto wrote:
> On 2017/03/10 23:58:08, Jeremy Klein wrote:
> > Should this also just be a RepeatingTimer? I know it doesn't matter in
> practice,
> > but think this class isn't really valid with a OneShotTimer.
> 
> Yeah, but unfortunately MockTimer doesn't derive from RepeatingTimer. Do you
> have any suggestions for how to make this work?

Gah, bummer :-/. Nah, not really. It's not a huge deal.

[Kyle Horimoto, 2017-03-11 00:10:27]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-03-11 00:11:00]

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-11 00:16:27]

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

[commit-bot: I haz the power, 2017-03-11 00:16:28]

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-03-11 00:43:28]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-03-11 00:43:29]

The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org
Link to the patchset: https://codereview.chromium.org/2741253002/#ps80001
(title: "Add missing dependency.")

[commit-bot: I haz the power, 2017-03-11 00:43:54]

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-11 01:43:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489193008747620,
"parent_rev": "a417069dab92fa16d63553b39a898b2f5ed3851e", "commit_rev":
"e4fe160f57cd3d79526b4e7b5c2d20be7014eadc"}

[commit-bot: I haz the power, 2017-03-11 01:44:01]

Description was changed from

==========
[CrOS Tether] Create KeepAliveScheduler, a class which schedules keep-alive
tickles to be sent to a connected host every 4 minutes until the device
disconnects.

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

to

==========
[CrOS Tether] Create KeepAliveScheduler, a class which schedules keep-alive
tickles to be sent to a connected host every 4 minutes until the device
disconnects.

BUG=672263

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

[commit-bot: I haz the power, 2017-03-11 01:44:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e4fe160f57cd3d79526b4e7b5c2d...