Separate SyncData methods into three groups, local, remote, and common.

sync/api/sync_data.h

Index: sync/api/sync_data.h
diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h
index d801149cac4d8bacc811588aad4a91f8775835e6..9e78603c6d99dd6b72e6758bfa454db1573162e8 100644
--- a/sync/api/sync_data.h
+++ b/sync/api/sync_data.h
@@ -86,22 +86,10 @@ class SYNC_EXPORT SyncData {
   // 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
-  // TO the syncer, not coming from.
-  const std::string& GetTag() const;
-
   // Returns the non unique title (for debugging). Currently only set for data
   // going TO the syncer, not from.
   const std::string& GetTitle() const;
 
-  // Returns the last motification time according to the server. This is
-  // only valid if IsLocal() is false, and may be null if the SyncData
-  // represents a deleted item.
-  const base::Time& GetRemoteModifiedTime() const;
-
-  // Should only be called by sync code when IsLocal() is false.
-  int64 GetRemoteId() const;
-
   // Whether this sync data is for local data or data coming from the syncer.
   bool IsLocal() const;
 
@@ -117,40 +105,88 @@ class SYNC_EXPORT SyncData {
   // The attachments may or may not be present on this device.
   AttachmentIdList GetAttachmentIds() const;
 
-  // Return a list of this SyncData's attachments.
-  //
-  // May only be called when IsLocal() is true.
-  const AttachmentList& GetLocalAttachmentsForUpload() const;
+  // TODO(zea): Query methods for other sync properties: parent, successor, etc.
 
-  // Retrieve the attachments indentified by |attachment_ids|. Invoke |callback|
-  // with the requested attachments.
-  //
-  // May only be called when IsLocal() is false.
-  //
-  // |callback| will be invoked when the operation is complete (successfully or
-  // otherwise).
-  //
-  // Retrieving the requested attachments may require reading local storage or
-  // requesting the attachments from the network.
+  // This class is comprised of methods that operate on "local" SyncData.  That
+  // is, SyncData where IsLocal() == true.  See also local().
+  class Local {

[tim (not reviewing), 2014/03/28 21:16:02]

I guess I was imagining we would organize the data at construction time, with
local fields going into Local and remote into Remote, exploiting the fact that
it can't change after construction.

Also if we keep Local/Remote to POD containers only, we wouldn't need to expose
the non-const ref (which is technically anti-style guide) and could keep
GetOrDownloadAttachments as a method on SyncData.

The other approach I was thinking of involved subclassing, and maybe in that
world we'd organize the methods on the proper subclass, but here I feel it'd be
better to keep it to data-only?

[maniscalco, 2014/03/28 21:45:13]

Re organizing the data at construction time... I explored that, but gave up once
I noticed GetTag (local) needs to access immutable_entity_.  I should this
approach.  I can probably make it work if the Remote and Local classes have
storage *and* have access to the common stuff via SyncData.

I'm not sure I follow your second comment about POD containers.  Maybe we should
sync up in person?

+   public:
+    // Return a list of this SyncData's attachments.
+    //
+    // May only be called when IsLocal() is true.
+    const AttachmentList& GetLocalAttachmentsForUpload() const;
+
+    // Returns the value of the unique client tag. This is only set for data
+    // going TO the syncer, not coming from.
+    const std::string& GetTag() const;
+
+   private:
+    friend SyncData;

[tim (not reviewing), 2014/03/28 21:16:02]

Do you actually need the friend?

[maniscalco, 2014/03/28 21:45:13]

Yeah, I was using it along with the private Local ctor to ensure that only
SyncData can construct SyncData::Local (or SyncData::Remote).  The motivation
was to "prevent callers from misusing SyncData::Local".

+    Local(SyncData* sync_data) : sync_data_(sync_data) {}
+
+    SyncData* sync_data_;
+  };
+
+  // Used to access Local methods.
   //
-  void GetOrDownloadAttachments(
-      const AttachmentIdList& attachment_ids,
-      const AttachmentService::GetOrDownloadCallback& callback);
+  // May only be called when IsLocal() is true.
+  const Local& local() const;
+
+  // This class is comprised of methods that operate on "remote" SyncData.  That
+  // is, SyncData where IsLocal() == false.  See also remote().
+  class Remote {
+   public:
+    // Returns the last motification time according to the server. This is
+    // only valid if IsLocal() is false, and may be null if the SyncData
+    // represents a deleted item.
+    const base::Time& GetRemoteModifiedTime() const;
+
+    // Should only be called by sync code when IsLocal() is false.
+    int64 GetRemoteId() const;

[tim (not reviewing), 2014/03/28 21:16:02]

I think 'Remote' in names of methods is redundant now that there's a class.

[maniscalco, 2014/03/28 21:45:13]

Agreed.  Removed.

+
+    // Retrieve the attachments indentified by |attachment_ids|. Invoke
+    // |callback| with the requested attachments.
+    //
+    // May only be called when IsLocal() is false.
+    //
+    // |callback| will be invoked when the operation is complete (successfully
+    // or otherwise).
+    //
+    // Retrieving the requested attachments may require reading local storage or
+    // requesting the attachments from the network.
+    //
+    void GetOrDownloadAttachments(
+        const AttachmentIdList& attachment_ids,
+        const AttachmentService::GetOrDownloadCallback& callback);
+
+    // Drop (delete from local storage) the attachments associated with this
+    // SyncData specified in |attachment_ids|. This method will not delete
+    // attachments from the server.
+    //
+    // May only be called when IsLocal() is false.
+    //
+    // |callback| will be invoked when the operation is complete (successfully
+    // or otherwise).
+    void DropAttachments(const AttachmentIdList& attachment_ids,
+                         const AttachmentService::DropCallback& callback);
+
+   private:
+    friend SyncData;
+    Remote(SyncData* sync_data) : sync_data_(sync_data) {}
+
+    SyncData* sync_data_;
+  };
 
-  // Drop (delete from local storage) the attachments associated with this
-  // SyncData specified in |attachment_ids|. This method will not delete
-  // attachments from the server.
+  // Used to access Remote methods.
   //
   // May only be called when IsLocal() is false.
-  //
-  // |callback| will be invoked when the operation is complete (successfully or
-  // otherwise).
-  void DropAttachments(const AttachmentIdList& attachment_ids,
-                       const AttachmentService::DropCallback& callback);
-
-  // TODO(zea): Query methods for other sync properties: parent, successor, etc.
+  Remote& remote();
+  const Remote& remote() const;
 
  private:
+  friend Local;
+  friend Remote;
+
   // Necessary since we forward-declare sync_pb::SyncEntity; see
   // comments in immutable.h.
   struct ImmutableSyncEntityTraits {
@@ -178,6 +214,9 @@ class SYNC_EXPORT SyncData {
       const base::Time& remote_modification_time,
       const syncer::AttachmentServiceProxy& attachment_service);
 
+  Local local_;
+  Remote remote_;
+
   // Whether this SyncData holds valid data.
   bool is_valid_;