[Sync] DeviceInfoService should format ClientTag in backwards compatible way

components/sync_driver/device_info_service.cc

Index: components/sync_driver/device_info_service.cc
diff --git a/components/sync_driver/device_info_service.cc b/components/sync_driver/device_info_service.cc
index 8318e5d290755509967c4802848a0a827806efe5..c80128a91a34d1ac15e994dbfad78a9d481e6c7c 100644
--- a/components/sync_driver/device_info_service.cc
+++ b/components/sync_driver/device_info_service.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/location.h"
+#include "base/strings/string_util.h"
 #include "sync/api/entity_change.h"
 #include "sync/api/metadata_batch.h"
 #include "sync/api/sync_error.h"
@@ -40,6 +41,8 @@ using RecordList = ModelTypeStore::RecordList;
 using Result = ModelTypeStore::Result;
 using WriteBatch = ModelTypeStore::WriteBatch;
 
+const char kClientTagPrefix[] = "DeviceInfo_";

[skym, 2016/04/14 21:08:26]

Can old and new device info services share this constant?

[maxbogue, 2016/04/14 22:30:13]

Or at least it should be in an anonymous namespace shouldn't it?

[pavely, 2016/04/14 23:37:23]

It would be nice. The best shared place for declaring this constant is
local_device_info_provider.h, this is the most natural common dependency between
DeviceInfoService and DeviceInfoSyncService. It feels like this constant is out
of place there.

Let me know if you feel strongly about it and I'll move constant there.

[pavely, 2016/04/14 23:37:23]

Done.

