Keep track of which attachments are referenced by which sync entries.

PutAttachmentMetadata on MutableEntry now notifies the Directory when
the attachments associated with an entry change.

Give Directory::InitializeIndices a ScopedKernelLock to be consistent
and make it easier to ensure we are locking when we need to.

GenericChangeProcess passes new attachments to AttachmentService for
storage and upload.

Add an output parameter to GenericChangeProcessor's HandleActionAdd and
HandleActionUpdate methods so they can keep track of potentially new
attachments and pass them to AttachmentService for storage/upload.

Convert AttachmentService's OnSyncDataAdd to StoreAttachments.

Implement HasAttachmentNotOnServer.

BUG=348625,353303,354530,356266

Review URL: https://codereview.chromium.org/247983002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267617 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 115 insertions, 35 deletions

diff --git a/sync/api/attachments/attachment_id.cc b/sync/api/attachments/attachment_id.cc
index 6f3e0a2..e61705e 100644
--- a/sync/api/attachments/attachment_id.cc
+++ b/sync/api/attachments/attachment_id.cc
@@ -5,7 +5,7 @@
 #include "sync/api/attachments/attachment_id.h"
 
 #include "base/logging.h"
-#include "base/rand_util.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/protocol/sync.pb.h"
 
 namespace syncer {
@@ -53,10 +53,7 @@ bool AttachmentId::operator<(const AttachmentId& other) const {
 
 // Static.
 AttachmentId AttachmentId::Create() {
-  // Only requirement here is that this id must be globally unique.
-  // TODO(maniscalco): Consider making this base64 encoded.
-  sync_pb::AttachmentIdProto proto;
-  proto.set_unique_id(base::RandBytesAsString(16));
+  sync_pb::AttachmentIdProto proto = CreateAttachmentIdProto();
   return AttachmentId(&proto);
 }
 
diff --git a/sync/api/attachments/attachment_service.h b/sync/api/attachments/attachment_service.h
index 81a1a63..9e5e25d 100644
--- a/sync/api/attachments/attachment_service.h
+++ b/sync/api/attachments/attachment_service.h
@@ -38,11 +38,22 @@ class SYNC_EXPORT AttachmentService {
   // The result of a DropAttachments operation.
   enum DropResult {
     DROP_SUCCESS,            // No error, all attachments dropped.
-    DROP_UNSPECIFIED_ERROR,  // An unspecified error occurred.
+    DROP_UNSPECIFIED_ERROR,  // An unspecified error occurred. Some or all
+                             // attachments may not have been dropped.
   };
 
   typedef base::Callback<void(const DropResult&)> DropCallback;
 
+  // The result of a StoreAttachments operation.
+  enum StoreResult {
+    STORE_SUCCESS,            // No error, all attachments stored (at least
+                              // locally).
+    STORE_UNSPECIFIED_ERROR,  // An unspecified error occurred. Some or all
+                              // attachments may not have been stored.
+  };
+
+  typedef base::Callback<void(const StoreResult&)> StoreCallback;
+
   AttachmentService();
   virtual ~AttachmentService();
 
@@ -55,10 +66,12 @@ class SYNC_EXPORT AttachmentService {
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                const DropCallback& callback) = 0;
 
-  // This method should be called when a SyncData is about to be added to the
-  // sync database so we have a chance to persist the Attachment locally and
-  // schedule it for upload to the sync server.
-  virtual void OnSyncDataAdd(const SyncData& sync_data) = 0;
+  // Store |attachments| on device and (eventually) upload them to the server.
+  //
+  // Invokes |callback| once the attachments have been written to device
+  // storage.
+  virtual void StoreAttachments(const AttachmentList& attachments,
+                                const StoreCallback& callback) = 0;
 
   // This method should be called when a SyncData is about to be deleted from
   // the sync database so we can remove any unreferenced attachments from local
diff --git a/sync/api/attachments/attachment_service_proxy.cc b/sync/api/attachments/attachment_service_proxy.cc
index 9bc40e5..6590033 100644
--- a/sync/api/attachments/attachment_service_proxy.cc
+++ b/sync/api/attachments/attachment_service_proxy.cc
@@ -35,6 +35,15 @@ void ProxyDropCallback(
   task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
 }
 
+// Invokes |callback| with |result| and |attachments| in the |task_runner|
+// thread.
+void ProxyStoreCallback(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+    const AttachmentService::StoreCallback& callback,
+    const AttachmentService::StoreResult& result) {
+  task_runner->PostTask(FROM_HERE, base::Bind(callback, result));
+}
+
 }  // namespace
 
 AttachmentServiceProxy::AttachmentServiceProxy() {
@@ -85,11 +94,17 @@ void AttachmentServiceProxy::DropAttachments(
                                             proxy_callback));
 }
 
-void AttachmentServiceProxy::OnSyncDataAdd(const SyncData& sync_data) {
+void AttachmentServiceProxy::StoreAttachments(const AttachmentList& attachments,
+                                              const StoreCallback& callback) {
   DCHECK(wrapped_task_runner_);
+  StoreCallback proxy_callback = base::Bind(
+      &ProxyStoreCallback, base::MessageLoopProxy::current(), callback);
   wrapped_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&AttachmentService::OnSyncDataAdd, core_, sync_data));
+      base::Bind(&AttachmentService::StoreAttachments,
+                 core_,
+                 attachments,
+                 proxy_callback));
 }
 
 void AttachmentServiceProxy::OnSyncDataDelete(const SyncData& sync_data) {
@@ -137,11 +152,13 @@ void AttachmentServiceProxy::Core::DropAttachments(
   wrapped_->DropAttachments(attachment_ids, callback);
 }
 
-void AttachmentServiceProxy::Core::OnSyncDataAdd(const SyncData& sync_data) {
+void AttachmentServiceProxy::Core::StoreAttachments(
+    const AttachmentList& attachments,
+    const StoreCallback& callback) {
   if (!wrapped_) {
     return;
   }
-  wrapped_->OnSyncDataAdd(sync_data);
+  wrapped_->StoreAttachments(attachments, callback);
 }
 
 void AttachmentServiceProxy::Core::OnSyncDataDelete(const SyncData& sync_data) {
diff --git a/sync/api/attachments/attachment_service_proxy.h b/sync/api/attachments/attachment_service_proxy.h
index 7085625..462cdda 100644
--- a/sync/api/attachments/attachment_service_proxy.h
+++ b/sync/api/attachments/attachment_service_proxy.h
@@ -58,7 +58,8 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
       const GetOrDownloadCallback& callback) OVERRIDE;
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                const DropCallback& callback) OVERRIDE;
-  virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+  virtual void StoreAttachments(const AttachmentList& attachment,
+                                const StoreCallback& callback) OVERRIDE;
   virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
   virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
                                 const SyncData& updated_sync_data) OVERRIDE;
