[CrOS Tether] Create HostScanCache, which caches scan results and inserts them into the network sta…

[CrOS Tether] Create HostScanCache, which caches scan results and inserts them into the network stack.

This CL also includes some code in the network stack to pipe scan results through and also includes a refactor of HostScanner, since that was the class which previously was in charge of passing scan results to the network stack.

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

[Kyle Horimoto, 2017-04-28 19:52:22]

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

[Kyle Horimoto, 2017-04-28 19:52:23]

stevenjb@: For network code.
hansberry@: For tether code.

[Ryan Hansberry, 2017-04-28 22:31:05]

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_cache.cc:79:
tether_guid_to_timer_map_.emplace(
I think it would be clearer to do this in StartTimer.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_cache.cc:193: // Check that a Timer exists
in |tether_guid_to_timer_map_| and that it is not
nit: This comment isn't really needed.

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:94: // show
Finish this sentence.

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:139:
EXPECT_GT(remote_devices.size(), 3u);
Is there a GTE? Would prefer >= 4.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:288: void VerifyEquivalent(
Please use a more descriptive name.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:533:
fake_host_scanner_operation_factory_->created_operations()[1]
what's going on with the mix of deref'ed and not deref'ed
fake_host_scanner_operation_factory_?

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.h:217: bool has_connected_to_host);
Please explain this argument in the documentation, and please consider a more
clear name -- right now it sounds like a description of the network itself
instead of our relationship to it, i.e. "this tether network has connected to a
host before" as opposed to "we have connected to this tether network before".
Perhaps 'has_been_connected_to_before', etc.

Unfortunately it seems this name is already established in the codebase so I do
not feel strongly about this.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.h:229: // network is registered and its
HasConnectedToHost value was already true,
I assume this shouldn't be camel-case and should be like
|has_connected_to_host_|? Might be wrong though.

[Kyle Horimoto, 2017-04-28 22:45:04]

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_cache.cc:79:
tether_guid_to_timer_map_.emplace(
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> I think it would be clearer to do this in StartTimer.

StartTimer() is also called in the case where the timer is already in the map.
It's cleaner to have the preconditions to that function be the same in both
cases.

Plus, I don't like the idea of initializing a map entry with only a key and no
value.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scan_cache.cc:193: // Check that a Timer exists
in |tether_guid_to_timer_map_| and that it is not
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> nit: This comment isn't really needed.

Done.

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner.cc:94: // show
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> Finish this sentence.

Done.

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

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:139:
EXPECT_GT(remote_devices.size(), 3u);
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> Is there a GTE? Would prefer >= 4.

Nope, only EXPECT_GT.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:288: void VerifyEquivalent(
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> Please use a more descriptive name.

Done.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/components/tet...
chromeos/components/tether/host_scanner_unittest.cc:533:
fake_host_scanner_operation_factory_->created_operations()[1]
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> what's going on with the mix of deref'ed and not deref'ed
> fake_host_scanner_operation_factory_?

created_operations() is a vector of pointers. Chromium style guide necessitates
that const parameters must be const references instead of pointers, so that's
how they are passed to the function.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
File chromeos/network/network_state_handler.h (right):

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.h:217: bool has_connected_to_host);
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> Please explain this argument in the documentation, and please consider a more
> clear name -- right now it sounds like a description of the network itself
> instead of our relationship to it, i.e. "this tether network has connected to
a
> host before" as opposed to "we have connected to this tether network before".
> Perhaps 'has_been_connected_to_before', etc.
> 
> Unfortunately it seems this name is already established in the codebase so I
do
> not feel strongly about this.

Changed the description.

We talked about this naming scheme last week and decided on it together :P. To
jog your memory, we decided that "has_been_connected_to_before" doesn't make
sense because it describes whether this individual network has been connected
to. What we want to know is whether we've connected to the tether host which is
providing this network before.

https://codereview.chromium.org/2852693004/diff/20001/chromeos/network/networ...
chromeos/network/network_state_handler.h:229: // network is registered and its
HasConnectedToHost value was already true,
On 2017/04/28 22:31:04, Ryan Hansberry wrote:
> I assume this shouldn't be camel-case and should be like
> |has_connected_to_host_|? Might be wrong though.

I named it that way because this is the name of the ONC property, but I realize
that isn't exactly clear since this file doesn't deal with ONC properties. I
reworded it.

[stevenjb, 2017-04-28 23:00:22]

1. I think you can (and therefore should) easily separate out the
src/chromeos/network changes, or at least the vast majority of them? I think
that CL would look good to me.

2. The FakeHostScanCache implementation is confusing to me. It seems like
HostScanCache implementation should be the same for any tests, with a Delegate
class handling anything that we want to stub out, rather than using victuals to
override critical methods. I didn't have a chance to take a real close look, so
maybe I'm just confused, but that was my first impression.

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

https://codereview.chromium.org/2852693004/diff/40001/chromeos/components/tet...
chromeos/components/tether/fake_host_scan_cache.h:42:
FakeHostScanCache::CacheEntry* GetCacheEntry(
The cache probably shouldn't be directly editable, return a const* instead?

[Kyle Horimoto, 2017-04-29 00:16:37]

On 2017/04/28 23:00:22, stevenjb wrote:
> 1. I think you can (and therefore should) easily separate out the
> src/chromeos/network changes, or at least the vast majority of them? I think
> that CL would look good to me.

Done: https://codereview.chromium.org/2850993002/
 
> 2. The FakeHostScanCache implementation is confusing to me. It seems like
> HostScanCache implementation should be the same for any tests, with a Delegate
> class handling anything that we want to stub out, rather than using victuals
to
> override critical methods. I didn't have a chance to take a real close look,
so
> maybe I'm just confused, but that was my first impression.

I understand your sentiment, and if it were simply an in-memory cache vs.
passing to NSH, I would agree. However, HostScanCache also includes other issues
such as setting Timers and handling "has connected" changes. Additionally, it
has dependencies on other classes (e.g., ActiveHost,
TetherHostResponseRecorder), so tests that currently rely on FakeHostCache would
also need to have these additional dependencies as well. I think keeping the
existing test double is the best way.

[Kyle Horimoto, 2017-04-29 00:16:55]

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

https://codereview.chromium.org/2852693004/diff/40001/chromeos/components/tet...
chromeos/components/tether/fake_host_scan_cache.h:42:
FakeHostScanCache::CacheEntry* GetCacheEntry(
On 2017/04/28 23:00:22, stevenjb wrote:
> The cache probably shouldn't be directly editable, return a const* instead?

Done.

[stevenjb, 2017-05-01 16:13:02]

On 2017/04/29 00:16:37, Kyle Horimoto wrote:
> On 2017/04/28 23:00:22, stevenjb wrote:
> > 1. I think you can (and therefore should) easily separate out the
> > src/chromeos/network changes, or at least the vast majority of them? I think
> > that CL would look good to me.
> 
> Done: https://codereview.chromium.org/2850993002/
>  
> > 2. The FakeHostScanCache implementation is confusing to me. It seems like
> > HostScanCache implementation should be the same for any tests, with a
Delegate
> > class handling anything that we want to stub out, rather than using victuals
> to
> > override critical methods. I didn't have a chance to take a real close look,
> so
> > maybe I'm just confused, but that was my first impression.
> 
> I understand your sentiment, and if it were simply an in-memory cache vs.
> passing to NSH, I would agree. However, HostScanCache also includes other
issues
> such as setting Timers and handling "has connected" changes. Additionally, it
> has dependencies on other classes (e.g., ActiveHost,
> TetherHostResponseRecorder), so tests that currently rely on FakeHostCache
would
> also need to have these additional dependencies as well. I think keeping the
> existing test double is the best way.

I'll let Ryan make the call, maybe I'm just confused by the class name? Just
make sure that your tests are actually testing interesting behavior.

[Ryan Hansberry, 2017-05-01 17:38:40]

lgtm. Please update the description of this CL as it no longer "includes some
code in the network stack".

[stevenjb, 2017-05-01 18:26:12]

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:29: // HostScanCacheTestst.
s/Testst/Tests/
Also... yuck. We're adding complexity to the run time code for the sake of
testing. Is there a better way we can test this?

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:71:
tether_guid_to_timer_map_.end();
No need for local

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:92:
tether_guid_to_timer_map_[tether_network_guid]->Stop();
You're doing an extra lookup here. Instead set an iterator above and use that,
i.e. found_iter->second->Stop();

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:135: if
(!active_host_tether_guid.empty()) {
Invert logic?

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:164: if
(HasConnectedToHost(tether_network_guid)) {
Invert and continue

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:200:
tether_guid_to_timer_map_[tether_network_guid]->Start(
Duplicate lookup in DCHECK. (Less critical, but still not ideal). Also, you
should probably DCHECK the lookup itself:

auto iter = tether_guid_to_timer_map_.find(tether_network_guid);
DCHECK(iter != tether_guid_to_timer_map_.end());
DCHECK(!iter->second->IsRunning());
...
iter->second->Start(...)

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:85: // stale data for the Tether
network with GUID |tether_network_guid|.
Why does the timer vary based on |tether_network_guid|? I'm confused.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:98: };
Does this need to be a class member?

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache_unittest.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:77: class
MockTimerFactory : public HostScanCache::TimerFactory {
Mock has a specific meaning in chrome code. This is a fake.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:136: expected_cache_ =
base::MakeUnique<FakeHostScanCache>();
I am trying to review this but I am finding the tests extraordinarily confusing.

1. The added complexity of overriding HostScanCache::TimerFactory makes me sad.
I'm sure there is a better way to do that.

2. Having both a HostScanCache and a FakeHostScanCache in the tests hurts my
head. It really seems to me that HostScanCache should either have an observer
that the test can use to keep track of what is going on, or mechanisms to query
HostScanCache's internal state.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/mock_tether_host_response_recorder.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/mock_tether_host_response_recorder.cc:17:
NotifyObserversPreviouslyConnectedHostIdsChanged();
Why not just call this directly in the test? (Use 'friend' if necessary).

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/mock_tether_host_response_recorder.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/mock_tether_host_response_recorder.h:25: void
NotifyPreviouslyConnectedHostIdsChanged();
It is unconventional for mocks to have methods. Instead make use of WillOnce,
etc in tests.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/tether_host_response_recorder.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_host_response_recorder.cc:54: if (changed) {
local not needed

[Kyle Horimoto, 2017-05-01 21:14:44]

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:29: // HostScanCacheTestst.
On 2017/05/01 18:26:11, stevenjb wrote:
> s/Testst/Tests/
> Also... yuck. We're adding complexity to the run time code for the sake of
> testing. Is there a better way we can test this?

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:71:
tether_guid_to_timer_map_.end();
On 2017/05/01 18:26:11, stevenjb wrote:
> No need for local

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:92:
tether_guid_to_timer_map_[tether_network_guid]->Stop();
On 2017/05/01 18:26:11, stevenjb wrote:
> You're doing an extra lookup here. Instead set an iterator above and use that,
> i.e. found_iter->second->Stop();

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:135: if
(!active_host_tether_guid.empty()) {
On 2017/05/01 18:26:11, stevenjb wrote:
> Invert logic?

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:164: if
(HasConnectedToHost(tether_network_guid)) {
On 2017/05/01 18:26:11, stevenjb wrote:
> Invert and continue

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:200:
tether_guid_to_timer_map_[tether_network_guid]->Start(
On 2017/05/01 18:26:11, stevenjb wrote:
> Duplicate lookup in DCHECK. (Less critical, but still not ideal). Also, you
> should probably DCHECK the lookup itself:
> 
> auto iter = tether_guid_to_timer_map_.find(tether_network_guid);
> DCHECK(iter != tether_guid_to_timer_map_.end());
> DCHECK(!iter->second->IsRunning());
> ...
> iter->second->Start(...)

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:85: // stale data for the Tether
network with GUID |tether_network_guid|.
On 2017/05/01 18:26:11, stevenjb wrote:
> Why does the timer vary based on |tether_network_guid|? I'm confused.

Removed this.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:98: };
On 2017/05/01 18:26:11, stevenjb wrote:
> Does this need to be a class member?

Yes; otherwise, we would not be able to inject MockTimers in the test.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache_unittest.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:77: class
MockTimerFactory : public HostScanCache::TimerFactory {
On 2017/05/01 18:26:11, stevenjb wrote:
> Mock has a specific meaning in chrome code. This is a fake.

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:136: expected_cache_ =
base::MakeUnique<FakeHostScanCache>();
On 2017/05/01 18:26:11, stevenjb wrote:
> I am trying to review this but I am finding the tests extraordinarily
confusing.
> 
> 1. The added complexity of overriding HostScanCache::TimerFactory makes me
sad.
> I'm sure there is a better way to do that.
> 
> 2. Having both a HostScanCache and a FakeHostScanCache in the tests hurts my
> head. It really seems to me that HostScanCache should either have an observer
> that the test can use to keep track of what is going on, or mechanisms to
query
> HostScanCache's internal state.

1. Addressed your comment. I added a set_tether_network_guid_for_next_timer()
function to the timer factory.

2. As discussed offline, I started adding an in-memory mapping for this, but it
essentially turned into the FakeHostScanCache implementation. I added a TODO to
clean this up at a later time.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/mock_tether_host_response_recorder.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/mock_tether_host_response_recorder.cc:17:
NotifyObserversPreviouslyConnectedHostIdsChanged();
On 2017/05/01 18:26:11, stevenjb wrote:
> Why not just call this directly in the test? (Use 'friend' if necessary).

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/mock_tether_host_response_recorder.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/mock_tether_host_response_recorder.h:25: void
NotifyPreviouslyConnectedHostIdsChanged();
On 2017/05/01 18:26:11, stevenjb wrote:
> It is unconventional for mocks to have methods. Instead make use of WillOnce,
> etc in tests.

Done.

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/tether_host_response_recorder.cc (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/tether_host_response_recorder.cc:54: if (changed) {
On 2017/05/01 18:26:12, stevenjb wrote:
> local not needed

Done.

[stevenjb, 2017-05-01 22:13:01]

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:98: };
On 2017/05/01 21:14:43, Kyle Horimoto wrote:
> On 2017/05/01 18:26:11, stevenjb wrote:
> > Does this need to be a class member?
> 
> Yes; otherwise, we would not be able to inject MockTimers in the test.

TimerFactory needs to be public, but TimerFactoryImpl shouldn't need to be, nor
should it need to be part of the class. i.e. in the C++ file:

namespace {

TimerFactoryImpl : public HostScanCache::TimerFactory {
  ...
};

}  // namespace

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.cc (right):

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:130: if
(active_host_tether_guid.empty()) {
No, I meant, isn't this logic backwards? If active_host_tether_guid is empty,
doesn't that clear everything?

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache_unittest.cc (right):

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:94: }
This is definitely a better solution IMHO. An extra small amount of complexity
is well worth it for a simpler API in the production code.

[Kyle Horimoto, 2017-05-02 00:02:19]

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.h (right):

https://codereview.chromium.org/2852693004/diff/100001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.h:98: };
On 2017/05/01 22:13:01, stevenjb wrote:
> On 2017/05/01 21:14:43, Kyle Horimoto wrote:
> > On 2017/05/01 18:26:11, stevenjb wrote:
> > > Does this need to be a class member?
> > 
> > Yes; otherwise, we would not be able to inject MockTimers in the test.
> 
> TimerFactory needs to be public, but TimerFactoryImpl shouldn't need to be,
nor
> should it need to be part of the class. i.e. in the C++ file:
> 
> namespace {
> 
> TimerFactoryImpl : public HostScanCache::TimerFactory {
>   ...
> };
> 
> }  // namespace
> 

Done.

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache.cc (right):

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
chromeos/components/tether/host_scan_cache.cc:130: if
(active_host_tether_guid.empty()) {
On 2017/05/01 22:13:01, stevenjb wrote:
> No, I meant, isn't this logic backwards? If active_host_tether_guid is empty,
> doesn't that clear everything?
> 

You're right! Done.

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
File chromeos/components/tether/host_scan_cache_unittest.cc (right):

https://codereview.chromium.org/2852693004/diff/120001/chromeos/components/te...
chromeos/components/tether/host_scan_cache_unittest.cc:94: }
On 2017/05/01 22:13:01, stevenjb wrote:
> This is definitely a better solution IMHO. An extra small amount of complexity
> is well worth it for a simpler API in the production code.

Yay! It actually didn't end up being as hard as I thought either - awesome! :)

[stevenjb, 2017-05-02 15:20:20]

ok, this lgtm.

[Kyle Horimoto, 2017-05-02 16:50:05]

The CQ bit was checked by khorimoto@chromium.org

[Kyle Horimoto, 2017-05-02 16:50: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/2852693004/#ps140001
(title: "stevenjb@ comments.")

[commit-bot: I haz the power, 2017-05-02 16:50:22]

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-05-02 17:58:16]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493743805057360,
"parent_rev": "ee7474ea14ebc2764e277fdf03557851c1976038", "commit_rev":
"6b5b7de103764340711bc970879caf4afeab23b0"}

[commit-bot: I haz the power, 2017-05-02 17:58:29]

Description was changed from

==========
[CrOS Tether] Create HostScanCache, which caches scan results and inserts them
into the network stack.

This CL also includes some code in the network stack to pipe scan results
through and also includes a refactor of HostScanner, since that was the class
which previously was in charge of passing scan results to the network stack.

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

to

==========
[CrOS Tether] Create HostScanCache, which caches scan results and inserts them
into the network stack.

This CL also includes some code in the network stack to pipe scan results
through and also includes a refactor of HostScanner, since that was the class
which previously was in charge of passing scan results to the network stack.

BUG=672263

Review-Url: https://codereview.chromium.org/2852693004
Cr-Commit-Position: refs/heads/master@{#468705}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b5b7de103764340711bc970879c...
==========

[commit-bot: I haz the power, 2017-05-02 17:58:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/6b5b7de103764340711bc970879c...

[findit-for-me, 2017-05-02 19:07:43]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2861443002/ by
findit-for-me@appspot.gserviceaccount.com.

The reason for reverting is: 
Findit (https://goo.gl/kROfz5) identified CL at revision 468705 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....