Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - part 3.

chrome/browser/sync/glue/device_info_sync_service.h

Index: chrome/browser/sync/glue/device_info_sync_service.h
diff --git a/chrome/browser/sync/glue/device_info_sync_service.h b/chrome/browser/sync/glue/device_info_sync_service.h
index 3313fc42733fbf18d9d80d9ef0f845823507987d..069b5795670e8f619fdff6e974cd186b3c140206 100644
--- a/chrome/browser/sync/glue/device_info_sync_service.h
+++ b/chrome/browser/sync/glue/device_info_sync_service.h
@@ -44,9 +44,18 @@ class DeviceInfoSyncService : public syncer::SyncableService,
   virtual void AddObserver(Observer* observer) OVERRIDE;
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
 
+  // Called to update local device backup time in the directory.

[stanisc, 2014/09/16 00:17:40]

I need to change this comment. It doesn't come from the directory anymore.

[stanisc, 2014/09/18 22:46:54]

Done.

+  void UpdateLocalDeviceBackupTime(base::Time backup_time);
+  // Reads local device backup time from the directory.
+  base::Time GetLocalDeviceBackupTime() const;

[stanisc, 2014/09/16 00:17:40]

I need to change this comment. It doesn't come from the directory anymore.

[stanisc, 2014/09/18 22:46:54]

Done.

+
  private:
-  // Create SyncData from local DeviceInfo.
-  static syncer::SyncData CreateLocalData(const DeviceInfo* info);
+  // Create SyncData from local DeviceInfo (and backup timestamp).

[stanisc, 2014/09/16 00:17:40]

I should replace "backup timestamp" with |local_device_backup_time_|.

[stanisc, 2014/09/18 22:46:53]

Done.

+  syncer::SyncData CreateLocalData(const DeviceInfo* info);
+  // Create SyncData from EntitySpecifics.
+  static syncer::SyncData CreateLocalData(
+      const sync_pb::EntitySpecifics& entity);
+
   // Allocate new DeviceInfo from SyncData.
   static DeviceInfo* CreateDeviceInfo(const syncer::SyncData sync_data);
   // Store SyncData in the cache.
@@ -57,6 +66,25 @@ class DeviceInfoSyncService : public syncer::SyncableService,
   // Notify all registered observers.
   void NotifyObservers();
 
+  // Updates backup time in place in |sync_data| if it is different than
+  // the one stored in |local_device_backup_time_|.
+  // Returns true if backup time was updated.
+  bool UpdateBackupTime(syncer::SyncData& sync_data);
+
+  // |local_device_backup_time_| accessors.
+  int64 local_device_backup_time() const { return local_device_backup_time_; }
+  bool has_local_device_backup_time() const {
+    return local_device_backup_time_ >= 0;
+  }
+  void set_local_device_backup_time(int64 value) {
+    local_device_backup_time_ = value;
+  }
+  void clear_local_device_backup_time() { local_device_backup_time_ = -1; }
+
+  // Local device last set backup time (in proto format).
+  // -1 if the value hasn't been specified
+  int64 local_device_backup_time_;

[pavely, 2014/09/17 23:04:38]

Consider storing local_device_backup_time_ as base::Time, it will be more
descriptive. You can use base::Time() as special value and is_null for check.
This will get rid of family of getters/setters above.

[stanisc, 2014/09/18 22:46:53]

That is exactly what I had when I first implemented GetLocalDeviceBackupTime and
UpdateBackupTime here. However I had to change it because it has to support  3
states - unset, null, and some valid value.
Initially I used is_null() to check whether it was unset. But it turned out
there is at lest one test that would set null backup time and expect it back.
Converting to an integer value allowed me to distinguish unset and null. Also it
reduced the number of conversions between the proto time and the base time which
made the code look clearer. 

+
   // |local_device_info_provider_| isn't owned.
   const LocalDeviceInfoProvider* const local_device_info_provider_;