+
 DeviceInfoService::DeviceInfoService(
     sync_driver::LocalDeviceInfoProvider* local_device_info_provider,
     const StoreFactoryFunction& callback,
@@ -85,36 +88,37 @@ SyncError DeviceInfoService::MergeSyncData(
   // data is blown away. However, this simplification is being ignored here and
   // a full difference is going to be calculated to explore what other service
   // implementations may look like.
-  std::set<std::string> local_tags_to_put;
+  std::set<std::string> local_guids_to_put;
   for (const auto& kv : all_data_) {
-    local_tags_to_put.insert(kv.first);
+    local_guids_to_put.insert(kv.first);
   }
 
   bool has_changes = false;
   const DeviceInfo* local_info =
       local_device_info_provider_->GetLocalDeviceInfo();
-  std::string local_tag = local_info->guid();
+  std::string local_guid = local_info->guid();
   scoped_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   for (const auto& kv : entity_data_map) {
-    const std::string tag = GetClientTag(kv.second.value());
     const DeviceInfoSpecifics& specifics =
         kv.second.value().specifics.device_info();
-    if (tag == local_tag) {
+    DCHECK_EQ(kv.first, SpecificsToTag(specifics));
+    if (specifics.cache_guid() == local_guid) {
       // Don't Put local data if it's the same as the remote copy.
       if (local_info->Equals(*CopyToModel(specifics))) {
-        local_tags_to_put.erase(tag);
+        local_guids_to_put.erase(local_guid);
       }
     } else {
       // Remote data wins conflicts.
-      local_tags_to_put.erase(tag);
+      local_guids_to_put.erase(specifics.cache_guid());
       has_changes = true;
       StoreSpecifics(make_scoped_ptr(new DeviceInfoSpecifics(specifics)),
                      batch.get());
     }
   }
 
-  for (const std::string& tag : local_tags_to_put) {
-    change_processor()->Put(tag, CopyToEntityData(*all_data_[tag]),
+  for (const std::string& guid : local_guids_to_put) {
+    change_processor()->Put(SpecificsToTag(*all_data_[guid]),
+                            CopyToEntityData(*all_data_[guid]),
                             metadata_change_list.get());
   }
 
@@ -136,19 +140,19 @@ SyncError DeviceInfoService::ApplySyncChanges(
   scoped_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   bool has_changes = false;
   for (EntityChange& change : entity_changes) {
-    const std::string tag = change.client_tag();
+    const std::string guid = TagToCacheGuid(change.client_tag());
     // Each device is the authoritative source for itself, ignore any remote
     // changes that have our local cache guid.
-    if (tag == local_device_info_provider_->GetLocalDeviceInfo()->guid()) {
+    if (guid == local_device_info_provider_->GetLocalDeviceInfo()->guid()) {
       continue;
     }
 
     if (change.type() == EntityChange::ACTION_DELETE) {
-      has_changes |= DeleteSpecifics(tag, batch.get());
+      has_changes |= DeleteSpecifics(guid, batch.get());
     } else {
       const DeviceInfoSpecifics& specifics =
           change.data().specifics.device_info();
-      DCHECK(tag == specifics.cache_guid());
+      DCHECK(guid == specifics.cache_guid());
       StoreSpecifics(make_scoped_ptr(new DeviceInfoSpecifics(specifics)),
                      batch.get());
       has_changes = true;
@@ -173,8 +177,9 @@ void DeviceInfoService::GetData(ClientTagList client_tags,
 
   scoped_ptr<DataBatchImpl> batch(new DataBatchImpl());
   for (const auto& tag : client_tags) {
-    const auto iter = all_data_.find(tag);
+    const auto& iter = all_data_.find(TagToCacheGuid(tag));
     if (iter != all_data_.end()) {
+      DCHECK_EQ(tag, SpecificsToTag(*iter->second));
       batch->Put(tag, CopyToEntityData(*iter->second));
     }
   }
@@ -193,14 +198,14 @@ void DeviceInfoService::GetAllData(DataCallback callback) {
 
   scoped_ptr<DataBatchImpl> batch(new DataBatchImpl());
   for (const auto& kv : all_data_) {
-    batch->Put(kv.first, CopyToEntityData(*kv.second));
+    batch->Put(SpecificsToTag(*kv.second), CopyToEntityData(*kv.second));
   }
   callback.Run(syncer::SyncError(), std::move(batch));
 }
 
 std::string DeviceInfoService::GetClientTag(const EntityData& entity_data) {
   DCHECK(entity_data.specifics.has_device_info());
-  return entity_data.specifics.device_info().cache_guid();
+  return SpecificsToTag(entity_data.specifics.device_info());
 }
 
 void DeviceInfoService::OnChangeProcessorSet() {
@@ -258,6 +263,16 @@ void DeviceInfoService::NotifyObservers() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDeviceInfoChange());
 }
 
+std::string DeviceInfoService::SpecificsToTag(
+    const sync_pb::DeviceInfoSpecifics& specifics) {
+  return kClientTagPrefix + specifics.cache_guid();
+}
+
+std::string DeviceInfoService::TagToCacheGuid(const std::string& tag) {
+  DCHECK(base::StartsWith(tag, kClientTagPrefix, base::CompareCase::SENSITIVE));
+  return tag.substr(strlen(kClientTagPrefix));
+}
+
 // TODO(skym): crbug.com/543406: It might not make sense for this to be a
 // scoped_ptr.
 // Static.
@@ -288,26 +303,27 @@ scoped_ptr<EntityData> DeviceInfoService::CopyToEntityData(
     const DeviceInfoSpecifics& specifics) {
   scoped_ptr<EntityData> entity_data(new EntityData());
   *entity_data->specifics.mutable_device_info() = specifics;
+  entity_data->non_unique_name = specifics.client_name();
   return entity_data;
 }
 
 void DeviceInfoService::StoreSpecifics(
     scoped_ptr<DeviceInfoSpecifics> specifics,
     WriteBatch* batch) {
-  const std::string tag = specifics->cache_guid();
+  const std::string guid = specifics->cache_guid();
   DVLOG(1) << "Storing DEVICE_INFO for " << specifics->client_name()
-           << " with ID " << tag;
-  store_->WriteData(batch, tag, specifics->SerializeAsString());
-  all_data_[tag] = std::move(specifics);
+           << " with ID " << guid;
+  store_->WriteData(batch, guid, specifics->SerializeAsString());
+  all_data_[guid] = std::move(specifics);
 }
 
-bool DeviceInfoService::DeleteSpecifics(const std::string& tag,
+bool DeviceInfoService::DeleteSpecifics(const std::string& guid,
                                         WriteBatch* batch) {
-  ClientIdToSpecifics::const_iterator iter = all_data_.find(tag);
+  ClientIdToSpecifics::const_iterator iter = all_data_.find(guid);
   if (iter != all_data_.end()) {
     DVLOG(1) << "Deleting DEVICE_INFO for " << iter->second->client_name()
-             << " with ID " << tag;
-    store_->DeleteData(batch, tag);
+             << " with ID " << guid;
+    store_->DeleteData(batch, guid);
     all_data_.erase(iter);
     return true;
   } else {
@@ -346,7 +362,8 @@ void DeviceInfoService::OnReadAllData(Result result,
     scoped_ptr<DeviceInfoSpecifics> specifics(
         make_scoped_ptr(new DeviceInfoSpecifics()));
     if (specifics->ParseFromString(r.value)) {
-      all_data_[r.id] = std::move(specifics);
+      std::string guid = specifics->cache_guid();

[skym, 2016/04/14 21:08:26]

Does this do an unnecessary copy?

[pavely, 2016/04/14 23:37:23]

Done.

+      all_data_[guid] = std::move(specifics);
     } else {
       LOG(WARNING) << "Failed to deserialize specifics.";
       // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
@@ -449,7 +466,8 @@ void DeviceInfoService::PutAndStore(const DeviceInfo& device_info) {
 
   scoped_ptr<MetadataChangeList> metadata_change_list =
       CreateMetadataChangeList();
-  change_processor()->Put(device_info.guid(), CopyToEntityData(*specifics),
+  change_processor()->Put(SpecificsToTag(*specifics),
+                          CopyToEntityData(*specifics),
                           metadata_change_list.get());
 
   scoped_ptr<WriteBatch> batch = store_->CreateWriteBatch();