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 10 matching lines @@
 #include "sync/internal_api/public/util/weak_handle.h"
 
 namespace sync_pb {
 class EntitySpecifics;
 class SyncEntity;
 }  // namespace sync_pb
 
 namespace syncer {
 
 class AttachmentService;
+class SyncDataLocal;
+class SyncDataRemote;
 
 // A light-weight container for immutable sync data. Pass-by-value and storage
 // in STL containers are supported and encouraged if helpful.
 class SYNC_EXPORT SyncData {
  public:
   // Creates an empty and invalid SyncData.
   SyncData();
   ~SyncData();
 
   // Default copy and assign welcome.
@@ skipping 38 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
+  // Return 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).
+  // Construct a SyncDataLocal from this SyncData.
+  //
+  // Can only be called when IsLocal() is true.
+  SyncDataLocal AsLocal() const;

[tim (not reviewing), 2014/03/31 22:38:12]

Hmm. Given that there's no way (that doesn't involve friend-ing) to disallow
public creation of SyncDataLocal/Remote, it's probably best to just have one way
to build these objects and make sure it is explicit and safe.

The explicit constructors you have on SDL/R should do the trick, provided you
move the DCHECK in AsLocal/AsRemote to the appropriate constructor. I was going
to say factory functions (and a private constructor) on SDL/R would be slightly
better because you could avoid constructing an illegitimate object altogether,
but if we're using DCHECKs we shouldn't code for the failure case anyway, so the
constructors suffice.

A nice side effect is that it becomes legal to call any method on SyncData at
all times. I was going to suggest a SyncData factory function to get the same
effect until I realized the SDL/R constructors had to be public. We eliminate
"only call this when ..." cases, which was one of the original purposes of this
cl. 

[maniscalco, 2014/03/31 23:11:07]

Done.  I've removed AsLocal/AsRemote and added DCHECKs to the ctors.

+
+  // Construct a SyncDataRemote from this SyncData.
+  //
+  // Can only be called when IsLocal() is false.
+  SyncDataRemote AsRemote() const;
 
   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.
-  //
-  // May only be called when IsLocal() is true.
-  const AttachmentList& GetLocalAttachmentsForUpload() const;
-
-  // 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);
-
   // TODO(zea): Query methods for other sync properties: parent, successor, etc.
 
- private:
+ protected:
+  // These data members are protected so derived types like SyncDataLocal and
+  // SyncDataRemote can access them.
+
   // 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);
-
-  // 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.
+  // This 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_;
+
+ private:
+  // Whether this SyncData holds valid data.
+  bool is_valid_;
+
+  // Clears |entity| and |attachments|.
+  SyncData(int64 id,
+           sync_pb::SyncEntity* entity,
+           AttachmentList* attachments,
+           const base::Time& remote_modification_time,
+           const syncer::AttachmentServiceProxy& attachment_service);
+};
+
+// As SyncData going to the syncer.

[tim (not reviewing), 2014/03/31 22:38:12]

nit - A SyncData

[maniscalco, 2014/03/31 23:11:07]

Done.

+class SYNC_EXPORT SyncDataLocal : public SyncData {
+ public:
+  explicit SyncDataLocal(const SyncData& sync_data);
+  ~SyncDataLocal();
+
+  // Return a list of this SyncData's attachments.
+  const AttachmentList& GetLocalAttachmentsForUpload() const;
+
+  // Return 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;
+};
+
+// A SyncData that comes from the syncer.
+class SYNC_EXPORT SyncDataRemote : public SyncData {
+ public:
+  explicit SyncDataRemote(const SyncData& sync_data);
+  ~SyncDataRemote();
+
+  // Return the last motification time according to the server. This may be null
+  // if the SyncData represents a deleted item.
+  const base::Time& GetModifiedTime() const;
+
+  int64 GetId() const;
+
+  // Retrieve the attachments indentified by |attachment_ids|. Invoke
+  // |callback| with the requested attachments.
+  //
+  // |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.
+  //
+  // |callback| will be invoked when the operation is complete (successfully
+  // or otherwise).
+  void DropAttachments(const AttachmentIdList& attachment_ids,
+                       const AttachmentService::DropCallback& callback);
 };
 
 // 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_