Update sync API to support attachments.

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 43f64f9185b10f798abe3086aec60fd85310d11e..66a2c1e6ca09e439b11a2922d3dc9a4eb6aa05c5 100644
--- a/sync/api/sync_data.h
+++ b/sync/api/sync_data.h
@@ -10,10 +10,15 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/stl_util.h"
 #include "base/time/time.h"
+#include "sync/api/attachments/attachment.h"
+#include "sync/api/attachments/attachment_service.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/util/immutable.h"
+#include "sync/internal_api/public/util/weak_handle.h"
 
 namespace sync_pb {
 class EntitySpecifics;
@@ -22,15 +27,17 @@ class SyncEntity;
 
 namespace syncer {
 
+class AttachmentService;
+
 // 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();
+  ~SyncData();
 
-   // Default copy and assign welcome.
+  // Default copy and assign welcome.
 
   // Helper methods for creating SyncData objects for local data.
   // The sync tag must be a string unique to this datatype and is used as a node
@@ -48,12 +55,26 @@ class SYNC_EXPORT SyncData {
       const std::string& sync_tag,
       const std::string& non_unique_title,
       const sync_pb::EntitySpecifics& specifics);
+  static SyncData CreateLocalDataWithAttachments(
+      const std::string& sync_tag,
+      const std::string& non_unique_title,
+      const sync_pb::EntitySpecifics& specifics,
+      const AttachmentList& attachments);
 
   // Helper method for creating SyncData objects originating from the syncer.
+  //
+  // TODO(maniscalco): Replace all calls to 3-arg CreateRemoteData with calls to
+  // the 4-arg version.

[tim (not reviewing), 2014/03/10 22:55:58]

For stuff like this, a recommended chromium convention is to file a bug to track
the TODO. 

[maniscalco, 2014/03/18 20:49:18]

Thanks for the reminder.  Done.

+  //

[tim (not reviewing), 2014/03/10 22:55:58]

Remove the trailing // line.

[maniscalco, 2014/03/18 20:49:18]

Done.

   static SyncData CreateRemoteData(
       int64 id,
       const sync_pb::EntitySpecifics& specifics,
-      const base::Time& last_modified_time);
+      const base::Time& last_modified_time,
+      const WeakHandle<syncer::AttachmentService>& attachment_service,
+      const AttachmentIdList& attachment_ids);
+  static SyncData CreateRemoteData(int64 id,
+                                   const sync_pb::EntitySpecifics& specifics,
+                                   const base::Time& last_modified_time);
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
   // without valid data is to use the default constructor.
@@ -87,6 +108,36 @@ class SYNC_EXPORT SyncData {
 
   std::string ToString() const;
 
+  // Return a list of this SyncData's attachment ids.
+  //
+  // The attachments may or may not be present on this host.
+  AttachmentIdList GetAttachmentIds() 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 GetAttachments(const AttachmentIdList& attachment_ids,

[tim (not reviewing), 2014/03/10 22:55:58]

As in the other file, I'm wondering if we should establish a convention to
disambiguate between "downloading" and "looking up" when we say "Get" and are
referring to an Attachment. Perhaps "Download" and "Lookup", where Lookup is
always taken to mean looking-up in a local index, never triggering any server
downloading.  If Lookup fails, items may exist somewhere, just not on this
client (yet). 

Although from the comment neither of those would apply for this method, would
they.. so maybe Get is the right call for the hybrid cases... more below.

[maniscalco, 2014/03/18 20:49:18]

Good point.  I agree.  I would like the verb to more clearly indicate the
method's semantics.  It seems like the most important thing to convey is whether
the operation is local-only or not.  With that in mind, what do you think of
this:

1. "Get" implies local-only.  No network involved.  Why not "Lookup"?  Because
"Get" matches the semantics of SCP::GetAllSyncData and the new SCP::GetSyncData
(see also my reply to your comment in SCP).

2. "Retrieve" for when it may come from the local machine or from the network. 
Why not "Download"?  Because "Download" implies network access and this
operation may or may not actually go over the network.

How does that sound?

[tim (not reviewing), 2014/03/19 22:32:48]

How about 'Get' as you say and 'GetOrDownload', then?

[maniscalco, 2014/03/25 21:40:30]

Done.

+                      const AttachmentService::GetCallback& callback);
+
+  // Drop (delete locally) 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,

[tim (not reviewing), 2014/03/10 22:55:58]

We talked about this but pointing out for posterity: At first glance, this
conflicts with the "immutability" of this class. But only if you take
"immutability" to cover the entire logical entity referred to by this SyncData
vs the physical instance of this class, and only if you take "dropping
attachment from local index" to mean an actual logical mutation of that entity,
which it really isn't.

Before, the logical and physical entities were roughly (conveniently) the same
thing, but this isn't true with attachments, and DropAttachments is where that
fact hits API in this patch. 

That works and you can argue (as I just did) that this "mutation" is justified.
An alternative that tries to make it simpler to reason about SyncData's
immutability might be to avoid putting any operations with data side-effects on
this class directly, and have a helper object (or, something like
Profile::ServiceAccessType) for when you want to modify the attachment cache.
That would work well for Drop, but less so for 'Get' in the event that you want
to actually download from server, unless we make 'Get' here be only a Lookup. 
It's also way less convenient than having Drop right here on this object, and
doesn't seem that dangerous, so I'm not sure it's worth it.
 

[maniscalco, 2014/03/18 20:49:18]

Agreed.

+                       const AttachmentService::DropCallback& callback);
+
   // TODO(zea): Query methods for other sync properties: parent, successor, etc.
 
  private:
@@ -112,7 +163,8 @@ class SYNC_EXPORT SyncData {
   // Clears |entity|.
   SyncData(int64 id,
            sync_pb::SyncEntity* entity,
-           const base::Time& remote_modification_time);
+           const base::Time& remote_modification_time,
+           const WeakHandle<syncer::AttachmentService>& attachment_service);
 
   // Whether this SyncData holds valid data.
   bool is_valid_;
@@ -126,6 +178,8 @@ class SYNC_EXPORT SyncData {
 
   // The actual shared sync entity being held.
   ImmutableSyncEntity immutable_entity_;
+
+  WeakHandle<AttachmentService> attachment_service_;
 };
 
 // gmock printer helper.