[Sync] DeviceInfoService static method and error cleanup.

components/sync/device_info/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/device_info/device_info_service.h"
 
+#include <stdint.h>
+
 #include <algorithm>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "components/sync/base/time.h"
 #include "components/sync/device_info/device_info_util.h"
@@ skipping 10 matching lines @@
 using base::TimeDelta;
 using sync_pb::DeviceInfoSpecifics;
 using sync_pb::EntitySpecifics;
 using sync_pb::ModelTypeState;
 
 using Record = ModelTypeStore::Record;
 using RecordList = ModelTypeStore::RecordList;
 using Result = ModelTypeStore::Result;
 using WriteBatch = ModelTypeStore::WriteBatch;
 
+namespace {
+
+// Find the timestamp for the last time this |device_info| was edited.
+Time GetLastUpdateTime(const DeviceInfoSpecifics& specifics) {

[maxbogue, 2016/10/28 20:20:01]

I genuinely find it more confusing to see just Time and not base::Time >_>

[skym, 2016/10/28 20:46:16]

Why? Is there some other 'Time' object you think I might be using?

+  if (specifics.has_last_updated_timestamp()) {
+    return ProtoTimeToTime(specifics.last_updated_timestamp());
+  } else {
+    return Time();
+  }
+}
+
+// Converts DeviceInfoSpecifics into a freshly allocated DeviceInfo.
+std::unique_ptr<DeviceInfo> SpecificsToModel(

[maxbogue, 2016/10/28 20:20:01]

Here as well I find Model to be vague. I think SpecificsToDeviceInfo and
DeviceInfoToSpecifics would be fine, descriptive names for these things.

[skym, 2016/10/28 20:46:16]

DeviceInfo describes everything going on here. DeviceInfo and
DeviceInfoSpecifics are both DeviceInfo. When every object is a DeviceInfo, no
object is a Device Info.

Model, contextually, has a very specific meaning. The old function was called
'CopyToModel' if that makes you feel any better, I'm not even introducing this!

+    const DeviceInfoSpecifics& specifics) {
+  return base::MakeUnique<DeviceInfo>(
+      specifics.cache_guid(), specifics.client_name(),
+      specifics.chrome_version(), specifics.sync_user_agent(),
+      specifics.device_type(), specifics.signin_scoped_device_id());
+}
+
+// Allocate a EntityData and copies |specifics| into it.
+std::unique_ptr<EntityData> CopyToEntityData(
+    const DeviceInfoSpecifics& specifics) {
+  auto entity_data = base::MakeUnique<EntityData>();
+  *entity_data->specifics.mutable_device_info() = specifics;
+  entity_data->non_unique_name = specifics.client_name();
+  return entity_data;
+}
+
+// Converts DeviceInfo into a freshly allocated DeviceInfoSpecifics. Takes
+// |last_updated_timestamp| to set because the model object does not contain
+// this concept.
+std::unique_ptr<DeviceInfoSpecifics> ModelToSpecifics(
+    const DeviceInfo& info,
+    int64_t last_updated_timestamp) {
+  auto specifics = base::MakeUnique<DeviceInfoSpecifics>();
+  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_last_updated_timestamp(last_updated_timestamp);
+  return specifics;
+}
+
+}  // namespace
+
 DeviceInfoService::DeviceInfoService(
     LocalDeviceInfoProvider* local_device_info_provider,
     const StoreFactoryFunction& callback,
     const ChangeProcessorFactory& change_processor_factory)
     : ModelTypeService(change_processor_factory, DEVICE_INFO),
       local_device_info_provider_(local_device_info_provider) {
   DCHECK(local_device_info_provider);
 
   // This is not threadsafe, but presuably the provider initializes on the same
   // thread as us so we're okay.
@@ skipping 36 matching lines @@
   const DeviceInfo* local_info =
       local_device_info_provider_->GetLocalDeviceInfo();
   std::string local_guid = local_info->guid();
   std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
   for (const auto& kv : entity_data_map) {
     const DeviceInfoSpecifics& specifics =
         kv.second.value().specifics.device_info();
     DCHECK_EQ(kv.first, specifics.cache_guid());
     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))) {
+      if (local_info->Equals(*SpecificsToModel(specifics))) {
         local_guids_to_put.erase(local_guid);
       } else {
         // This device is valid right now and this entry is about to be
         // committed, use this as an opportunity to refresh the timestamp.
         all_data_[local_guid]->set_last_updated_timestamp(
             TimeToProtoTime(Time::Now()));
       }
     } else {
       // Remote data wins conflicts.
       local_guids_to_put.erase(specifics.cache_guid());
@@ skipping 104 matching lines @@
 bool DeviceInfoService::IsSyncing() const {
   return !all_data_.empty();
 }
 
 std::unique_ptr<DeviceInfo> DeviceInfoService::GetDeviceInfo(
     const std::string& client_id) const {
   const ClientIdToSpecifics::const_iterator iter = all_data_.find(client_id);
   if (iter == all_data_.end()) {
     return std::unique_ptr<DeviceInfo>();
   }
-  return CopyToModel(*iter->second);
+  return SpecificsToModel(*iter->second);
 }
 
 std::vector<std::unique_ptr<DeviceInfo>> DeviceInfoService::GetAllDeviceInfo()
     const {
   std::vector<std::unique_ptr<DeviceInfo>> list;
   for (ClientIdToSpecifics::const_iterator iter = all_data_.begin();
        iter != all_data_.end(); ++iter) {
-    list.push_back(CopyToModel(*iter->second));
+    list.push_back(SpecificsToModel(*iter->second));
   }
   return list;
 }
 
 void DeviceInfoService::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
 
 void DeviceInfoService::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 int DeviceInfoService::CountActiveDevices() const {
   return CountActiveDevices(Time::Now());
 }
 
 void DeviceInfoService::NotifyObservers() {
   for (auto& observer : observers_)
     observer.OnDeviceInfoChange();
 }
 
-// Static.
-std::unique_ptr<DeviceInfoSpecifics> DeviceInfoService::CopyToSpecifics(
-    const DeviceInfo& info) {
-  std::unique_ptr<DeviceInfoSpecifics> specifics =
-      base::WrapUnique(new DeviceInfoSpecifics);
-  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());
-  return specifics;
-}
-
-// Static.
-std::unique_ptr<DeviceInfo> DeviceInfoService::CopyToModel(
-    const DeviceInfoSpecifics& specifics) {
-  return base::MakeUnique<DeviceInfo>(
-      specifics.cache_guid(), specifics.client_name(),
-      specifics.chrome_version(), specifics.sync_user_agent(),
-      specifics.device_type(), specifics.signin_scoped_device_id());
-}
-
-// Static.
-std::unique_ptr<EntityData> DeviceInfoService::CopyToEntityData(
-    const DeviceInfoSpecifics& specifics) {
-  std::unique_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(
     std::unique_ptr<DeviceInfoSpecifics> specifics,
     WriteBatch* batch) {
   const std::string guid = specifics->cache_guid();
-  DVLOG(1) << "Storing DEVICE_INFO for " << specifics->client_name()
-           << " with ID " << guid;
   store_->WriteData(batch, guid, specifics->SerializeAsString());
   all_data_[guid] = std::move(specifics);
 }
 
 bool DeviceInfoService::DeleteSpecifics(const std::string& guid,
                                         WriteBatch* batch) {
   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 " << guid;
     store_->DeleteData(batch, guid);
     all_data_.erase(iter);
     return true;
   } else {
     return false;
   }
 }
 
 void DeviceInfoService::OnProviderInitialized() {
   // Now that the provider has initialized, remove the subscription. The service
   // should only need to give the processor metadata upon initialization. If
   // sync is disabled and enabled, our provider will try to retrigger this
   // event, but we do not want to send any more metadata to the processor.
   subscription_.reset();
 
   has_provider_initialized_ = true;
   LoadMetadataIfReady();
 }
 
 void DeviceInfoService::OnStoreCreated(Result result,
                                        std::unique_ptr<ModelTypeStore> store) {
   if (result == Result::SUCCESS) {
     std::swap(store_, store);
     store_->ReadAllData(
         base::Bind(&DeviceInfoService::OnReadAllData, base::AsWeakPtr(this)));
   } else {
     ReportStartupErrorToSync("ModelTypeStore creation failed.");
-    // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
-    // failure.
   }
 }
 
 void DeviceInfoService::OnReadAllData(Result result,
                                       std::unique_ptr<RecordList> record_list) {
   if (result != Result::SUCCESS) {
     ReportStartupErrorToSync("Initial load of data failed.");
-    // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
-    // failure.
     return;
   }
 
   for (const Record& r : *record_list.get()) {
     std::unique_ptr<DeviceInfoSpecifics> specifics =
         base::MakeUnique<DeviceInfoSpecifics>();
     if (specifics->ParseFromString(r.value)) {
       all_data_[specifics->cache_guid()] = std::move(specifics);
     } else {
       ReportStartupErrorToSync("Failed to deserialize specifics.");
-      // TODO(skym, crbug.com/582460): Handle unrecoverable initialization
-      // failure.
     }
   }
 
   has_data_loaded_ = true;
   LoadMetadataIfReady();
 }
 
 void DeviceInfoService::LoadMetadataIfReady() {
   if (has_data_loaded_ && has_provider_initialized_) {
     store_->ReadAllMetadata(base::Bind(&DeviceInfoService::OnReadAllMetadata,
                                        base::AsWeakPtr(this)));
   }
 }
 
 void DeviceInfoService::OnReadAllMetadata(
     Result result,
     std::unique_ptr<RecordList> metadata_records,
     const std::string& global_metadata) {
   if (result != Result::SUCCESS) {
-    // Store has encountered some serious error. We should still be able to
-    // continue as a read only service, since if we got this far we must have
-    // loaded all data out succesfully.
     ReportStartupErrorToSync("Load of metadata completely failed.");
     return;
   }
 
-  std::unique_ptr<MetadataBatch> batch(new MetadataBatch());
+  auto batch = base::MakeUnique<MetadataBatch>();
   ModelTypeState state;
   if (state.ParseFromString(global_metadata)) {
     batch->SetModelTypeState(state);
   } else {
-    // TODO(skym): How bad is this scenario? We may be able to just give an
-    // empty batch to the processor and we'll treat corrupted data type state
-    // as no data type state at all. The question is do we want to add any of
-    // the entity metadata to the batch or completely skip that step? We're
-    // going to have to perform a merge shortly. Does this decision/logic even
-    // belong in this service?
-    change_processor()->OnMetadataLoaded(
-        change_processor()->CreateAndUploadError(
-            FROM_HERE, "Failed to deserialize global metadata."),
-        nullptr);
+    ReportStartupErrorToSync("Failed to deserialize global metadata.");
+    return;
   }
+
   for (const Record& r : *metadata_records.get()) {
     sync_pb::EntityMetadata entity_metadata;
     if (entity_metadata.ParseFromString(r.value)) {
       batch->AddMetadata(r.id, entity_metadata);
     } else {
-      // TODO(skym): This really isn't too bad. We just want to regenerate
-      // metadata for this particular entity. Unfortunately there isn't a
-      // convenient way to tell the processor to do this.
-      LOG(WARNING) << "Failed to deserialize entity metadata.";
+      ReportStartupErrorToSync("Failed to deserialize entity metadata.");
     }
   }
+
   change_processor()->OnMetadataLoaded(SyncError(), std::move(batch));
   ReconcileLocalAndStored();
 }
 
 void DeviceInfoService::OnCommit(Result result) {
   if (result != Result::SUCCESS) {
-    LOG(WARNING) << "Failed a write to store.";
+    change_processor()->CreateAndUploadError(FROM_HERE,
+                                             "Failed a write to store.");
   }
 }
 
 void DeviceInfoService::ReconcileLocalAndStored() {
   // On initial syncing we will have a change processor here, but it will not be
   // tracking changes. We need to persist a copy of our local device info to
   // disk, but the Put call to the processor will be ignored. That should be
   // fine however, as the discrepancy will be picked up later in merge. We don't
   // bother trying to track this case and act intelligently because simply not
   // much of a benefit in doing so.
   DCHECK(has_provider_initialized_);
 
   const DeviceInfo* current_info =
       local_device_info_provider_->GetLocalDeviceInfo();
   auto iter = all_data_.find(current_info->guid());
 
   // Convert to DeviceInfo for Equals function.
   if (iter != all_data_.end() &&
-      current_info->Equals(*CopyToModel(*iter->second))) {
+      current_info->Equals(*SpecificsToModel(*iter->second))) {
     const TimeDelta pulse_delay(DeviceInfoUtil::CalculatePulseDelay(
         GetLastUpdateTime(*iter->second), Time::Now()));
     if (!pulse_delay.is_zero()) {
       pulse_timer_.Start(FROM_HERE, pulse_delay,
                          base::Bind(&DeviceInfoService::SendLocalData,
                                     base::Unretained(this)));
       return;
     }
   }
   SendLocalData();
 }
 
 void DeviceInfoService::SendLocalData() {
   DCHECK(has_provider_initialized_);
 
   // It is possible that the provider no longer has data for us, such as when
   // the user signs out. No-op this pulse, but keep the timer going in case sync
   // is enabled later.
   if (local_device_info_provider_->GetLocalDeviceInfo() != nullptr) {
     std::unique_ptr<DeviceInfoSpecifics> specifics =
-        CopyToSpecifics(*local_device_info_provider_->GetLocalDeviceInfo());
-    specifics->set_last_updated_timestamp(TimeToProtoTime(Time::Now()));
-
+        ModelToSpecifics(*local_device_info_provider_->GetLocalDeviceInfo(),
+                         TimeToProtoTime(Time::Now()));
     std::unique_ptr<MetadataChangeList> metadata_change_list =
         CreateMetadataChangeList();
     if (change_processor()->IsTrackingMetadata()) {
       change_processor()->Put(specifics->cache_guid(),
                               CopyToEntityData(*specifics),
                               metadata_change_list.get());
     }
 
     std::unique_ptr<WriteBatch> batch = store_->CreateWriteBatch();
     StoreSpecifics(std::move(specifics), batch.get());
@@ skipping 27 matching lines @@
                        });
 }
 
 void DeviceInfoService::ReportStartupErrorToSync(const std::string& msg) {
   // TODO(skym): Shouldn't need to log this here, reporting should always log.
   LOG(WARNING) << msg;
   change_processor()->OnMetadataLoaded(
       change_processor()->CreateAndUploadError(FROM_HERE, msg), nullptr);
 }
 
-// static
-Time DeviceInfoService::GetLastUpdateTime(
-    const DeviceInfoSpecifics& specifics) {
-  if (specifics.has_last_updated_timestamp()) {
-    return ProtoTimeToTime(specifics.last_updated_timestamp());
-  } else {
-    return Time();
-  }
-}
-
 }  // namespace syncer