[DeviceSync v2] Check if CryptAuthDeviceRegistry changes

The previously void methods AddDevice(), DeleteDevice(), and
SetRegistry() now return a boolean, indicating whether or not the
registry changed as a result of the call. Also, the pref will now only
be updated when the registry changes. And, DeleteDevice() no longer
DCHECKs that a device with the input |instance_id| is in the registry,
instead returning false if such a device doesn't exist.

The RemoteDeviceProvider needs to know when a DeviceSync updates the
local device cache. The aforementioned changes to
CryptAuthDeviceRegistry will help make that determination for v2
DeviceSyncs.

Bug: 951969
Change-Id: I6ad98cadc659d02895da3b16eed90b0ba50773bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1638386
Commit-Queue: Josh Nohle <nohle@chromium.org>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667227}

chromeos/services/device_sync/cryptauth_device_registry.cc

@@ -28,25 +28,37 @@ const CryptAuthDevice* CryptAuthDeviceRegistry::GetDevice(
   return &it->second;
 }
 
-void CryptAuthDeviceRegistry::AddDevice(const CryptAuthDevice& device) {
+bool CryptAuthDeviceRegistry::AddDevice(const CryptAuthDevice& device) {
+  const CryptAuthDevice* existing_device = GetDevice(device.instance_id());
+  if (existing_device && device == *existing_device)
+    return false;
+
   instance_id_to_device_map_.insert_or_assign(device.instance_id(), device);
 
   OnDeviceRegistryUpdated();
+  return true;
 }
 
-void CryptAuthDeviceRegistry::DeleteDevice(const std::string& instance_id) {
-  DCHECK(base::Contains(instance_id_to_device_map_, instance_id));
+bool CryptAuthDeviceRegistry::DeleteDevice(const std::string& instance_id) {
+  if (!base::Contains(instance_id_to_device_map_, instance_id))
+    return false;
+
   instance_id_to_device_map_.erase(instance_id);
 
   OnDeviceRegistryUpdated();
+  return true;
 }
 
-void CryptAuthDeviceRegistry::SetRegistry(
+bool CryptAuthDeviceRegistry::SetRegistry(
     const base::flat_map<std::string, CryptAuthDevice>&
         instance_id_to_device_map) {
+  if (instance_id_to_device_map_ == instance_id_to_device_map)
+    return false;
+
   instance_id_to_device_map_ = instance_id_to_device_map;
 
   OnDeviceRegistryUpdated();
+  return true;
 }
 
 }  // namespace device_sync

chromeos/services/device_sync/cryptauth_device_registry.h

@@ -30,13 +30,16 @@ class CryptAuthDeviceRegistry {
 
   // Adds |device| to the registry. If a device with the same Instance ID
   // already exists in the registry, the existing device will be overwritten.
-  void AddDevice(const CryptAuthDevice& device);
+  // Returns true if the registry changes.
+  bool AddDevice(const CryptAuthDevice& device);
 
-  // Removes the device with corresponding |instance_id|.
-  void DeleteDevice(const std::string& instance_id);
+  // Removes the device with corresponding |instance_id|.  Returns true if the
+  // registry changes.
+  bool DeleteDevice(const std::string& instance_id);
 
-  // Replaces the entire registry with |instance_id_to_device_map|.
-  void SetRegistry(const base::flat_map<std::string, CryptAuthDevice>&
+  // Replaces the entire registry with |instance_id_to_device_map|. Returns true
+  // if the registry changes.
+  bool SetRegistry(const base::flat_map<std::string, CryptAuthDevice>&
                        instance_id_to_device_map);
 
  protected:

chromeos/services/device_sync/cryptauth_device_registry_impl_unittest.cc

@@ -122,8 +122,9 @@ class DeviceSyncCryptAuthDeviceRegistryImplTest : public testing::Test {
 TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, AddAndGetDevices) {
   CreateDeviceRegistry();
 
-  device_registry()->AddDevice(GetDeviceForTest(0));
-  device_registry()->AddDevice(GetDeviceForTest(1));
+  EXPECT_TRUE(device_registry()->AddDevice(GetDeviceForTest(0)));
+  EXPECT_FALSE(device_registry()->AddDevice(GetDeviceForTest(0)));
+  EXPECT_TRUE(device_registry()->AddDevice(GetDeviceForTest(1)));
 
   VerifyDeviceRegistry(
       {{kInstanceId0, GetDeviceForTest(0)},
@@ -135,13 +136,13 @@ TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, AddAndGetDevices) {
 TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, OverwriteDevice) {
   CreateDeviceRegistry();
 
-  device_registry()->AddDevice(GetDeviceForTest(0));
+  EXPECT_TRUE(device_registry()->AddDevice(GetDeviceForTest(0)));
   EXPECT_EQ(GetDeviceForTest(0), *device_registry()->GetDevice(kInstanceId0));
 
   CryptAuthDevice device_with_same_instance_id(
       kInstanceId0, "name", "key", base::Time::FromDoubleT(5000),
       cryptauthv2::BetterTogetherDeviceMetadata(), {});
-  device_registry()->AddDevice(device_with_same_instance_id);
+  EXPECT_TRUE(device_registry()->AddDevice(device_with_same_instance_id));
   EXPECT_EQ(device_with_same_instance_id,
             *device_registry()->GetDevice(kInstanceId0));
 }
@@ -151,25 +152,27 @@ TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, OverwriteRegistry) {
 
   base::flat_map<std::string, CryptAuthDevice> old_devices = {
       {kInstanceId0, GetDeviceForTest(0)}};
-  device_registry()->SetRegistry(old_devices);
+  EXPECT_TRUE(device_registry()->SetRegistry(old_devices));
   VerifyDeviceRegistry(old_devices);
+  EXPECT_FALSE(device_registry()->SetRegistry(old_devices));
 
   base::flat_map<std::string, CryptAuthDevice> new_devices = {
       {kInstanceId1, GetDeviceForTest(1)}};
-  device_registry()->SetRegistry(new_devices);
+  EXPECT_TRUE(device_registry()->SetRegistry(new_devices));
   VerifyDeviceRegistry(new_devices);
 }
 
 TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, DeleteDevice) {
   CreateDeviceRegistry();
 
-  device_registry()->AddDevice(GetDeviceForTest(0));
+  EXPECT_TRUE(device_registry()->AddDevice(GetDeviceForTest(0)));
   VerifyDeviceRegistry(
       {{kInstanceId0, GetDeviceForTest(0)}} /* expected_devices */);
 
-  device_registry()->DeleteDevice(kInstanceId0);
+  EXPECT_TRUE(device_registry()->DeleteDevice(kInstanceId0));
   EXPECT_FALSE(device_registry()->GetDevice(kInstanceId0));
   VerifyDeviceRegistry({} /* expected_devices */);
+  EXPECT_FALSE(device_registry()->DeleteDevice(kInstanceId0));
 }
 
 TEST_F(DeviceSyncCryptAuthDeviceRegistryImplTest, PopulateRegistryFromPref) {