Update sync API to support attachments.

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>
 
 #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;
 class SyncEntity;
 }  // namespace sync_pb
 
 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
   // identifier server-side.
   // For deletes: |datatype| must specify the datatype who node is being
   // deleted.
   // For adds/updates: the specifics must be valid and the non-unique title (can
   // be the same as sync tag) must be specfied.
   // Note: the non_unique_title is primarily for debug purposes, and will be
   // overwritten if the datatype is encrypted.
   static SyncData CreateLocalDelete(
       const std::string& sync_tag,
       ModelType datatype);
   static SyncData CreateLocalData(
       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.
   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.
@@ skipping 13 matching lines @@
   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;
 
   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:
   // 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|.
   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_;
 
   // 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_;
+
+  WeakHandle<AttachmentService> 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_