Refactor syncable DEVICE_INFO type from ChangeProcessor to SyncableService - Part 2

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 "chrome/browser/sync/glue/device_info_tracker.h"
+#include "sync/api/sync_change_processor.h"
+#include "sync/api/sync_data.h"
+#include "sync/api/sync_error_factory.h"
+#include "sync/api/syncable_service.h"
+
+namespace browser_sync {
+
+class LocalDeviceInfoProvider;
+
+// SyncableService implementation for DEVICE_INFO model type.
+class DeviceInfoSyncService : public syncer::SyncableService,
+                              public DeviceInfoTracker {
+ public:
+  explicit DeviceInfoSyncService(
+      LocalDeviceInfoProvider* local_device_info_provider);
+  virtual ~DeviceInfoSyncService();
+
+  // syncer::SyncableService implementation.
+  virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
+      syncer::ModelType type,
+      const syncer::SyncDataList& initial_sync_data,
+      scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+      scoped_ptr<syncer::SyncErrorFactory> error_handler) OVERRIDE;
+  virtual void StopSyncing(syncer::ModelType type) OVERRIDE;
+  virtual syncer::SyncDataList GetAllSyncData(
+      syncer::ModelType type) const OVERRIDE;
+  virtual syncer::SyncError ProcessSyncChanges(
+      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 scoped_ptr<Subscription> RegisterOnDeviceInfoChangedCallback(
+      const base::Closure& callback) OVERRIDE;
+
+ private:
+  // Create SyncData from local DeviceInfo.

[rlarocque, 2014/08/01 20:40:36]

nit: I prefer to have a whitespace line before a comment line.  I don't think
the style guide specifies one way or the other, so feel free to leave it as is.

+  static syncer::SyncData CreateLocalData(const DeviceInfo* info);
+  // 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);
+
+  // |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_;

[rlarocque, 2014/08/01 20:40:36]

I think Pavel already pointed this out, but I'll mention it again in case he
didn't.

This is a little bit strange.  Most data types were built first, then had sync
features grafted on afterwards.  We didn't want to rewrite them when adding sync
functionality, so they tend to keep their data in their own format then convert
it to a sync-friendly format at the last minute, just before calling
ProcessSyncChanges.  They also tend to convert from SyncData into their own data
structures as soon as the data is received.

This type was synced before it was a proper data type, so you had the
opportunity to do things differently.  Yours might be one of the few types that
keeps an internal data structure filled with SyncDatas.  I don't see any
problems with this, but I just figured you should be aware that this is slightly
unusual.

As long as we don't make any significant changes SyncData (and I see no reason
why we would), then this won't cause any problems, and it makes it easier to
manage book-keeping for sync.

[stanisc, 2014/08/01 21:37:42]

Yeah, I've thought about using native DeviceInfo when implementing this code.
The current implementation of SyncedDeviceTracker actually does the same - it
stores SyncData in the directory and creates native DeviceInfo on demand. I
decided to stick with SyncData mainly for the following two reasons:
1) GetAllSyncData method should probably return the original SyncData - remote
SyncData for sync'ed devices and local SyncData for the local device. I guess it
is possible to recreate SyncData from DeviceInfo based on whether device GUID
matches the local device or not. But I thought it would be simpler to just cache
SyncData that comes from the sync and don't have to worry about recreating it.
2) Copy semantics is disabled on DeviceInfo which makes it unsuitable to be used
with STL collections. I don't know if there is a good reason for that. The type
seems to be simple and safe enough to be copied around. So perhaps the right
approach could to enable copy constructor on DeviceInfo and copy DeviceInfo
values to the cache.

[rlarocque, 2014/08/01 23:03:23]

SGTM for now.  I see this as an experiment.  Since we might end up doing
something similar in some of the non-blocking sync data types, it would be nice
to see how it works out here.

+
+  // Registered observer callbacks.
+  base::CallbackList<void(void)> callback_list_;

[rlarocque, 2014/08/01 20:40:36]

Why not use base/observer_list.h?

[stanisc, 2014/08/01 21:37:42]

I thought CallbackList was simpler and more elegant, mainly because it doesn't
require an observer code to implement an Observer interface. I'll go ahead and
move to ObserverList.

[stanisc, 2014/08/01 22:55:50]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(DeviceInfoSyncService);
+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_DEVICE_INFO_SYNC_SERVICE_H_