Commit 4d6b9b15 authored by Josh Nohle's avatar Josh Nohle Committed by Commit Bot

[Nearby] Filter out unreachable contacts

In cl/339905938, the Nearby server started returning both reachable
and unreachable contacts in the ListContactPeople RPC response. The
ContactRecord proto message now has a field denoting the contact's
reachability.

In this CL, we filter out unreachable contacts, counting the number
of removed unreachable contacts. This count is communicated to
observers. It may be useful to inform the user in the contacts UI that
a certain number of contacts are not being shown because they are not
associated with a GAIA account, and that Nearby Share requires this
association. This information is already present in the Android Nearby
Share UI.

Note: This CL should not be submitted until cl/339905938 reaches prod;
otherwise, all contacts will be filtered out due to the default-false
|is_reachable| field. After cl/339905938 hits prod but before this CL
lands, unreachable contacts will not be filtered out.

Bug: 1143837
Change-Id: I439469e02e9694e5853c807928dd687eaf7f3f69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2514176Reviewed-by: default avatarJames Vecore <vecore@google.com>
Reviewed-by: default avatarAlex Gough <ajgo@chromium.org>
Commit-Queue: Josh Nohle <nohle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823749}
parent fc86c958
......@@ -338,7 +338,8 @@ void NearbyShareCertificateManagerImpl::UpdatePrivateCertificateInStorage(
void NearbyShareCertificateManagerImpl::OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) {}
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) {}
void NearbyShareCertificateManagerImpl::OnContactsUploaded(
bool did_contacts_change_since_last_upload) {
......
......@@ -117,7 +117,8 @@ class NearbyShareCertificateManagerImpl
// NearbyShareContactManager::Observer:
void OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) override;
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) override;
void OnContactsUploaded(bool did_contacts_change_since_last_upload) override;
// NearbyShareLocalDeviceDataManager::Observer:
......
......@@ -40,12 +40,14 @@ void NearbyShareContactDownloader::Run() {
}
void NearbyShareContactDownloader::Succeed(
std::vector<nearbyshare::proto::ContactRecord> contacts) {
std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out) {
DCHECK(was_run_);
DCHECK(success_callback_);
RecordSuccessMetrics(contacts);
std::move(success_callback_).Run(std::move(contacts));
std::move(success_callback_)
.Run(std::move(contacts), num_unreachable_contacts_filtered_out);
}
void NearbyShareContactDownloader::Fail() {
......
......@@ -18,7 +18,8 @@
class NearbyShareContactDownloader {
public:
using SuccessCallback = base::OnceCallback<void(
std::vector<nearbyshare::proto::ContactRecord> contacts)>;
std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out)>;
using FailureCallback = base::OnceClosure;
// |device_id|: The ID used by the Nearby server to differentiate multiple
......@@ -41,7 +42,8 @@ class NearbyShareContactDownloader {
virtual void OnRun() = 0;
// Invokes the success callback with the input parameters.
void Succeed(std::vector<nearbyshare::proto::ContactRecord> contacts);
void Succeed(std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out);
// Invokes the failure callback.
void Fail();
......
......@@ -130,7 +130,9 @@ void NearbyShareContactDownloaderImpl::OnListContactPeopleSuccess(
<< " contacts succeeded.";
RecordContactDownloadMetrics(contacts_, current_page_number_);
// Remove device contacts if the feature flag is disabled.
if (!base::FeatureList::IsEnabled(features::kNearbySharingDeviceContacts)) {
size_t initial_num_contacts = contacts_.size();
contacts_.erase(
std::remove_if(
contacts_.begin(), contacts_.end(),
......@@ -139,9 +141,26 @@ void NearbyShareContactDownloaderImpl::OnListContactPeopleSuccess(
nearbyshare::proto::ContactRecord::DEVICE_CONTACT;
}),
contacts_.end());
NS_LOG(VERBOSE) << __func__ << ": Removed "
<< initial_num_contacts - contacts_.size()
<< " device contacts.";
}
Succeed(std::move(contacts_));
// Remove unreachable contacts.
size_t initial_num_contacts = contacts_.size();
contacts_.erase(
std::remove_if(contacts_.begin(), contacts_.end(),
[](const nearbyshare::proto::ContactRecord& contact) {
return !contact.is_reachable();
}),
contacts_.end());
uint32_t num_unreachable_contacts_filtered_out =
initial_num_contacts - contacts_.size();
NS_LOG(VERBOSE) << __func__ << ": Removed "
<< num_unreachable_contacts_filtered_out
<< " unreachable contacts.";
Succeed(std::move(contacts_), num_unreachable_contacts_filtered_out);
}
void NearbyShareContactDownloaderImpl::OnListContactPeopleFailure(
......
......@@ -27,6 +27,8 @@ const char kTestDeviceId[] = "test_device_id";
const char kTestContactRecordId1[] = "contact_id_1";
const char kTestContactRecordId2[] = "contact_id_2";
const char kTestContactRecordId3[] = "contact_id_3";
const char kTestContactRecordId4[] = "contact_id_4";
const char kTestContactRecordId5[] = "contact_id_5";
const char kTestPageToken[] = "token";
constexpr base::TimeDelta kTestTimeout = base::TimeDelta::FromMinutes(123);
......@@ -38,14 +40,25 @@ const std::vector<nearbyshare::proto::ContactRecord>& TestContactRecordList() {
nearbyshare::proto::ContactRecord contact1;
contact1.set_id(kTestContactRecordId1);
contact1.set_type(nearbyshare::proto::ContactRecord::GOOGLE_CONTACT);
contact1.set_is_reachable(true);
nearbyshare::proto::ContactRecord contact2;
contact2.set_id(kTestContactRecordId2);
contact2.set_type(nearbyshare::proto::ContactRecord::DEVICE_CONTACT);
contact2.set_is_reachable(true);
nearbyshare::proto::ContactRecord contact3;
contact3.set_id(kTestContactRecordId3);
contact3.set_type(nearbyshare::proto::ContactRecord::UNKNOWN);
contact3.set_is_reachable(true);
nearbyshare::proto::ContactRecord contact4;
contact4.set_id(kTestContactRecordId4);
contact4.set_type(nearbyshare::proto::ContactRecord::GOOGLE_CONTACT);
contact4.set_is_reachable(false);
nearbyshare::proto::ContactRecord contact5;
contact5.set_id(kTestContactRecordId5);
contact5.set_type(nearbyshare::proto::ContactRecord::GOOGLE_CONTACT);
contact5.set_is_reachable(false);
return std::vector<nearbyshare::proto::ContactRecord>{
contact1, contact2, contact3};
contact1, contact2, contact3, contact4, contact5};
}());
return *list;
}
......@@ -70,6 +83,7 @@ class NearbyShareContactDownloaderImplTest
struct Result {
bool success;
base::Optional<std::vector<nearbyshare::proto::ContactRecord>> contacts;
base::Optional<uint32_t> num_unreachable_contacts_filtered_out;
};
NearbyShareContactDownloaderImplTest() = default;
......@@ -114,10 +128,13 @@ class NearbyShareContactDownloaderImplTest
}
void VerifySuccess(
const std::vector<nearbyshare::proto::ContactRecord>& expected_contacts) {
const std::vector<nearbyshare::proto::ContactRecord>& expected_contacts,
uint32_t expected_num_unreachable_contacts_filtered_out) {
ASSERT_TRUE(result_);
EXPECT_TRUE(result_->success);
ASSERT_TRUE(result_->contacts);
EXPECT_EQ(expected_num_unreachable_contacts_filtered_out,
result_->num_unreachable_contacts_filtered_out);
ASSERT_EQ(expected_contacts.size(), result_->contacts->size());
for (size_t i = 0; i < expected_contacts.size(); ++i) {
......@@ -129,6 +146,8 @@ class NearbyShareContactDownloaderImplTest
void VerifyFailure() {
ASSERT_TRUE(result_);
EXPECT_FALSE(result_->success);
EXPECT_FALSE(result_->contacts);
EXPECT_FALSE(result_->num_unreachable_contacts_filtered_out);
}
private:
......@@ -150,14 +169,19 @@ class NearbyShareContactDownloaderImplTest
}
// The callbacks passed into NearbyShareContactDownloader ctor.
void OnSuccess(std::vector<nearbyshare::proto::ContactRecord> contacts) {
void OnSuccess(std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out) {
result_ = Result();
result_->success = true;
result_->contacts = std::move(contacts);
result_->num_unreachable_contacts_filtered_out =
num_unreachable_contacts_filtered_out;
}
void OnFailure() {
result_ = Result();
result_->success = false;
result_->contacts.reset();
result_->num_unreachable_contacts_filtered_out.reset();
}
base::test::SingleThreadTaskEnvironment task_environment_{
......@@ -186,7 +210,12 @@ TEST_F(NearbyShareContactDownloaderImplTest, Success) {
TestContactRecordList().end()),
/*next_page_token=*/base::nullopt));
VerifySuccess(/*expected_contacts=*/TestContactRecordList());
// The last two records are filtered out because the are not reachable.
VerifySuccess(/*expected_contacts=*/
std::vector<nearbyshare::proto::ContactRecord>(
TestContactRecordList().begin(),
TestContactRecordList().begin() + 3),
/*expected_num_unreachable_contacts_filtered_out=*/2);
}
TEST_F(NearbyShareContactDownloaderImplTest, Failure_ListContactPeople) {
......@@ -240,5 +269,6 @@ TEST_F(NearbyShareContactDownloaderImplTest, Success_FilterOutDeviceContacts) {
// The device contact is filtered out.
VerifySuccess(/*expected_contacts=*/{TestContactRecordList()[0],
TestContactRecordList()[2]});
TestContactRecordList()[2]},
/*expected_num_unreachable_contacts_filtered_out=*/2);
}
......@@ -42,9 +42,11 @@ void NearbyShareContactManager::SetAllowedContacts(
void NearbyShareContactManager::NotifyContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) {
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) {
for (auto& observer : observers_) {
observer.OnContactsDownloaded(allowed_contact_ids, contacts);
observer.OnContactsDownloaded(allowed_contact_ids, contacts,
num_unreachable_contacts_filtered_out);
}
}
......
......@@ -38,7 +38,8 @@ class NearbyShareContactManager : public nearby_share::mojom::ContactManager {
public:
virtual void OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) = 0;
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) = 0;
virtual void OnContactsUploaded(
bool did_contacts_change_since_last_upload) = 0;
};
......@@ -87,7 +88,8 @@ class NearbyShareContactManager : public nearby_share::mojom::ContactManager {
void NotifyContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts);
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out);
void NotifyContactsUploaded(bool did_contacts_change_since_last_upload);
private:
......
......@@ -249,7 +249,8 @@ void NearbyShareContactManagerImpl::OnContactsDownloadRequested() {
}
void NearbyShareContactManagerImpl::OnContactsDownloadSuccess(
std::vector<nearbyshare::proto::ContactRecord> contacts) {
std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out) {
contact_downloader_.reset();
NS_LOG(VERBOSE) << __func__ << ": Nearby Share download of "
......@@ -261,8 +262,10 @@ void NearbyShareContactManagerImpl::OnContactsDownloadSuccess(
// Notify observers that the contact list was downloaded.
std::set<std::string> allowed_contact_ids = GetAllowedContacts();
NotifyContactsDownloaded(allowed_contact_ids, contacts);
NotifyMojoObserverContactsDownloaded(allowed_contact_ids, contacts);
NotifyContactsDownloaded(allowed_contact_ids, contacts,
num_unreachable_contacts_filtered_out);
NotifyMojoObserverContactsDownloaded(allowed_contact_ids, contacts,
num_unreachable_contacts_filtered_out);
std::vector<nearbyshare::proto::Contact> contacts_to_upload =
ContactRecordsToContacts(GetAllowedContacts(), contacts);
......@@ -341,7 +344,8 @@ bool NearbyShareContactManagerImpl::SetAllowlist(
void NearbyShareContactManagerImpl::NotifyMojoObserverContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) {
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) {
if (observers_set_.empty()) {
return;
}
......@@ -353,6 +357,7 @@ void NearbyShareContactManagerImpl::NotifyMojoObserverContactsDownloaded(
// Notify mojo remotes.
for (auto& remote : observers_set_) {
remote->OnContactsDownloaded(allowed_contact_ids_vector,
ProtoToMojo(contacts));
ProtoToMojo(contacts),
num_unreachable_contacts_filtered_out);
}
}
......@@ -89,14 +89,16 @@ class NearbyShareContactManagerImpl : public NearbyShareContactManager {
std::set<std::string> GetAllowedContacts() const;
void OnContactsDownloadRequested();
void OnContactsDownloadSuccess(
std::vector<nearbyshare::proto::ContactRecord> contacts);
std::vector<nearbyshare::proto::ContactRecord> contacts,
uint32_t num_unreachable_contacts_filtered_out);
void OnContactsDownloadFailure();
void OnContactsUploadFinished(const std::string& contact_upload_hash,
bool success);
bool SetAllowlist(const std::set<std::string>& new_allowlist);
void NotifyMojoObserverContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts);
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out);
PrefService* pref_service_ = nullptr;
NearbyShareClientFactory* http_client_factory_ = nullptr;
......
......@@ -30,6 +30,7 @@
namespace {
const uint32_t kTestNumUnreachableContactsFilteredOut = 123;
const char kTestContactIdPrefix[] = "id_";
const char kTestContactEmailPrefix[] = "email_";
const char kTestContactPhonePrefix[] = "phone_";
......@@ -68,6 +69,7 @@ std::vector<nearbyshare::proto::ContactRecord> TestContactRecordList(
contact.set_id(GetTestContactId(i));
contact.set_image_url("https://google.com");
contact.set_person_name("John Doe");
contact.set_is_reachable(true);
// only one of these fields should be set...
switch ((i % 3)) {
case 0:
......@@ -117,9 +119,12 @@ class TestDownloadContactsObserver
public:
void OnContactsDownloaded(
const std::vector<std::string>& allowed_contacts,
std::vector<nearby_share::mojom::ContactRecordPtr> contacts) override {
std::vector<nearby_share::mojom::ContactRecordPtr> contacts,
uint32_t num_unreachable_contacts_filtered_out) override {
allowed_contacts_ = allowed_contacts;
contacts_ = std::move(contacts);
num_unreachable_contacts_filtered_out_ =
num_unreachable_contacts_filtered_out;
on_contacts_downloaded_called_ = true;
}
......@@ -129,6 +134,7 @@ class TestDownloadContactsObserver
std::vector<std::string> allowed_contacts_;
std::vector<nearby_share::mojom::ContactRecordPtr> contacts_;
uint32_t num_unreachable_contacts_filtered_out_;
bool on_contacts_downloaded_called_ = false;
bool on_contacts_download_failed_called_ = false;
mojo::Receiver<nearby_share::mojom::DownloadContactsObserver> receiver_{this};
......@@ -147,6 +153,7 @@ class NearbyShareContactManagerImplTest
struct ContactsDownloadedNotification {
std::set<std::string> allowed_contact_ids;
std::vector<nearbyshare::proto::ContactRecord> contacts;
uint32_t num_unreachable_contacts_filtered_out;
};
struct ContactsUploadedNotification {
bool did_contacts_change_since_last_upload;
......@@ -209,7 +216,8 @@ class NearbyShareContactManagerImplTest
size_t num_upload_contacts_calls =
local_device_data_manager_.upload_contacts_calls().size();
latest_downloader()->Succeed(contacts);
latest_downloader()->Succeed(contacts,
kTestNumUnreachableContactsFilteredOut);
VerifyDownloadNotificationSent(
/*initial_num_notifications=*/num_download_notifications,
......@@ -304,7 +312,8 @@ class NearbyShareContactManagerImplTest
// NearbyShareContactManager::Observer:
void OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) override {
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) override {
ContactsDownloadedNotification notification;
notification.allowed_contact_ids = allowed_contact_ids;
notification.contacts = contacts;
......
......@@ -80,7 +80,7 @@ message Contact {
}
// A contact record from People backend.
// NextId=6
// NextId=7
message ContactRecord {
// The type of the ContactRecord.
enum Type {
......@@ -109,6 +109,9 @@ message ContactRecord {
// The type of the ContactRecord.
Type type = 5;
// True if the contact record is WPS reachable.
bool is_reachable = 6;
}
// A ShareTarget is a potential destination of a share.
......
......@@ -32,6 +32,8 @@ const char kContactMessageTimeKey[] = "time";
const char kContactMessageContactsChangedKey[] = "contactsChanged";
const char kContactMessageAllowedIdsKey[] = "allowedIds";
const char kContactMessageContactRecordKey[] = "contactRecords";
const char kContactMessageNumUnreachableContactsKey[] =
"numUnreachableContacts";
// Converts Contact to a raw dictionary value used as a JSON argument to
// JavaScript functions.
......@@ -42,7 +44,8 @@ base::Value ContactMessageToDictionary(
base::Optional<bool> did_contacts_change_since_last_upload,
const base::Optional<std::set<std::string>>& allowed_contact_ids,
const base::Optional<std::vector<nearbyshare::proto::ContactRecord>>&
contacts) {
contacts,
base::Optional<uint32_t> num_unreachable_contacts_filtered_out) {
base::Value dictionary(base::Value::Type::DICTIONARY);
dictionary.SetKey(kContactMessageTimeKey, GetJavascriptTimestamp());
......@@ -70,6 +73,10 @@ base::Value ContactMessageToDictionary(
dictionary.SetStringKey(kContactMessageContactRecordKey,
FormatAsJSON(base::Value(std::move(contact_list))));
}
if (num_unreachable_contacts_filtered_out.has_value()) {
dictionary.SetIntKey(kContactMessageNumUnreachableContactsKey,
*num_unreachable_contacts_filtered_out);
}
return dictionary;
}
......@@ -125,18 +132,22 @@ void NearbyInternalsContactHandler::HandleDownloadContacts(
void NearbyInternalsContactHandler::OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) {
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) {
FireWebUIListener("contacts-updated",
ContactMessageToDictionary(
/*did_contacts_change_since_last_upload=*/base::nullopt,
allowed_contact_ids, contacts));
allowed_contact_ids, contacts,
num_unreachable_contacts_filtered_out));
}
void NearbyInternalsContactHandler::OnContactsUploaded(
bool did_contacts_change_since_last_upload) {
FireWebUIListener(
"contacts-updated",
ContactMessageToDictionary(did_contacts_change_since_last_upload,
ContactMessageToDictionary(
did_contacts_change_since_last_upload,
/*allowed_contact_ids=*/base::nullopt,
/*contacts=*/base::nullopt));
/*contacts=*/base::nullopt,
/*num_unreachable_contacts_filtered_out=*/base::nullopt));
}
......@@ -48,7 +48,8 @@ class NearbyInternalsContactHandler
// NearbyShareContactManager::Observer:
void OnContactsDownloaded(
const std::set<std::string>& allowed_contact_ids,
const std::vector<nearbyshare::proto::ContactRecord>& contacts) override;
const std::vector<nearbyshare::proto::ContactRecord>& contacts,
uint32_t num_unreachable_contacts_filtered_out) override;
void OnContactsUploaded(bool did_contacts_change_since_last_upload) override;
// Message handler callback that requests a contacts download from the contact
......
......@@ -146,7 +146,8 @@ interface DownloadContactsObserver {
// array may be empty if the user has no contacts (in which case
// |allowed_contacts| will also be empty).
OnContactsDownloaded(array<string> allowed_contacts,
array<ContactRecord> contacts);
array<ContactRecord> contacts,
uint32 num_unreachable_contacts_filtered_out);
// Notifies the observer that contacts have failed to download.
// NOTE: At the moment we don't provide a specific reason as to why this
......
......@@ -73,7 +73,8 @@ cr.define('nearby_share', function() {
completeDownload() {
this.observer_.onContactsDownloaded(
this.allowedContacts_, this.contactRecords || []);
this.allowedContacts_, this.contactRecords || [],
/*num_unreachable_contacts_filtered_out=*/ 3);
}
}
// #cr_define_end
......
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