Update client name in OnSyncStarting

SyncMode is not available when GetDeviceName is called. So
we have to manually update the device name and merge it with
sync data.

Bug: 1009454
Change-Id: Icf0760eabf41171b9d7f222b620b0c41bc32bc59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866651
Commit-Queue: Himanshu Jaju <himanshujaju@chromium.org>
Reviewed-by: Alex Chau <alexchau@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707806}

base/system/sys_info.h

@@ -83,6 +83,11 @@ class BASE_EXPORT SysInfo {
     // Note: validate any new usage with the privacy team.
     // TODO(crbug.com/907518): Implement support on other platforms.
     std::string serial_number;
+
+    bool operator==(const HardwareInfo& rhs) const {
+      return manufacturer == rhs.manufacturer && model == rhs.model &&
+             serial_number == rhs.serial_number;
+    }
   };
   // Returns via |callback| a struct containing descriptive UTF-8 strings for
   // the current machine manufacturer and model, or empty strings if the

components/sync_device_info/device_info.cc

@@ -52,7 +52,10 @@ DeviceInfo::DeviceInfo(const std::string& guid,
       hardware_info_(hardware_info),
       last_updated_timestamp_(last_updated_timestamp),
       send_tab_to_self_receiving_enabled_(send_tab_to_self_receiving_enabled),
-      sharing_info_(sharing_info) {}
+      sharing_info_(sharing_info) {
+  // We do not store device's serial number in DeviceInfo.
+  hardware_info_.serial_number.clear();
+}
 
 DeviceInfo::~DeviceInfo() {}
 
@@ -144,6 +147,7 @@ bool DeviceInfo::Equals(const DeviceInfo& other) const {
          this->sync_user_agent() == other.sync_user_agent() &&
          this->device_type() == other.device_type() &&
          this->signin_scoped_device_id() == other.signin_scoped_device_id() &&
+         this->hardware_info() == other.hardware_info() &&
          this->send_tab_to_self_receiving_enabled() ==
              other.send_tab_to_self_receiving_enabled() &&
          this->sharing_info() == other.sharing_info();
@@ -176,4 +180,8 @@ void DeviceInfo::set_sharing_info(
   sharing_info_ = sharing_info;
 }
 
+void DeviceInfo::set_client_name(const std::string& client_name) {
+  client_name_ = client_name;
+}
+
 }  // namespace syncer

components/sync_device_info/device_info.h

@@ -120,6 +120,8 @@ class DeviceInfo {
 
   void set_sharing_info(const base::Optional<SharingInfo>& sharing_info);
 
+  void set_client_name(const std::string& client_name);
+
   // Converts the |DeviceInfo| values to a JS friendly DictionaryValue,
   // which extension APIs can expose to third party apps.
   std::unique_ptr<base::DictionaryValue> ToValue();
@@ -127,7 +129,7 @@ class DeviceInfo {
  private:
   const std::string guid_;
 
-  const std::string client_name_;
+  std::string client_name_;
 
   const std::string chrome_version_;
 

components/sync_device_info/device_info_sync_bridge.cc

@@ -204,6 +204,16 @@ void DeviceInfoSyncBridge::OnSyncStarting(
   device_info_prefs_->AddLocalCacheGuid(local_cache_guid_);
   // SyncMode determines the client name in GetLocalClientName().
   sync_mode_ = request.sync_mode;
+
+  if (!change_processor()->IsTrackingMetadata()) {
+    return;
+  }
+
+  // Local device's client name needs to updated if OnSyncStarting is called
+  // after local device has already been initialized since the client name
+  // depends on |sync_mode_|.
+  local_device_info_provider_->UpdateClientName(GetLocalClientName());
+  ReconcileLocalAndStored();
 }
 
 std::unique_ptr<MetadataChangeList>

components/sync_device_info/device_info_sync_bridge_unittest.cc

@@ -213,41 +213,6 @@ DeviceInfoSpecifics CreateSpecifics(
   return specifics;
 }
 
-DeviceInfoSpecifics DeviceInfoToSpecifics(const DeviceInfo& info) {
-  auto hardware_info = info.hardware_info();
-  DeviceInfoSpecifics specifics;
-  specifics.set_cache_guid(info.guid());
-  specifics.set_client_name(info.client_name());
-  specifics.set_chrome_version(info.chrome_version());
-  specifics.set_sync_user_agent(info.sync_user_agent());
-  specifics.set_device_type(info.device_type());
-  specifics.set_signin_scoped_device_id(info.signin_scoped_device_id());
-  specifics.set_model(hardware_info.model);
-  specifics.set_manufacturer(hardware_info.manufacturer);
-  specifics.set_last_updated_timestamp(TimeToProtoTime(base::Time::Now()));
-
-  sync_pb::FeatureSpecificFields* feature_fields =
-      specifics.mutable_feature_fields();
-  feature_fields->set_send_tab_to_self_receiving_enabled(
-      info.send_tab_to_self_receiving_enabled());
-
-  const base::Optional<DeviceInfo::SharingInfo>& sharing_info =
-      info.sharing_info();
-  if (sharing_info) {
-    sync_pb::SharingSpecificFields* sharing_fields =
-        specifics.mutable_sharing_fields();
-    sharing_fields->set_fcm_token(sharing_info->fcm_token);
-    sharing_fields->set_p256dh(sharing_info->p256dh);
-    sharing_fields->set_auth_secret(sharing_info->auth_secret);
-    for (sync_pb::SharingSpecificFields::EnabledFeatures feature :
-         sharing_info->enabled_features) {
-      sharing_fields->add_enabled_features(feature);
-    }
-  }
-
-  return specifics;
-}
-
 ModelTypeState StateWithEncryption(const std::string& encryption_key_name) {
   ModelTypeState state;
   state.set_initial_sync_done(true);
@@ -317,6 +282,11 @@ class TestLocalDeviceInfoProvider : public MutableLocalDeviceInfoProvider {
 
   void Clear() override { local_device_info_.reset(); }
 
+  void UpdateClientName(const std::string& client_name) override {
+    ASSERT_TRUE(local_device_info_);
+    local_device_info_->set_client_name(client_name);
+  }
+
   version_info::Channel GetChannel() const override {
     return version_info::Channel::UNKNOWN;
   }
@@ -344,7 +314,14 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
       : store_(ModelTypeStoreTestUtil::CreateInMemoryStoreForTest()),
         local_hardware_info_(GetLocalHardwareInfoBlocking()) {
     DeviceInfoPrefs::RegisterProfilePrefs(pref_service_.registry());
-    ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
+
+    // By default, mimic a real processor's behavior for IsTrackingMetadata().
+    ON_CALL(mock_processor_, ModelReadyToSync(_))
+        .WillByDefault([this](std::unique_ptr<MetadataBatch> batch) {
+          ON_CALL(mock_processor_, IsTrackingMetadata())
+              .WillByDefault(
+                  Return(batch->GetModelTypeState().initial_sync_done()));
+        });
   }
 
   ~DeviceInfoSyncBridgeTest() override {
@@ -393,6 +370,7 @@ class DeviceInfoSyncBridgeTest : public testing::Test,
         bridge()->CreateMetadataChangeList();
 
     metadata_change_list->UpdateModelTypeState(StateWithEncryption(""));
+    ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
     bridge()->MergeSyncData(std::move(metadata_change_list),
                             EntityChangeList());
   }
@@ -789,6 +767,7 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeEmpty) {
   EXPECT_CALL(*processor(), Delete(_, _)).Times(0);
 
   bridge()->OnSyncStarting(TestDataTypeActivationRequest(SyncMode::kFull));
+  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
   auto error = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
                                        EntityChangeList());
   EXPECT_FALSE(error);
@@ -810,6 +789,7 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuid) {
   EXPECT_CALL(*processor(), Delete(_, _)).Times(0);
 
   bridge()->OnSyncStarting(TestDataTypeActivationRequest(SyncMode::kFull));
+  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
   auto error =
       bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
                               EntityAddList({CreateLocalDeviceSpecifics()}));
@@ -1051,6 +1031,7 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplyStopSyncChangesWithClearData) {
   // If sync is re-enabled and the remote data is now empty, we shouldn't
   // contain remote data.
   bridge()->OnSyncStarting(TestDataTypeActivationRequest(SyncMode::kFull));
+  ON_CALL(*processor(), IsTrackingMetadata()).WillByDefault(Return(true));
   bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
                           EntityChangeList());
   // Local device.
@@ -1200,27 +1181,17 @@ TEST_F(DeviceInfoSyncBridgeTest, RefreshLocalDeviceNameForSyncModeToggle) {
   ASSERT_EQ(expected_device_name_full_sync, device->client_name());
 
   // Toggle to transport only sync mode.
-  syncer::EntityChangeList entity_change_list;
-  entity_change_list.push_back(EntityChange::CreateUpdate(
-      device->guid(), SpecificsToEntity(DeviceInfoToSpecifics(*device))));
-  bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
+  bridge()->ApplyStopSyncChanges(/*delete_metadata_change_list=*/nullptr);
   bridge()->OnSyncStarting(
       TestDataTypeActivationRequest(SyncMode::kTransportOnly));
-  bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
-                          std::move(entity_change_list));
 
   device = local_device()->GetLocalDeviceInfo();
   ASSERT_TRUE(device);
   ASSERT_EQ(expected_device_name_transport_only, device->client_name());
 
   // Toggle to full sync mode.
-  ASSERT_TRUE(entity_change_list.empty());
-  entity_change_list.push_back(EntityChange::CreateUpdate(
-      device->guid(), SpecificsToEntity(DeviceInfoToSpecifics(*device))));
-  bridge()->ApplyStopSyncChanges(bridge()->CreateMetadataChangeList());
+  bridge()->ApplyStopSyncChanges(/*delete_metadata_change_list=*/nullptr);
   bridge()->OnSyncStarting(TestDataTypeActivationRequest(SyncMode::kFull));
-  bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
-                          std::move(entity_change_list));
 
   device = local_device()->GetLocalDeviceInfo();
   ASSERT_TRUE(device);

components/sync_device_info/local_device_info_provider.h

@@ -46,6 +46,10 @@ class MutableLocalDeviceInfoProvider : public LocalDeviceInfoProvider {
                           const std::string& client_name,
                           const base::SysInfo::HardwareInfo& hardware_info) = 0;
   virtual void Clear() = 0;
+
+  // Updates the local device's client name. Initialize() must be called before
+  // calling this function.
+  virtual void UpdateClientName(const std::string& client_name) = 0;
 };
 
 }  // namespace syncer

components/sync_device_info/local_device_info_provider_impl.cc

@@ -76,4 +76,10 @@ void LocalDeviceInfoProviderImpl::Clear() {
   local_device_info_.reset();
 }
 
+void LocalDeviceInfoProviderImpl::UpdateClientName(
+    const std::string& client_name) {
+  DCHECK(local_device_info_);
+  local_device_info_->set_client_name(client_name);
+}
+
 }  // namespace syncer

components/sync_device_info/local_device_info_provider_impl.h

@@ -33,6 +33,7 @@ class LocalDeviceInfoProviderImpl : public MutableLocalDeviceInfoProvider {
                   const std::string& client_name,
                   const base::SysInfo::HardwareInfo& hardware_info) override;
   void Clear() override;
+  void UpdateClientName(const std::string& client_name) override;
   version_info::Channel GetChannel() const override;
   const DeviceInfo* GetLocalDeviceInfo() const override;
   std::unique_ptr<Subscription> RegisterOnInitializedCallback(