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

sync/api/sync_data.h

 // Copyright 2012 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 SYNC_API_SYNC_DATA_H_
 #define SYNC_API_SYNC_DATA_H_
 
 #include <iosfwd>
 #include <string>
 #include <vector>
@@ skipping 68 matching lines @@
   // without valid data is to use the default constructor.
   bool IsValid() const;
 
   // Return the datatype we're holding information about. Derived from the sync
   // datatype specifics.
   ModelType 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
-  // 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;
 
   // TODO(maniscalco): Reduce the dependence on knowing whether a SyncData is
   // local (in the IsLocal() == true sense) or remote.  Make it harder for users
   // of SyncData to accidentally call local-only methods on a remote SyncData
   // (bug 357305).
 
   std::string ToString() const;
 
   // Return a list of this SyncData's attachment ids.
   //
   // The attachments may or may not be present on this device.
   AttachmentIdList GetAttachmentIds() const;
 
-  // Return a list of this SyncData's attachments.
+  // TODO(zea): Query methods for other sync properties: parent, successor, etc.
+
+  // 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.
   //
   // May only be called when IsLocal() is true.
-  const AttachmentList& GetLocalAttachmentsForUpload() const;
+  const Local& local() const;
 
-  // Retrieve the attachments indentified by |attachment_ids|. Invoke |callback|
-  // with the requested attachments.
+  // 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_;
+  };
+
+  // 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).
-  //
-  // 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);
-
-  // 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 {
     typedef sync_pb::SyncEntity* Wrapper;
 
     static void InitializeWrapper(Wrapper* wrapper);
 
     static void DestroyWrapper(Wrapper* wrapper);
 
     static const sync_pb::SyncEntity& Unwrap(const Wrapper& wrapper);
 
     static sync_pb::SyncEntity* UnwrapMutable(Wrapper* wrapper);
 
     static void Swap(sync_pb::SyncEntity* t1, sync_pb::SyncEntity* t2);
   };
 
   typedef Immutable<sync_pb::SyncEntity, ImmutableSyncEntityTraits>
       ImmutableSyncEntity;
 
   // Clears |entity| and |attachments|.
   SyncData(
       int64 id,
       sync_pb::SyncEntity* entity,
       AttachmentList* attachments,
       const base::Time& remote_modification_time,
       const syncer::AttachmentServiceProxy& attachment_service);
 
+  Local local_;
+  Remote remote_;
+
   // Whether this SyncData holds valid data.
   bool is_valid_;
 
   // Equal to kInvalidId iff this is local.
   int64 id_;
 
   // This is only valid if IsLocal() is false, and may be null if the
   // SyncData represents a deleted item.
   base::Time remote_modification_time_;
 
   // The actual shared sync entity being held.
   ImmutableSyncEntity immutable_entity_;
 
   Immutable<AttachmentList> attachments_;
 
   AttachmentServiceProxy attachment_service_;
 };
 
 // gmock printer helper.
 void PrintTo(const SyncData& sync_data, std::ostream* os);
 
 typedef std::vector<SyncData> SyncDataList;
 
 }  // namespace syncer
 
 #endif  // SYNC_API_SYNC_DATA_H_