Implement new SyncAPI and convert Preferences to it.

chrome/browser/sync/api/sync_data.h

Index: chrome/browser/sync/api/sync_data.h
diff --git a/chrome/browser/sync/api/sync_data.h b/chrome/browser/sync/api/sync_data.h
new file mode 100644
index 0000000000000000000000000000000000000000..174c34a6d2f02ec78ba6524a5fc0132fcf76a484
--- /dev/null
+++ b/chrome/browser/sync/api/sync_data.h
@@ -0,0 +1,86 @@
+// Copyright (c) 2011 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_API_SYNC_DATA_H_
+#define CHROME_BROWSER_SYNC_API_SYNC_DATA_H_
+#pragma once
+
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/sync/protocol/sync.pb.h"

[akalin, 2011/05/19 00:58:45]

replace with fwd declare

[Nicolas Zea, 2011/05/19 21:17:45]

Done.

+#include "chrome/browser/sync/syncable/model_type.h"
+
+typedef syncable::ModelType SyncDataType;
+
+// A reference counted immutable SyncEntity.
+class RefCountedSyncEntity : public

[akalin, 2011/05/19 00:58:45]

consider making this a private inner class of SyncData.

Also, SharedSyncEntity may be a shorter name.

[Nicolas Zea, 2011/05/19 21:17:45]

Done.

+    base::RefCountedThreadSafe<RefCountedSyncEntity> {
+ public:
+  // Takes ownership of data.
+  explicit RefCountedSyncEntity(sync_pb::SyncEntity* sync_entity);
+
+  // Returns immutable reference to local sync entity.
+  const sync_pb::SyncEntity& sync_entity();

[akalin, 2011/05/19 00:58:45]

function should be const

[Nicolas Zea, 2011/05/19 21:17:45]

Done.

+
+ private:
+  friend class base::RefCountedThreadSafe<RefCountedSyncEntity>;
+
+  // Private due to ref counting.
+  ~RefCountedSyncEntity();
+
+  scoped_ptr<sync_pb::SyncEntity> sync_entity_;
+};
+
+// A light-weight container for immutable sync data. Pass-by-value and storage
+// in STL containers are supported and encouraged if helpful.
+class SyncData {
+ public:
+  // Creates an empty and invalid SyncData.
+  SyncData();
+   ~SyncData();
+
+   // Default copy and assign welcome.
+
+  // Helper methods for creating SyncData objects for local data.
+  // Local sync data must always at least set the sync tag.
+  static SyncData CreateLocalData(const std::string& sync_tag);

[akalin, 2011/05/19 00:58:45]

What do you think about making these (possibly-friended) non-member functions?

[Nicolas Zea, 2011/05/19 21:17:45]

Because we are in the global namespace, I think it's probably better to prefer
static member methods over global methods.

+  static SyncData CreateLocalData(
+      const std::string& sync_tag,
+      const sync_pb::EntitySpecifics& specifics);
+
+  // Helper method for creating SyncData objects originating from the syncer.

[akalin, 2011/05/19 00:58:45]

I think we should find some way to not expose this in the public header.  Maybe
make them (possibly-friended) non-member functions that are declared in another
header?

[Nicolas Zea, 2011/05/19 21:17:45]

Now that we have explicit validity rules/checking and SyncData's origin (local
vs remote) is exposed, I think it makes sense to have this here too, as opposed
to special casing the syncer usage. 

SyncData is a generic sync construct now, and I think it's apparent both from
the name and the comment that this is not for usage by a local syncableservice.

+  static SyncData CreateRemoteData(
+      const sync_pb::SyncEntity& entity);
+  static SyncData CreateRemoteData(
+    const sync_pb::EntitySpecifics& specifics);
+
+  // Whether this SyncData holds valid data. The only way to have a SyncData
+  // without valid data is to use the default constructor.
+  bool IsValid() const;
+  // Return the datatype modified by this sync event. Derived from the sync
+  // datatype specifics.
+  SyncDataType GetDataType() const;
+  // Return the current sync datatype specifics.
+  const sync_pb::EntitySpecifics& GetSpecifics() const;
+  // Returns the value of the unique client tag. This is only set for data going

[akalin, 2011/05/19 00:58:45]

Did we end up needing this?

[Nicolas Zea, 2011/05/19 21:17:45]

Yes, all changes coming from a local syncableservice must specify the tag.

[akalin, 2011/05/20 00:08:05]

I mean is this accessor specifically needed?  Doesn't the sync code work with
the hash, and why would the local data need to read the tag back out?

[Nicolas Zea, 2011/05/20 02:00:28]

The sync code creates the hash from this field, so uses this accessor to get the
tag.

The local data might use it if it holds on to a syncdata object for whatever
reason, for example if it must buffer local changes, and possibly overwriting
with the newest version (for example for the purpose of doing datatype specific
commit throttling, which I've thought about).

Plus it helps to have it around for testing/sanity checks in the code. The way I
see it, if the local service can set it, it should be able to verify it.

+  // TO the syncer, not coming from.
+  const std::string& GetTag() const;
+  // Whether this sync data is for local data or data coming from the syncer.
+  bool IsLocal() const;
+
+  // TODO(zea): Query methods for other sync properties: title, parent,
+  // successor, etc. Possibly support for hashed tag and sync id?
+
+ protected:
+  // The actual shared sync entity being held.
+  scoped_refptr<RefCountedSyncEntity> shared_entity_;
+
+  // Whether this data originated locally or from the syncer (remote data).
+  bool is_local_;
+};
+
+#endif  // CHROME_BROWSER_SYNC_API_SYNC_DATA_H_