@@ -89,7 +90,8 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
         const GetOrDownloadCallback& callback) OVERRIDE;
     virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                  const DropCallback& callback) OVERRIDE;
-    virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+    virtual void StoreAttachments(const AttachmentList& attachment,
+                                  const StoreCallback& callback) OVERRIDE;
     virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
     virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
                                   const SyncData& updated_sync_data) OVERRIDE;
diff --git a/sync/api/attachments/attachment_service_proxy_unittest.cc b/sync/api/attachments/attachment_service_proxy_unittest.cc
index f6e3502..08c499e 100644
--- a/sync/api/attachments/attachment_service_proxy_unittest.cc
+++ b/sync/api/attachments/attachment_service_proxy_unittest.cc
@@ -56,9 +56,12 @@ class StubAttachmentService : public AttachmentService,
         FROM_HERE, base::Bind(callback, AttachmentService::DROP_SUCCESS));
   }
 
-  virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE {
+  virtual void StoreAttachments(const AttachmentList& attachments,
+                                const StoreCallback& callback) OVERRIDE {
     CalledOnValidThread();
     Increment();
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, AttachmentService::STORE_SUCCESS));
   }
 
   virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE {
@@ -119,8 +122,11 @@ class AttachmentServiceProxyTest : public testing::Test,
                    base::Unretained(this));
     callback_drop = base::Bind(&AttachmentServiceProxyTest::IncrementDrop,
                                base::Unretained(this));
