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

chrome/browser/sync/glue/device_info_sync_service.h

 // Copyright 2014 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.
 
 #ifndef CHROME_BROWSER_SYNC_GLUE_DEVICE_INFO_SYNC_SERVICE_H_
 #define CHROME_BROWSER_SYNC_GLUE_DEVICE_INFO_SYNC_SERVICE_H_
 
 #include "base/observer_list.h"
 #include "chrome/browser/sync/glue/device_info_tracker.h"
 #include "sync/api/sync_change_processor.h"
@@ skipping 26 matching lines @@
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) OVERRIDE;
 
   // DeviceInfoTracker implementation.
   virtual scoped_ptr<DeviceInfo> GetDeviceInfo(
       const std::string& client_id) const OVERRIDE;
   virtual ScopedVector<DeviceInfo> GetAllDeviceInfo() const OVERRIDE;
   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.
   void StoreSyncData(const std::string& client_id,
                      const syncer::SyncData& sync_data);
   // Delete SyncData from the cache.
   void DeleteSyncData(const std::string& client_id);
   // 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_;
 
   // Receives ownership of |sync_processor_| and |error_handler_| in
   // MergeDataAndStartSyncing() and destroy them in StopSyncing().
   scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;
   scoped_ptr<syncer::SyncErrorFactory> error_handler_;
 
   // Cache of all syncable and local data.
   typedef std::map<std::string, syncer::SyncData> SyncDataMap;
   SyncDataMap all_data_;
 
   // Registered observers, not owned.
   ObserverList<Observer, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceInfoSyncService);
 };
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_GLUE_DEVICE_INFO_SYNC_SERVICE_H_