Reland "[SendTabToSelf] Use new cache guid method to assert that local device...

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

This is a reland of 38392a67

In this reland, the old logic remains unchanged as a fallback mechanism
in case the GUID-based detection fails (as the reports suggest).

Original change's description:
> [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}

Bug: 966413
Change-Id: I85ae3c33c83f29f9a59cb8f3c2219e22e31a2bad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1828817
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Ramya Nagarajan <ramyan@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701073}

components/send_tab_to_self/send_tab_to_self_bridge.cc

@@ -712,6 +712,12 @@ 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();
@@ -734,10 +740,17 @@ 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 if it is the local device.
+    if (device_info_tracker_->IsRecentLocalCacheGuid(device->guid())) {
+      continue;
+    }
+
     // 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).
+    // TODO(crbug.com/991943): This shouldn't be required now that
+    // IsRecentLocalCacheGuid() is used above. However, issues have been
+    // observed on Android, as if IsRecentLocalCacheGuid() didn't get populated
+    // reliably.
     if (device->guid() == change_processor()->TrackedCacheGuid() ||
         device->client_name() == local_device_name_) {
       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(), /*local=*/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) {
@@ -86,4 +85,10 @@ void FakeDeviceInfoTracker::OverrideActiveDeviceCount(int count) {
     observer.OnDeviceInfoChange();
 }
 
+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

@@ -42,9 +42,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_;
   // Registered observers, not owned.
   base::ObserverList<Observer, true>::Unchecked observers_;