+    callback_store = base::Bind(&AttachmentServiceProxyTest::IncrementStore,
+                                base::Unretained(this));
     count_callback_get_or_download = 0;
     count_callback_drop = 0;
+    count_callback_store = 0;
   }
 
   virtual void TearDown()
@@ -147,6 +153,12 @@ class AttachmentServiceProxyTest : public testing::Test,
     ++count_callback_drop;
   }
 
+  // a StoreCallback
+  void IncrementStore(const AttachmentService::StoreResult&) {
+    CalledOnValidThread();
+    ++count_callback_store;
+  }
+
   void WaitForStubThread() {
     base::WaitableEvent done(false, false);
     stub_thread->message_loop()->PostTask(
@@ -165,21 +177,23 @@ class AttachmentServiceProxyTest : public testing::Test,
 
   AttachmentService::GetOrDownloadCallback callback_get_or_download;
   AttachmentService::DropCallback callback_drop;
+  AttachmentService::StoreCallback callback_store;
 
   // number of times callback_get_or_download was invoked
   int count_callback_get_or_download;
   // number of times callback_drop was invoked
   int count_callback_drop;
+  // number of times callback_store was invoked
+  int count_callback_store;
 };
 
 // Verify that each of AttachmentServiceProxy's regular methods (those that
 // don't take callbacks) are invoked on the stub.
 TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) {
-  proxy->OnSyncDataAdd(sync_data);
   proxy->OnSyncDataDelete(sync_data_delete);
   proxy->OnSyncDataUpdate(AttachmentIdList(), sync_data);
   WaitForStubThread();
-  EXPECT_EQ(3, stub->GetCallCount());
+  EXPECT_EQ(2, stub->GetCallCount());
 }
 
 // Verify that each of AttachmentServiceProxy's callback methods (those that
@@ -188,9 +202,10 @@ TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) {
 TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) {
   proxy->GetOrDownloadAttachments(AttachmentIdList(), callback_get_or_download);
   proxy->DropAttachments(AttachmentIdList(), callback_drop);
+  proxy->StoreAttachments(AttachmentList(), callback_store);
   // Wait for the posted calls to execute in the stub thread.
   WaitForStubThread();
-  EXPECT_EQ(2, stub->GetCallCount());
+  EXPECT_EQ(3, stub->GetCallCount());
   // At this point the stub thread has finished executed the calls. However, the
   // result callbacks it has posted may not have executed yet. Wait a second
   // time to ensure the stub thread has executed the posted result callbacks.
@@ -199,6 +214,7 @@ TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) {
   loop.RunUntilIdle();
   EXPECT_EQ(1, count_callback_get_or_download);
   EXPECT_EQ(1, count_callback_drop);
+  EXPECT_EQ(1, count_callback_store);
 }
 
 // Verify that it's safe to use an AttachmentServiceProxy even after its wrapped
diff --git a/sync/api/attachments/fake_attachment_service.cc b/sync/api/attachments/fake_attachment_service.cc
index b9eb88e..5cb01e5 100644
--- a/sync/api/attachments/fake_attachment_service.cc
+++ b/sync/api/attachments/fake_attachment_service.cc
@@ -51,10 +51,15 @@ void FakeAttachmentService::DropAttachments(
                                      callback));
 }
 
-void FakeAttachmentService::OnSyncDataAdd(const SyncData& sync_data) {
+void FakeAttachmentService::StoreAttachments(const AttachmentList& attachments,
+                                             const StoreCallback& callback) {
   DCHECK(CalledOnValidThread());
-  // TODO(maniscalco): Ensure the linked attachments get persisted in local
-  // storage and schedule them for upload to the server (bug 356351).
+  attachment_store_->Write(attachments,
+                           base::Bind(&FakeAttachmentService::WriteDone,
+                                      weak_ptr_factory_.GetWeakPtr(),
+                                      callback));
+  // TODO(maniscalco): Ensure the linked attachments are schedule for upload to
+  // the server (bug 356351).
 }
 
 void FakeAttachmentService::OnSyncDataDelete(const SyncData& sync_data) {
@@ -99,4 +104,16 @@ void FakeAttachmentService::DropDone(const DropCallback& callback,
                                          base::Bind(callback, drop_result));
 }
 
+void FakeAttachmentService::WriteDone(const StoreCallback& callback,
+                                      const AttachmentStore::Result& result) {
+  AttachmentService::StoreResult store_result =
+      AttachmentService::STORE_UNSPECIFIED_ERROR;
+  if (result == AttachmentStore::SUCCESS) {
+    store_result = AttachmentService::STORE_SUCCESS;
+  }
+  // TODO(maniscalco): Deal with case where an error occurred (bug 361251).
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         base::Bind(callback, store_result));
+}
+
 }  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_service.h b/sync/api/attachments/fake_attachment_service.h
