[SendTabToSelf] Use new cache guid method to assert that local device is not listed as a target

Bug: 966413
Change-Id: I29fe887806b07a51d7490603aaf81415a5a349cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1721379
Commit-Queue: Jeffrey Cohen <jeffreycohen@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682341}

components/send_tab_to_self/send_tab_to_self_bridge.cc

@@ -49,7 +49,7 @@ enum class UMAAddEntryStatus {
 // These values are persisted to logs. Entries should not be renumbered and
 // numeric values should never be reused.
 enum class UMANotifyLocalDevice {
-  // The addedentry was sent to the local machine.
+  // The added entry was sent to the local machine.
   LOCAL = 0,
   // The added entry was targeted at a different machine.
   REMOTE = 1,
@@ -712,11 +712,17 @@ bool SendTabToSelfBridge::ShouldUpdateTargetDeviceInfoList() const {
 }
 
 void SendTabToSelfBridge::SetTargetDeviceInfoList() {
+  // Verify that the current trackedCacheGuid is the local guid without
+  // enforcing that tracked cache guid is set.
+  DCHECK(device_info_tracker_->IsRecentLocalCacheGuid(
+             change_processor()->TrackedCacheGuid()) ||
+         change_processor()->TrackedCacheGuid().empty());
+
   std::vector<std::unique_ptr<syncer::DeviceInfo>> all_devices =
       device_info_tracker_->GetAllDeviceInfo();
   number_of_devices_ = all_devices.size();
 
-  // Sort the DeviceInfo vector so the most recenly modified devices are first.
+  // Sort the DeviceInfo vector so the most recently modified devices are first.
   std::stable_sort(all_devices.begin(), all_devices.end(),
                    [](const std::unique_ptr<syncer::DeviceInfo>& device1,
                       const std::unique_ptr<syncer::DeviceInfo>& device2) {
@@ -734,12 +740,8 @@ void SendTabToSelfBridge::SetTargetDeviceInfoList() {
       break;
     }
 
-    // TODO(crbug.com/966413): Implement a better way to dedupe local devices in
-    // case the user has other devices with the same name.
-    // Don't include this device. Also compare the name as the device can have
-    // different cache guids (e.g. after stopping and re-starting sync).
-    if (device->guid() == change_processor()->TrackedCacheGuid() ||
-        device->client_name() == local_device_name_) {
+    // Don't include this device if it is the local device.
+    if (device_info_tracker_->IsRecentLocalCacheGuid(device->guid())) {
       continue;
     }
 

components/send_tab_to_self/send_tab_to_self_bridge_unittest.cc

@@ -99,7 +99,7 @@ class SendTabToSelfBridgeTest : public testing::Test {
         sync_pb::SyncEnums_DeviceType_TYPE_LINUX, "scoped_is",
         clock()->Now() - base::TimeDelta::FromDays(1),
         /*send_tab_to_self_receiving_enabled=*/true);
-    AddTestDevice(local_device_.get());
+    AddTestDevice(local_device_.get(), true);
   }
 
   // Initialized the bridge based on the current local device and store. Can
@@ -179,8 +179,11 @@ class SendTabToSelfBridgeTest : public testing::Test {
         .WillByDefault(Return(cache_guid));
   }
 
-  void AddTestDevice(const syncer::DeviceInfo* device) {
+  void AddTestDevice(const syncer::DeviceInfo* device, bool local = false) {
     device_info_tracker_.Add(device);
+    if (local) {
+      device_info_tracker_.SetLocalCacheGuid(device->guid());
+    }
   }
 
   syncer::MockModelTypeChangeProcessor* processor() { return &mock_processor_; }

components/sync_device_info/fake_device_info_tracker.cc

@@ -34,7 +34,11 @@ bool FakeDeviceInfoTracker::IsSyncing() const {
 
 std::unique_ptr<DeviceInfo> FakeDeviceInfoTracker::GetDeviceInfo(
     const std::string& client_id) const {
-  NOTREACHED();
+  for (const DeviceInfo* device : devices_) {
+    if (device->guid() == client_id) {
+      return CloneDeviceInfo(*device);
+    }
+  }
   return nullptr;
 }
 
@@ -66,12 +70,7 @@ void FakeDeviceInfoTracker::ForcePulseForTest() {
 
 bool FakeDeviceInfoTracker::IsRecentLocalCacheGuid(
     const std::string& cache_guid) const {
-  for (const DeviceInfo* device : devices_) {
-    if (device->guid() == cache_guid) {
-      return true;
-    }
-  }
-  return false;
+  return (local_device_cache_guid_ == cache_guid);
 }
 
 void FakeDeviceInfoTracker::Add(const DeviceInfo* device) {
@@ -82,4 +81,10 @@ void FakeDeviceInfoTracker::OverrideActiveDeviceCount(int count) {
   active_device_count_ = count;
 }
 
+void FakeDeviceInfoTracker::SetLocalCacheGuid(const std::string& cache_guid) {
+  // ensure that this cache guid is present in the tracker.
+  DCHECK(GetDeviceInfo(cache_guid));
+  local_device_cache_guid_ = cache_guid;
+}
+
 }  // namespace syncer

components/sync_device_info/fake_device_info_tracker.h

@@ -41,9 +41,13 @@ class FakeDeviceInfoTracker : public DeviceInfoTracker {
   // actual number of devices in |devices_|.
   void OverrideActiveDeviceCount(int count);
 
+  // Marks an existing DeviceInfo entry as being on the local device.
+  void SetLocalCacheGuid(const std::string& cache_guid);
+
  private:
   // DeviceInfo stored here are not owned.
   std::vector<const DeviceInfo*> devices_;
+  std::string local_device_cache_guid_;
   base::Optional<int> active_device_count_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeDeviceInfoTracker);