Commit 7830751b authored by Kyle Horimoto's avatar Kyle Horimoto Committed by Commit Bot

[CrOS Tether] Fix scan behavior of HostScanner.

Previously, when a scan began, we would clear all existing scan results
from the cache, then add results back to the cache as the scan
progressed. This was janky because it would result in the UI removing
Tether networks and then adding them back shortly afterwards when scan
results came in.

This CL changes this functionality so that results are only removed
after a scan ends and the associated device is not found.

Bug: 672263, 746726
Change-Id: I6df2516b1bc0e604c3a496838804bc6ee5c8292c
Reviewed-on: https://chromium-review.googlesource.com/581633
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: default avatarRyan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488817}
parent 9fd3a29a
......@@ -32,20 +32,14 @@ void FakeHostScanCache::SetHostScanResult(const HostScanCacheEntry& entry) {
bool FakeHostScanCache::RemoveHostScanResult(
const std::string& tether_network_guid) {
if (tether_network_guid == active_host_tether_network_guid())
return false;
return cache_.erase(tether_network_guid) > 0;
}
void FakeHostScanCache::ClearCacheExceptForActiveHost() {
auto it = cache_.begin();
while (it != cache_.end()) {
if (it->first == active_host_tether_network_guid_)
it++;
else
it = cache_.erase(it);
}
std::unordered_set<std::string> FakeHostScanCache::GetTetherGuidsInCache() {
std::unordered_set<std::string> tether_guids;
for (const auto& entry : cache_)
tether_guids.insert(entry.first);
return tether_guids;
}
bool FakeHostScanCache::ExistsInCache(const std::string& tether_network_guid) {
......
......@@ -7,6 +7,7 @@
#include <string>
#include <unordered_map>
#include <unordered_set>
#include "base/macros.h"
#include "chromeos/components/tether/host_scan_cache.h"
......@@ -21,16 +22,6 @@ class FakeHostScanCache : virtual public HostScanCache {
FakeHostScanCache();
~FakeHostScanCache() override;
// Getter and setter for the active host Tether network GUID. This value is
// used to implement the ClearCacheExceptForActiveHost() function.
std::string& active_host_tether_network_guid() {
return active_host_tether_network_guid_;
}
void set_active_host_tether_network_guid(
const std::string& active_host_tether_network_guid) {
active_host_tether_network_guid_ = active_host_tether_network_guid;
}
// Getters for contents of the cache.
const HostScanCacheEntry* GetCacheEntry(
const std::string& tether_network_guid);
......@@ -44,11 +35,10 @@ class FakeHostScanCache : virtual public HostScanCache {
void SetHostScanResult(const HostScanCacheEntry& entry) override;
bool RemoveHostScanResult(const std::string& tether_network_guid) override;
bool ExistsInCache(const std::string& tether_network_guid) override;
void ClearCacheExceptForActiveHost() override;
std::unordered_set<std::string> GetTetherGuidsInCache() override;
bool DoesHostRequireSetup(const std::string& tether_network_guid) override;
private:
std::string active_host_tether_network_guid_;
std::unordered_map<std::string, HostScanCacheEntry> cache_;
DISALLOW_COPY_AND_ASSIGN(FakeHostScanCache);
......
......@@ -5,6 +5,9 @@
#ifndef CHROMEOS_COMPONENTS_TETHER_HOST_SCAN_CACHE_H_
#define CHROMEOS_COMPONENTS_TETHER_HOST_SCAN_CACHE_H_
#include <string>
#include <unordered_set>
#include "base/macros.h"
#include "chromeos/components/tether/host_scan_cache_entry.h"
......@@ -33,16 +36,8 @@ class HostScanCache {
// in the cache.
virtual bool ExistsInCache(const std::string& tether_network_guid) = 0;
// Removes all scan results from the cache unless they correspond to the
// active host; the active host must always remain in the cache while
// connecting/connected to ensure the UI is up to date.
// TODO(khorimoto): Remove this function. Currently, scan results are cleared
// when a new scan starts and are filled back in as the scan completes. This
// allows for a situation to occur where the UI removes a network then adds
// it back at some time later, which is undesirable as a user. Instead,
// existing scan results should remain in the cache until a scan concludes and
// no updated scan results for those existing results exist.
virtual void ClearCacheExceptForActiveHost() = 0;
// Returns a set of all Tether network GUIDs that are present in the cache.
virtual std::unordered_set<std::string> GetTetherGuidsInCache() = 0;
// Returns whether the scan result corresponding to |tether_network_guid|
// requires first-time setup (i.e., user interaction) to allow tethering.
......
......@@ -70,6 +70,9 @@ void HostScanner::OnTetherHostsFetched(
const cryptauth::RemoteDeviceList& tether_hosts) {
is_fetching_hosts_ = false;
tether_guids_in_cache_before_scan_ =
host_scan_cache_->GetTetherGuidsInCache();
host_scanner_operation_ = HostScannerOperation::Factory::NewInstance(
tether_hosts, connection_manager_, host_scan_device_prioritizer_,
tether_host_response_recorder_);
......@@ -81,48 +84,30 @@ void HostScanner::OnTetherAvailabilityResponse(
std::vector<HostScannerOperation::ScannedDeviceInfo>&
scanned_device_list_so_far,
bool is_final_scan_result) {
if (scanned_device_list_so_far.empty()) {
// If a new scan is just starting up, remove existing cache entries; if they
// are still within range to communicate with the current device, they will
// show up in a subsequent OnTetherAvailabilityResponse() invocation.
host_scan_cache_->ClearCacheExceptForActiveHost();
} else {
// Add all results received so far to the cache.
for (auto& scanned_device_info : scanned_device_list_so_far) {
SetCacheEntry(scanned_device_info);
}
// Ensure all results received so far are in the cache (setting entries which
// already exist is a no-op).
for (const auto& scanned_device_info : scanned_device_list_so_far) {
SetCacheEntry(scanned_device_info);
}
if (scanned_device_list_so_far.size() == 1) {
const cryptauth::RemoteDevice& remote_device =
scanned_device_list_so_far.at(0).remote_device;
int32_t signal_strength;
NormalizeDeviceStatus(scanned_device_list_so_far.at(0).device_status,
nullptr /* carrier */,
nullptr /* battery_percentage */, &signal_strength);
notification_presenter_->NotifyPotentialHotspotNearby(remote_device,
signal_strength);
} else {
notification_presenter_->NotifyMultiplePotentialHotspotsNearby();
}
if (scanned_device_list_so_far.size() == 1u) {
const cryptauth::RemoteDevice& remote_device =
scanned_device_list_so_far.at(0).remote_device;
int32_t signal_strength;
NormalizeDeviceStatus(scanned_device_list_so_far.at(0).device_status,
nullptr /* carrier */,
nullptr /* battery_percentage */, &signal_strength);
notification_presenter_->NotifyPotentialHotspotNearby(remote_device,
signal_strength);
} else if (scanned_device_list_so_far.size() > 1u) {
// Note: If a single-device notification was previously displayed, calling
// NotifyMultiplePotentialHotspotsNearby() will reuse the existing
// notification.
notification_presenter_->NotifyMultiplePotentialHotspotsNearby();
}
if (is_final_scan_result) {
if (scanned_device_list_so_far.empty()) {
RecordHostScanResult(HostScanResultEventType::NOTIFICATION_NOT_SHOWN);
} else if (scanned_device_list_so_far.size() == 1) {
RecordHostScanResult(
HostScanResultEventType::NOTIFICATION_SHOWN_SINGLE_HOST);
} else {
RecordHostScanResult(
HostScanResultEventType::NOTIFICATION_SHOWN_MULTIPLE_HOSTS);
}
// If the final scan result has been received, the operation is finished.
// Delete it.
host_scanner_operation_->RemoveObserver(this);
host_scanner_operation_.reset();
NotifyScanFinished();
previous_scan_time_ = clock_->Now();
OnFinalScanResultReceived(scanned_device_list_so_far);
}
}
......@@ -165,6 +150,46 @@ void HostScanner::SetCacheEntry(
.Build());
}
void HostScanner::OnFinalScanResultReceived(
std::vector<HostScannerOperation::ScannedDeviceInfo>& final_scan_results) {
// Search through all GUIDs that were in the cache before the scan began. If
// any of those GUIDs are not present in the final scan results, remove them
// from the cache.
for (const auto& tether_guid_in_cache : tether_guids_in_cache_before_scan_) {
bool is_guid_in_final_scan_results = false;
for (const auto& scan_result : final_scan_results) {
if (device_id_tether_network_guid_map_->GetTetherNetworkGuidForDeviceId(
scan_result.remote_device.GetDeviceId()) ==
tether_guid_in_cache) {
is_guid_in_final_scan_results = true;
break;
}
}
if (!is_guid_in_final_scan_results)
host_scan_cache_->RemoveHostScanResult(tether_guid_in_cache);
}
if (final_scan_results.empty()) {
RecordHostScanResult(HostScanResultEventType::NOTIFICATION_NOT_SHOWN);
} else if (final_scan_results.size() == 1u) {
RecordHostScanResult(
HostScanResultEventType::NOTIFICATION_SHOWN_SINGLE_HOST);
} else {
RecordHostScanResult(
HostScanResultEventType::NOTIFICATION_SHOWN_MULTIPLE_HOSTS);
}
// If the final scan result has been received, the operation is finished.
// Delete it.
host_scanner_operation_->RemoveObserver(this);
host_scanner_operation_.reset();
NotifyScanFinished();
previous_scan_time_ = clock_->Now();
}
void HostScanner::RecordHostScanResult(HostScanResultEventType event_type) {
DCHECK(event_type != HostScanResultEventType::HOST_SCAN_RESULT_MAX);
UMA_HISTOGRAM_ENUMERATION("InstantTethering.HostScanResult", event_type,
......
......@@ -5,6 +5,8 @@
#ifndef CHROMEOS_COMPONENTS_TETHER_HOST_SCANNER_H_
#define CHROMEOS_COMPONENTS_TETHER_HOST_SCANNER_H_
#include <string>
#include <unordered_set>
#include <vector>
#include "base/gtest_prod_util.h"
......@@ -55,6 +57,8 @@ class HostScanner : public HostScannerOperation::Observer {
// Returns true if a scan has occurred recently within a timespan, determined
// by HostScanCache::kNumMinutesBeforeCacheEntryExpires.
// TODO(hansberry): Remove this in your upcoming CL since it's no longer
// necessary.
virtual bool HasRecentlyScanned();
// Starts a host scan if there is no current scan. If a scan is active, this
......@@ -87,7 +91,8 @@ class HostScanner : public HostScannerOperation::Observer {
void OnTetherHostsFetched(const cryptauth::RemoteDeviceList& tether_hosts);
void SetCacheEntry(
const HostScannerOperation::ScannedDeviceInfo& scanned_device_info);
void OnFinalScanResultReceived(
std::vector<HostScannerOperation::ScannedDeviceInfo>& final_scan_results);
void RecordHostScanResult(HostScanResultEventType event_type);
TetherHostFetcher* tether_host_fetcher_;
......@@ -101,6 +106,7 @@ class HostScanner : public HostScannerOperation::Observer {
bool is_fetching_hosts_;
std::unique_ptr<HostScannerOperation> host_scanner_operation_;
std::unordered_set<std::string> tether_guids_in_cache_before_scan_;
base::Time previous_scan_time_;
base::ObserverList<Observer> observer_list_;
......
......@@ -34,7 +34,8 @@ MasterHostScanCache::MasterHostScanCache(
MasterHostScanCache::~MasterHostScanCache() {
DCHECK(ActiveHost::ActiveHostStatus::DISCONNECTED ==
active_host_->GetActiveHostStatus());
ClearCacheExceptForActiveHost();
for (const auto& tether_guid : GetTetherGuidsInCache())
RemoveHostScanResult(tether_guid);
}
void MasterHostScanCache::SetHostScanResult(const HostScanCacheEntry& entry) {
......@@ -104,39 +105,13 @@ bool MasterHostScanCache::RemoveHostScanResult(
removed_from_timer_map;
}
void MasterHostScanCache::ClearCacheExceptForActiveHost() {
// Create a list of all Tether network GUIDs serving as keys to
// |tether_guid_to_timer_map_|.
std::vector<std::string> tether_network_guids;
tether_network_guids.reserve(tether_guid_to_timer_map_.size());
for (auto& it : tether_guid_to_timer_map_)
tether_network_guids.push_back(it.first);
std::string active_host_tether_guid = active_host_->GetTetherNetworkGuid();
if (active_host_tether_guid.empty()) {
PA_LOG(INFO) << "Clearing all " << tether_guid_to_timer_map_.size() << " "
<< "entries from the cache.";
} else {
PA_LOG(INFO) << "Clearing " << (tether_guid_to_timer_map_.size() - 1) << " "
<< "of the " << tether_guid_to_timer_map_.size() << " "
<< "entries from the cache. Not removing the entry "
<< "corresponding to the Tether network with GUID \""
<< active_host_tether_guid << "\" because it represents the "
<< "active host.";
}
// Iterate through the keys, removing all scan results not corresponding to
// the active host. Iteration is done via a list of pre-computed keys instead
// if iterating through the map because RemoteHostScanResult() will remove
// key/value pairs from the map, which would invalidate the map iterator.
for (auto& tether_network_guid : tether_network_guids) {
if (active_host_->GetTetherNetworkGuid() == tether_network_guid) {
// Do not remove the active host from the cache.
continue;
}
RemoveHostScanResult(tether_network_guid);
}
std::unordered_set<std::string> MasterHostScanCache::GetTetherGuidsInCache() {
std::unordered_set<std::string> tether_guids;
for (const auto& entry : tether_guid_to_timer_map_)
tether_guids.insert(entry.first);
DCHECK(tether_guids == persistent_host_scan_cache_->GetTetherGuidsInCache() &&
tether_guids == network_host_scan_cache_->GetTetherGuidsInCache());
return tether_guids;
}
bool MasterHostScanCache::ExistsInCache(
......
......@@ -6,7 +6,9 @@
#define CHROMEOS_COMPONENTS_TETHER_MASTER_HOST_SCAN_CACHE_H_
#include <memory>
#include <string>
#include <unordered_map>
#include <unordered_set>
#include "base/gtest_prod_util.h"
#include "base/macros.h"
......@@ -53,7 +55,7 @@ class MasterHostScanCache : public HostScanCache {
void SetHostScanResult(const HostScanCacheEntry& entry) override;
bool RemoveHostScanResult(const std::string& tether_network_guid) override;
bool ExistsInCache(const std::string& tether_network_guid) override;
void ClearCacheExceptForActiveHost() override;
std::unordered_set<std::string> GetTetherGuidsInCache() override;
bool DoesHostRequireSetup(const std::string& tether_network_guid) override;
private:
......
......@@ -141,8 +141,6 @@ class MasterHostScanCacheTest : public testing::Test {
// If the device whose correlated timer has fired is not the active host, it
// is expected to be removed from the cache.
EXPECT_EQ(fake_active_host_->GetTetherNetworkGuid(),
expected_cache_->active_host_tether_network_guid());
if (fake_active_host_->GetTetherNetworkGuid() != tether_network_guid) {
expected_cache_->RemoveHostScanResult(tether_network_guid);
}
......@@ -157,7 +155,6 @@ class MasterHostScanCacheTest : public testing::Test {
tether_network_guid),
tether_network_guid, "wifiNetworkGuid");
}
expected_cache_->set_active_host_tether_network_guid(tether_network_guid);
}
void SetHostScanResult(const HostScanCacheEntry& entry) {
......@@ -169,12 +166,8 @@ class MasterHostScanCacheTest : public testing::Test {
void RemoveHostScanResult(const std::string& tether_network_guid) {
host_scan_cache_->RemoveHostScanResult(tether_network_guid);
expected_cache_->RemoveHostScanResult(tether_network_guid);
}
void ClearCacheExceptForActiveHost() {
host_scan_cache_->ClearCacheExceptForActiveHost();
expected_cache_->ClearCacheExceptForActiveHost();
if (fake_active_host_->GetTetherNetworkGuid() != tether_network_guid)
expected_cache_->RemoveHostScanResult(tether_network_guid);
}
// Verifies that the information present in |expected_cache_| mirrors what
......@@ -183,6 +176,8 @@ class MasterHostScanCacheTest : public testing::Test {
EXPECT_EQ(expected_size, expected_cache_->size());
EXPECT_EQ(expected_size, fake_network_host_scan_cache_->size());
EXPECT_EQ(expected_size, fake_persistent_host_scan_cache_->size());
EXPECT_EQ(expected_cache_->GetTetherGuidsInCache(),
host_scan_cache_->GetTetherGuidsInCache());
for (auto& it : expected_cache_->cache()) {
const std::string tether_network_guid = it.first;
......@@ -286,23 +281,13 @@ TEST_F(MasterHostScanCacheTest, TestSetScanResultThenUpdateAndRemove) {
VerifyCacheContainsExpectedContents(0 /* expected_size */);
}
TEST_F(MasterHostScanCacheTest, TestSetScanResult_SetActiveHost_ThenClear) {
TEST_F(MasterHostScanCacheTest, TestSetScanResult_SetActiveHost) {
SetHostScanResult(test_entries_.at(host_scan_test_util::kTetherGuid0));
VerifyCacheContainsExpectedContents(1u /* expected_size */);
SetHostScanResult(test_entries_.at(host_scan_test_util::kTetherGuid1));
VerifyCacheContainsExpectedContents(2u /* expected_size */);
SetHostScanResult(test_entries_.at(host_scan_test_util::kTetherGuid2));
VerifyCacheContainsExpectedContents(3u /* expected_size */);
// Now, set the active host to be the device 0.
SetActiveHost(host_scan_test_util::kTetherGuid0);
// Clear the cache except for the active host.
ClearCacheExceptForActiveHost();
VerifyCacheContainsExpectedContents(1u /* expected_size */);
// Attempt to remove the active host. This operation should fail since
// removing the active host from the cache is not allowed.
RemoveHostScanResult(host_scan_test_util::kTetherGuid0);
......
......@@ -6,6 +6,7 @@
#include "chromeos/components/tether/device_id_tether_network_guid_map.h"
#include "chromeos/components/tether/tether_host_response_recorder.h"
#include "chromeos/network/network_state.h"
#include "chromeos/network/network_state_handler.h"
#include "components/proximity_auth/logging/logging.h"
......@@ -65,10 +66,15 @@ bool NetworkHostScanCache::ExistsInCache(
nullptr;
}
void NetworkHostScanCache::ClearCacheExceptForActiveHost() {
// This function will be removed in a future CL, so it is not implemented
// here.
NOTIMPLEMENTED();
std::unordered_set<std::string> NetworkHostScanCache::GetTetherGuidsInCache() {
NetworkStateHandler::NetworkStateList tether_network_list;
network_state_handler_->GetVisibleNetworkListByType(
NetworkTypePattern::Tether(), &tether_network_list);
std::unordered_set<std::string> tether_guids;
for (auto* const network : tether_network_list)
tether_guids.insert(network->guid());
return tether_guids;
}
bool NetworkHostScanCache::DoesHostRequireSetup(
......
......@@ -6,7 +6,9 @@
#define CHROMEOS_COMPONENTS_TETHER_NETWORK_HOST_SCAN_CACHE_H_
#include <memory>
#include <string>
#include <unordered_map>
#include <unordered_set>
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
......@@ -37,7 +39,7 @@ class NetworkHostScanCache : public HostScanCache,
void SetHostScanResult(const HostScanCacheEntry& entry) override;
bool RemoveHostScanResult(const std::string& tether_network_guid) override;
bool ExistsInCache(const std::string& tether_network_guid) override;
void ClearCacheExceptForActiveHost() override;
std::unordered_set<std::string> GetTetherGuidsInCache() override;
bool DoesHostRequireSetup(const std::string& tether_network_guid) override;
// TetherHostResponseRecorder::Observer:
......
......@@ -100,6 +100,9 @@ class NetworkHostScanCacheTest : public NetworkStateTest {
// in NetworkStateHandler.
void VerifyCacheMatchesNetworkStack(size_t expected_size) {
EXPECT_EQ(expected_size, expected_cache_->size());
EXPECT_EQ(expected_cache_->GetTetherGuidsInCache(),
host_scan_cache_->GetTetherGuidsInCache());
for (auto& it : expected_cache_->cache()) {
const std::string tether_network_guid = it.first;
const HostScanCacheEntry& entry = it.second;
......
......@@ -172,10 +172,12 @@ bool PersistentHostScanCacheImpl::ExistsInCache(
return entries.find(tether_network_guid) != entries.end();
}
void PersistentHostScanCacheImpl::ClearCacheExceptForActiveHost() {
// This function will be removed in a future CL, so it is not implemented
// here.
NOTIMPLEMENTED();
std::unordered_set<std::string>
PersistentHostScanCacheImpl::GetTetherGuidsInCache() {
std::unordered_set<std::string> tether_guids;
for (const auto& entry : GetStoredCacheEntries())
tether_guids.insert(entry.first);
return tether_guids;
}
bool PersistentHostScanCacheImpl::DoesHostRequireSetup(
......
......@@ -5,7 +5,9 @@
#ifndef CHROMEOS_COMPONENTS_TETHER_PERSISTENT_HOST_SCAN_CACHE_IMPL_H_
#define CHROMEOS_COMPONENTS_TETHER_PERSISTENT_HOST_SCAN_CACHE_IMPL_H_
#include <string>
#include <unordered_map>
#include <unordered_set>
#include "base/macros.h"
#include "base/values.h"
......@@ -32,7 +34,7 @@ class PersistentHostScanCacheImpl : public PersistentHostScanCache {
void SetHostScanResult(const HostScanCacheEntry& entry) override;
bool RemoveHostScanResult(const std::string& tether_network_guid) override;
bool ExistsInCache(const std::string& tether_network_guid) override;
void ClearCacheExceptForActiveHost() override;
std::unordered_set<std::string> GetTetherGuidsInCache() override;
bool DoesHostRequireSetup(const std::string& tether_network_guid) override;
// PersistentHostScanCache:
......
......@@ -51,6 +51,8 @@ class PersistentHostScanCacheImplTest : public testing::Test {
EXPECT_EQ(expected_size, entries.size());
EXPECT_EQ(expected_size, expected_cache_->size());
EXPECT_EQ(expected_cache_->cache(), entries);
EXPECT_EQ(expected_cache_->GetTetherGuidsInCache(),
host_scan_cache_->GetTetherGuidsInCache());
}
const std::unordered_map<std::string, HostScanCacheEntry> test_entries_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment