Update sync API to support attachments.

sync/api/sync_data.cc

Index: sync/api/sync_data.cc
diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc
index d52e04ec21300951be156dcd2ae4ec0a29fdf7f2..170e22fa7065fad057880a44fcd347a4abbbc4a2 100644
--- a/sync/api/sync_data.cc
+++ b/sync/api/sync_data.cc
@@ -10,11 +10,26 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
+#include "sync/api/attachments/attachment_service.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base_node.h"
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
 
+using syncer::Attachment;
+using syncer::AttachmentIdList;
+using syncer::AttachmentList;
+using sync_pb::AttachmentId;
+
+namespace {
+
+template<typename T>
+AttachmentId AttachmentIdOf(const Attachment& attachment) {
+  return attachment.GetId();
+}
+
+}  // namespace
+
 namespace syncer {
 
 void SyncData::ImmutableSyncEntityTraits::InitializeWrapper(
@@ -46,13 +61,16 @@ SyncData::SyncData()
     : is_valid_(false),
       id_(kInvalidId) {}
 
-SyncData::SyncData(int64 id,
-                   sync_pb::SyncEntity* entity,
-                   const base::Time& remote_modification_time)
+SyncData::SyncData(
+    int64 id,
+    sync_pb::SyncEntity* entity,
+    const base::Time& remote_modification_time,
+    const WeakHandle<syncer::AttachmentService>& attachment_service)
     : is_valid_(true),
       id_(id),
       remote_modification_time_(remote_modification_time),
-      immutable_entity_(entity) {}
+      immutable_entity_(entity),
+      attachment_service_(attachment_service) {}
 
 SyncData::~SyncData() {}
 
@@ -70,21 +88,63 @@ SyncData SyncData::CreateLocalData(
     const std::string& sync_tag,
     const std::string& non_unique_title,
     const sync_pb::EntitySpecifics& specifics) {
+  syncer::AttachmentList attachments;
+  return CreateLocalDataWithAttachments(
+      sync_tag, non_unique_title, specifics, attachments);
+}
+
+// TODO(maniscalco): What should happen if the same Attachment appears in the
+// list twice? Document the behavior and add a test case.
+//
+// Static.
+SyncData SyncData::CreateLocalDataWithAttachments(
+    const std::string& sync_tag,
+    const std::string& non_unique_title,
+    const sync_pb::EntitySpecifics& specifics,
+    const AttachmentList& attachments) {
   sync_pb::SyncEntity entity;
   entity.set_client_defined_unique_tag(sync_tag);
   entity.set_non_unique_name(non_unique_title);
   entity.mutable_specifics()->CopyFrom(specifics);
-  return SyncData(kInvalidId, &entity, base::Time());
+  std::transform(attachments.begin(),
+                 attachments.end(),
+                 RepeatedFieldBackInserter(entity.mutable_attachment_id()),
+                 AttachmentIdOf<Attachment>);
+  // TODO(maniscalco): Actually pass the attachments to the ctor and make them
+  // available to the AttachmentService once this SyncData gets passed into
+  // GenericChangeProcesso::ProcessSyncChanges.
+  return SyncData(kInvalidId,
+                  &entity,
+                  base::Time(),
+                  WeakHandle<AttachmentService>());
 }
 
 // Static.
 SyncData SyncData::CreateRemoteData(
-    int64 id, const sync_pb::EntitySpecifics& specifics,
-    const base::Time& modification_time) {
+    int64 id,
+    const sync_pb::EntitySpecifics& specifics,
+    const base::Time& modification_time,
+    const WeakHandle<AttachmentService>& attachment_service,
+    const AttachmentIdList& attachment_ids) {
   DCHECK_NE(id, kInvalidId);
   sync_pb::SyncEntity entity;
   entity.mutable_specifics()->CopyFrom(specifics);
-  return SyncData(id, &entity, modification_time);
+  std::copy(attachment_ids.begin(),
+            attachment_ids.end(),
+            google::protobuf::RepeatedFieldBackInserter(
+                entity.mutable_attachment_id()));
+  return SyncData(id, &entity, modification_time, attachment_service);
+}
+
+// Static.
+SyncData SyncData::CreateRemoteData(int64 id,
+                                    const sync_pb::EntitySpecifics& specifics,
+                                    const base::Time& modification_time) {
+  return CreateRemoteData(id,
+                          specifics,
+                          modification_time,
+                          WeakHandle<AttachmentService>(),
+                          AttachmentIdList());
 }
 
 bool SyncData::IsValid() const {
@@ -150,4 +210,28 @@ void PrintTo(const SyncData& sync_data, std::ostream* os) {
   *os << sync_data.ToString();
 }
 
+AttachmentIdList SyncData::GetAttachmentIds() const {
+  AttachmentIdList result;
+  const sync_pb::SyncEntity& entity = immutable_entity_.Get();
+  std::copy(entity.attachment_id().begin(),
+            entity.attachment_id().end(),
+            std::back_inserter(result));
+  return result;
+}
+
+void SyncData::GetAttachments(const AttachmentIdList& attachment_ids,
+                              const AttachmentService::GetCallback& callback) {
+  DCHECK(!IsLocal());
+  attachment_service_.Call(
+      FROM_HERE, &AttachmentService::GetAttachments, attachment_ids, callback);
+}
+
+void SyncData::DropAttachments(
+    const AttachmentIdList& attachment_ids,
+    const AttachmentService::DropCallback& callback) {
+  DCHECK(!IsLocal());
+  attachment_service_.Call(

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

As much as I do like the succinct-ness of WeakHandle I think we're starting to
move away from it in favor of DIY (with a task_runner + weak ptr) for
consistency with the rest of the codebase. 

But yeah, having to define your own AttServProxy object that posts via
TaskRunner to a ref-counted core object with a WeakPtr to the AttachmentService
is a bit of a pain :\

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

In some ways the explicit proxy object approach seems less error prone because,
to the caller, it looks like any other object (no need to remember a special way
of invoking methods in the right thread).  LMK if you agree and I'll drop my use
of WeakHandle.

Assuming I drop WeakHandle, I see a couple options.

1. Build the proxy/post functionality into FakeAttachmentService such that it's
transparent to users of FAS.  That is, FAS will look like a thread-safe
implementation of AS.

2. Leave FAS as it is now, not thread-safe.  Create an AttachmentServiceProxy
class that wraps an implementation of AS (like FAS).

I think I like #2 because it's more composable and will work with any
implementation of AS.

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

I agree with everything.  At the same time I wouldn't strongly oppose keeping
WeakHandle and converting things later if we ever officially decide to drop
WeakHandle.  But if I was starting from scratch I'd probably do #2.

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

SGTM.  I've gotten rid of WeakHandle and created AttachmentServiceProxy.  PTAL.

+      FROM_HERE, &AttachmentService::DropAttachments, attachment_ids, callback);
+}
+
 }  // namespace syncer