[Sync] DeviceInfoService should format ClientTag in backwards compatible way

components/sync_driver/device_info_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync_driver/device_info_service.h"
 
 #include <set>
 #include <utility>
 #include <vector>
 
 #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"
 #include "sync/internal_api/public/data_batch_impl.h"
 #include "sync/internal_api/public/simple_metadata_change_list.h"
 #include "sync/protocol/data_type_state.pb.h"
 #include "sync/protocol/sync.pb.h"
 
 namespace sync_driver_v2 {
 
@@ skipping 10 matching lines @@
 using sync_driver::DeviceInfo;
 using sync_pb::DataTypeState;
 using sync_pb::DeviceInfoSpecifics;
 using sync_pb::EntitySpecifics;
 
 using Record = ModelTypeStore::Record;
 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,
     const ChangeProcessorFactory& change_processor_factory)
     : ModelTypeService(change_processor_factory, syncer::DEVICE_INFO),
       local_device_info_provider_(local_device_info_provider),
       weak_factory_(this) {
   DCHECK(local_device_info_provider);
 
   // This is not threadsafe, but presuably the provider initializes on the same
@@ skipping 25 matching lines @@
         FROM_HERE, SyncError::DATATYPE_ERROR,
         "Cannot call MergeSyncData without provider, data, and processor.",
         syncer::DEVICE_INFO);
   }
 
   // Local data should typically be near empty, with the only possible value
   // corresponding to this device. This is because on signout all device info
   // 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());
   }
 
   CommitAndNotify(std::move(batch), std::move(metadata_change_list),
                   has_changes);
   return syncer::SyncError();
 }
 
 SyncError DeviceInfoService::ApplySyncChanges(
     scoped_ptr<MetadataChangeList> metadata_change_list,
     EntityChangeList entity_changes) {
   if (!has_provider_initialized_ || !has_metadata_loaded_) {
     return SyncError(
         FROM_HERE, SyncError::DATATYPE_ERROR,
         "Cannot call ApplySyncChanges before provider and data have loaded.",
         syncer::DEVICE_INFO);
   }
 
   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;
     }
   }
 
   CommitAndNotify(std::move(batch), std::move(metadata_change_list),
                   has_changes);
   return SyncError();
 }
 
 void DeviceInfoService::GetData(ClientTagList client_tags,
                                 DataCallback callback) {
   if (!has_metadata_loaded_) {
     callback.Run(
         SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
                   "Should not call GetData before metadata has loaded.",
                   syncer::DEVICE_INFO),
         scoped_ptr<DataBatchImpl>());
     return;
   }
 
   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));
     }
   }
   callback.Run(syncer::SyncError(), std::move(batch));
 }
 
 void DeviceInfoService::GetAllData(DataCallback callback) {
   if (!has_metadata_loaded_) {
     callback.Run(
         SyncError(FROM_HERE, SyncError::DATATYPE_ERROR,
                   "Should not call GetAllData before metadata has loaded.",
                   syncer::DEVICE_INFO),
         scoped_ptr<DataBatchImpl>());
     return;
   }
 
   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() {
   // We've recieved a new processor that needs metadata. If we're still in the
   // process of loading data and/or metadata, then |has_metadata_loaded_| is
   // false and we'll wait for those async reads to happen. If we've already
   // loaded metadata and then subsequently we get a new processor, we must not
   // have created the processor ourselves because we had no metadata. So there
   // must not be any metadata on disk.
   if (has_metadata_loaded_) {
@@ skipping 37 matching lines @@
 
 int DeviceInfoService::CountActiveDevices() const {
   // TODO(skym): crbug.com/590006: Implementation.
   return 0;
 }
 
 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.
 scoped_ptr<DeviceInfoSpecifics> DeviceInfoService::CopyToSpecifics(
     const DeviceInfo& info) {
   scoped_ptr<DeviceInfoSpecifics> specifics =
       make_scoped_ptr(new DeviceInfoSpecifics);
   specifics->set_cache_guid(info.guid());
   specifics->set_client_name(info.client_name());
   specifics->set_chrome_version(info.chrome_version());
@@ skipping 10 matching lines @@
       specifics.cache_guid(), specifics.client_name(),
       specifics.chrome_version(), specifics.sync_user_agent(),
       specifics.device_type(), specifics.signin_scoped_device_id()));
 }
 
 // Static.
 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 {
     return false;
   }
 }
 
 void DeviceInfoService::OnProviderInitialized() {
   has_provider_initialized_ = true;
   TryReconcileLocalAndStored();
@@ skipping 18 matching lines @@
     LOG(WARNING) << "Initial load of data failed.";
     // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
     // failure.
     return;
   }
 
   for (const Record& r : *record_list.get()) {
     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
       // failure.
     }
   }
   store_->ReadAllMetadata(base::Bind(&DeviceInfoService::OnReadAllMetadata,
                                      weak_factory_.GetWeakPtr()));
 }
 
@@ skipping 82 matching lines @@
       PutAndStore(*current_info);
     }
   }
 }
 
 void DeviceInfoService::PutAndStore(const DeviceInfo& device_info) {
   scoped_ptr<DeviceInfoSpecifics> specifics = CopyToSpecifics(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();
   StoreSpecifics(std::move(specifics), batch.get());
 
   CommitAndNotify(std::move(batch), std::move(metadata_change_list), true);
 }
 
 void DeviceInfoService::CommitAndNotify(
     scoped_ptr<WriteBatch> batch,
     scoped_ptr<MetadataChangeList> metadata_change_list,
     bool should_notify) {
   static_cast<SimpleMetadataChangeList*>(metadata_change_list.get())
       ->TransferChanges(store_.get(), batch.get());
   store_->CommitWriteBatch(
       std::move(batch),
       base::Bind(&DeviceInfoService::OnCommit, weak_factory_.GetWeakPtr()));
   if (should_notify) {
     NotifyObservers();
   }
 }
 
 }  // namespace sync_driver_v2