index eb97c52..5bc443a 100644
--- a/sync/api/attachments/fake_attachment_service.h
+++ b/sync/api/attachments/fake_attachment_service.h
@@ -29,7 +29,8 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService,
       OVERRIDE;
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                const DropCallback& callback) OVERRIDE;
-  virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+  virtual void StoreAttachments(const AttachmentList& attachments,
+                                const StoreCallback& callback) OVERRIDE;
   virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
   virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
                                 const SyncData& updated_sync_data) OVERRIDE;
@@ -40,6 +41,8 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService,
                 scoped_ptr<AttachmentMap> attachments);
   void DropDone(const DropCallback& callback,
                 const AttachmentStore::Result& result);
+  void WriteDone(const StoreCallback& callback,
+                 const AttachmentStore::Result& result);
 
   const scoped_ptr<AttachmentStore> attachment_store_;
   // Must be last data member.
diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc
index fa48ea5..35a21f9 100644
--- a/sync/api/sync_data.cc
+++ b/sync/api/sync_data.cc
@@ -36,6 +36,21 @@ syncer::AttachmentId ProtoToId(const sync_pb::AttachmentIdProto& proto) {
   return syncer::AttachmentId::CreateFromProto(proto);
 }
 
+// Return true iff |attachments| contains one or more elements with the same
+// AttachmentId.
+bool ContainsDuplicateAttachments(const syncer::AttachmentList& attachments) {
+  std::set<syncer::AttachmentId> id_set;
+  AttachmentList::const_iterator iter = attachments.begin();
+  AttachmentList::const_iterator end = attachments.end();
+  for (; iter != end; ++iter) {
+    if (id_set.find(iter->GetId()) != id_set.end()) {
+      return true;
+    }
+    id_set.insert(iter->GetId());
+  }
+  return false;
+}
+
 }  // namespace
 
 namespace syncer {
@@ -96,15 +111,13 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag,
       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) {
+  DCHECK(!ContainsDuplicateAttachments(attachments));
   sync_pb::SyncEntity entity;
   entity.set_client_defined_unique_tag(sync_tag);
   entity.set_non_unique_name(non_unique_title);
@@ -113,9 +126,6 @@ SyncData SyncData::CreateLocalDataWithAttachments(
                  attachments.end(),
                  RepeatedFieldBackInserter(entity.mutable_attachment_id()),
                  AttachmentToProto);
-  // TODO(maniscalco): Actually pass the attachments to the ctor and make them
-  // available to the AttachmentService once this SyncData gets passed into
-  // GenericChangeProcesso::ProcessSyncChanges (bug 354530).
   AttachmentList copy_of_attachments(attachments);
   return SyncData(kInvalidId,
                   &entity,
diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h
index bb99b74..8795141 100644
--- a/sync/api/sync_data.h
+++ b/sync/api/sync_data.h
@@ -43,14 +43,19 @@ class SYNC_EXPORT SyncData {
   // 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
+  //
+  // |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.
+  //
+  // For adds/updates: |specifics| must be valid and |non_unique_title| (can be
+  // the same as |sync_tag|) must be specfied.  Note: |non_unique_title| is
+  // primarily for debug purposes, and will be overwritten if the datatype is
+  // encrypted.
+  //
+  // For data with attachments: |attachments| must not contain duplicates.
   static SyncData CreateLocalDelete(
       const std::string& sync_tag,
       ModelType datatype);