Begin connecting AttachmentStore to AttachmentService.

AttachmentStore - Refactor interface to operate on lists of attachments
instead of individual attachments.  Operating on lists makes it easier
to integrate with AttachmentService because AttachmentService operates
on lists.

AttachmentService - Simplify result enums to model only success and
error (no more "not found"). In the future, once we have a better idea
of what error cases we want to differentiate, we may expand the enums.

AttachmentService - Return AttachmentMap by scoped_ptr to avoid an
unnecessary copy.

BUG=356351

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

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

9 files changed, 320 insertions, 142 deletions

diff --git a/sync/api/attachments/attachment_service.h b/sync/api/attachments/attachment_service.h
index d148b0c..81a1a63 100644
--- a/sync/api/attachments/attachment_service.h
+++ b/sync/api/attachments/attachment_service.h
@@ -20,22 +20,24 @@ class SyncData;
 //
 // Outside of sync code, AttachmentService shouldn't be used directly. Instead
 // use the functionality provided by SyncData and SyncChangeProcessor.
+//
+// Destroying this object does not necessarily cancel outstanding async
+// operations. If you need cancel like semantics, use WeakPtr in the callbacks.
 class SYNC_EXPORT AttachmentService {
  public:
   // The result of a GetOrDownloadAttachments operation.
   enum GetOrDownloadResult {
-    GET_SUCCESS,            // No error.
-    GET_NOT_FOUND,          // Attachment was not found or does not exist.
+    GET_SUCCESS,            // No error, all attachments returned.
     GET_UNSPECIFIED_ERROR,  // An unspecified error occurred.
   };
 
   typedef base::Callback<
-      void(const GetOrDownloadResult&, const AttachmentMap& attachments)>
+      void(const GetOrDownloadResult&, scoped_ptr<AttachmentMap> attachments)>
       GetOrDownloadCallback;
 
   // The result of a DropAttachments operation.
   enum DropResult {
-    DROP_SUCCESS,            // No error.
+    DROP_SUCCESS,            // No error, all attachments dropped.
     DROP_UNSPECIFIED_ERROR,  // An unspecified error occurred.
   };
 
diff --git a/sync/api/attachments/attachment_service_proxy.cc b/sync/api/attachments/attachment_service_proxy.cc
index b95a41f..9bc40e5 100644
--- a/sync/api/attachments/attachment_service_proxy.cc
+++ b/sync/api/attachments/attachment_service_proxy.cc
@@ -21,8 +21,9 @@ void ProxyGetOrDownloadCallback(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     const AttachmentService::GetOrDownloadCallback& callback,
     const AttachmentService::GetOrDownloadResult& result,
-    const AttachmentMap& attachments) {
-  task_runner->PostTask(FROM_HERE, base::Bind(callback, result, attachments));
+    scoped_ptr<AttachmentMap> attachments) {
+  task_runner->PostTask(
+      FROM_HERE, base::Bind(callback, result, base::Passed(&attachments)));
 }
 
 // Invokes |callback| with |result| and |attachments| in the |task_runner|
diff --git a/sync/api/attachments/attachment_service_proxy_unittest.cc b/sync/api/attachments/attachment_service_proxy_unittest.cc
index eed5e6b..f6e3502 100644
--- a/sync/api/attachments/attachment_service_proxy_unittest.cc
+++ b/sync/api/attachments/attachment_service_proxy_unittest.cc
@@ -40,10 +40,12 @@ class StubAttachmentService : public AttachmentService,
       OVERRIDE {
     CalledOnValidThread();
     Increment();
+    scoped_ptr<AttachmentMap> attachments(new AttachmentMap());
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
-        base::Bind(
-            callback, AttachmentService::GET_NOT_FOUND, AttachmentMap()));
+        base::Bind(callback,
+                   AttachmentService::GET_UNSPECIFIED_ERROR,
+                   base::Passed(&attachments)));
   }
 
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
@@ -134,7 +136,7 @@ class AttachmentServiceProxyTest : public testing::Test,
 
   // a GetOrDownloadCallback
   void IncrementGetOrDownload(const AttachmentService::GetOrDownloadResult&,
-                              const AttachmentMap&) {
+                              scoped_ptr<AttachmentMap>) {
     CalledOnValidThread();
     ++count_callback_get_or_download;
   }
diff --git a/sync/api/attachments/attachment_store.h b/sync/api/attachments/attachment_store.h
index 1d623ce..d86cd1b 100644
--- a/sync/api/attachments/attachment_store.h
+++ b/sync/api/attachments/attachment_store.h
@@ -8,6 +8,8 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "sync/api/attachments/attachment.h"
+#include "sync/api/attachments/attachment_id.h"
 #include "sync/base/sync_export.h"
 
 namespace base {
@@ -20,6 +22,9 @@ class Attachment;
 class AttachmentId;
 
 // A place to locally store and access Attachments.
+//
+// Destroying this object does not necessarily cancel outstanding async
+// operations. If you need cancel like semantics, use WeakPtr in the callbacks.
 class SYNC_EXPORT AttachmentStore {
  public:
   AttachmentStore();
@@ -29,36 +34,50 @@ class SYNC_EXPORT AttachmentStore {
   // resumable transfers (bug 353292).
 
   enum Result {
-    SUCCESS,            // No error.
-    NOT_FOUND,          // Attachment was not found or does not exist.
-    UNSPECIFIED_ERROR,  // An unspecified error occurred.
+    SUCCESS,            // No error, all completed successfully.
+    UNSPECIFIED_ERROR,  // An unspecified error occurred for one or more items.
   };
 
-  typedef base::Callback<void(const Result&, scoped_ptr<Attachment>)>
+  typedef base::Callback<void(const Result&, scoped_ptr<AttachmentMap>)>
       ReadCallback;
-  typedef base::Callback<void(const Result&, const AttachmentId& id)>
-      WriteCallback;
+  typedef base::Callback<void(const Result&)> WriteCallback;
   typedef base::Callback<void(const Result&)> DropCallback;
 
-  // Asynchronously reads the attachment identified by |id|.
+  // Asynchronously reads the attachments identified by |ids|.
   //
-  // |callback| will be invoked when finished. If the attachment does not exist,
-  // |callback|'s Result will be NOT_FOUND and |callback|'s attachment will be
-  // null.
-  virtual void Read(const AttachmentId& id, const ReadCallback& callback) = 0;
+  // |callback| will be invoked when finished. If any of the attachments do not
+  // exist or could not be read, |callback|'s Result will be UNSPECIFIED_ERROR
+  // and |callback| may receive a partial result. That is, AttachmentMap may be
+  // empty or may contain the attachments that were read successfully.
+  //
+  // Reads on individual attachments are treated atomically; |callback| will not
+  // read only part of an attachment.
+  virtual void Read(const AttachmentIdList& ids,
+                    const ReadCallback& callback) = 0;
 
-  // Asynchronously writes |bytes| to the store.
+  // Asynchronously writes |attachments| to the store.
+  //
+  // Will not overwrite stored attachments. Attempting to overwrite an
+  // attachment that already exists is not an error.
   //
-  // |callback| will be invoked when finished.
-  virtual void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
+  // |callback| will be invoked when finished. If any of the attachments could
+  // not be written |callback|'s Result will be UNSPECIFIED_ERROR. When this
+  // happens, some or none of the attachments may have been written
+  // successfully.
+  virtual void Write(const AttachmentList& attachments,
                      const WriteCallback& callback) = 0;
 
-  // Asynchronously drops the attchment with the given id from this store.
+  // Asynchronously drops |attchments| from this store.
+  //
+  // This does not remove attachments from the server.
   //
-  // This does not remove the attachment from the server. |callback| will be
-  // invoked when finished. If the attachment does not exist, |callback|'s
-  // Result will be NOT_FOUND.
-  virtual void Drop(const AttachmentId& id, const DropCallback& callback) = 0;
+  // |callback| will be invoked when finished. Attempting to drop an attachment
+  // that does not exist is not an error. If any of the existing attachment
+  // could not be dropped, |callback|'s Result will be UNSPECIFIED_ERROR. When
+  // this happens, some or none of the attachments may have been dropped
+  // successfully.
+  virtual void Drop(const AttachmentIdList& ids,
+                    const DropCallback& callback) = 0;
 };
 
 }  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_service.cc b/sync/api/attachments/fake_attachment_service.cc
index 86dfaeb..8e9e867 100644
--- a/sync/api/attachments/fake_attachment_service.cc
+++ b/sync/api/attachments/fake_attachment_service.cc
@@ -4,14 +4,17 @@
 
 #include "sync/api/attachments/fake_attachment_service.h"
 
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
 #include "base/test/test_simple_task_runner.h"
+#include "sync/api/attachments/attachment.h"
 #include "sync/api/attachments/fake_attachment_store.h"
 
 namespace syncer {
 
 FakeAttachmentService::FakeAttachmentService(
     scoped_ptr<AttachmentStore> attachment_store)
-    : attachment_store_(attachment_store.Pass()) {
+    : attachment_store_(attachment_store.Pass()), weak_ptr_factory_(this) {
   DCHECK(CalledOnValidThread());
   DCHECK(attachment_store_);
 }
@@ -34,16 +37,20 @@ void FakeAttachmentService::GetOrDownloadAttachments(
     const AttachmentIdList& attachment_ids,
     const GetOrDownloadCallback& callback) {
   DCHECK(CalledOnValidThread());
-  // TODO(maniscalco): Fire off a bunch of AttachmentStore operations.  Invoke
-  // callback after all have completed (bug 356351).
+  attachment_store_->Read(attachment_ids,
+                          base::Bind(&FakeAttachmentService::ReadDone,
+                                     weak_ptr_factory_.GetWeakPtr(),
+                                     callback));
 }
 
 void FakeAttachmentService::DropAttachments(
     const AttachmentIdList& attachment_ids,
     const DropCallback& callback) {
   DCHECK(CalledOnValidThread());
-  // TODO(maniscalco): Fire off a bunch of AttachmentStore operations.  Invoke
-  // callback after all have completed (bug 356351).
+  attachment_store_->Drop(attachment_ids,
+                          base::Bind(&FakeAttachmentService::DropDone,
+                                     weak_ptr_factory_.GetWeakPtr(),
+                                     callback));
 }
 
 void FakeAttachmentService::OnSyncDataAdd(const SyncData& sync_data) {
@@ -69,4 +76,29 @@ void FakeAttachmentService::OnSyncDataUpdate(
   // attachments from local storage (bug 356351).
 }
 
+void FakeAttachmentService::ReadDone(const GetOrDownloadCallback& callback,
+                                     const AttachmentStore::Result& result,
+                                     scoped_ptr<AttachmentMap> attachments) {
+  AttachmentService::GetOrDownloadResult get_result =
+      AttachmentService::GET_UNSPECIFIED_ERROR;
+  if (result == AttachmentStore::SUCCESS) {
+    get_result = AttachmentService::GET_SUCCESS;
+  }
+  // TODO(maniscalco): Deal with case where an error occurred (bug 361251).
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(callback, get_result, base::Passed(&attachments)));
+}
+
+void FakeAttachmentService::DropDone(const DropCallback& callback,
+                                     const AttachmentStore::Result& result) {
+  AttachmentService::DropResult drop_result =
+      AttachmentService::DROP_UNSPECIFIED_ERROR;
+  if (result == AttachmentStore::SUCCESS) {
+    drop_result = AttachmentService::DROP_SUCCESS;
+  }
+  // TODO(maniscalco): Deal with case where an error occurred (bug 361251).
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         base::Bind(callback, drop_result));
+}
+
 }  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_service.h b/sync/api/attachments/fake_attachment_service.h
index 3bbd5d6d..eb97c52 100644
--- a/sync/api/attachments/fake_attachment_service.h
+++ b/sync/api/attachments/fake_attachment_service.h
@@ -35,7 +35,15 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService,
                                 const SyncData& updated_sync_data) OVERRIDE;
 
  private:
+  void ReadDone(const GetOrDownloadCallback& callback,
+                const AttachmentStore::Result& result,
+                scoped_ptr<AttachmentMap> attachments);
+  void DropDone(const DropCallback& callback,
+                const AttachmentStore::Result& result);
+
   const scoped_ptr<AttachmentStore> attachment_store_;
+  // Must be last data member.
+  base::WeakPtrFactory<FakeAttachmentService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeAttachmentService);
 };
diff --git a/sync/api/attachments/fake_attachment_store.cc b/sync/api/attachments/fake_attachment_store.cc
index 53e42a2..18dd927 100644
--- a/sync/api/attachments/fake_attachment_store.cc
+++ b/sync/api/attachments/fake_attachment_store.cc
@@ -22,10 +22,9 @@ class FakeAttachmentStore::Backend
   Backend(
       const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner);
 
-  void Read(const AttachmentId& id, const ReadCallback& callback);
-  void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
-             const WriteCallback& callback);
-  void Drop(const AttachmentId& id, const DropCallback& callback);
+  void Read(const AttachmentIdList& ids, const ReadCallback& callback);
+  void Write(const AttachmentList& attachments, const WriteCallback& callback);
+  void Drop(const AttachmentIdList& ids, const DropCallback& callback);
 
  private:
   friend class base::RefCountedThreadSafe<Backend>;
@@ -42,36 +41,48 @@ FakeAttachmentStore::Backend::Backend(
 
 FakeAttachmentStore::Backend::~Backend() {}
 
-void FakeAttachmentStore::Backend::Read(const AttachmentId& id,
+void FakeAttachmentStore::Backend::Read(const AttachmentIdList& ids,
                                         const ReadCallback& callback) {
-  AttachmentMap::iterator iter = attachments_.find(id);
-  scoped_ptr<Attachment> attachment;
-  Result result = NOT_FOUND;
-  if (iter != attachments_.end()) {
-    attachment.reset(new Attachment(iter->second));
-    result = SUCCESS;
+  Result result_code = SUCCESS;
+  AttachmentIdList::const_iterator id_iter = ids.begin();
+  AttachmentIdList::const_iterator id_end = ids.end();
+  scoped_ptr<AttachmentMap> result_map(new AttachmentMap);
+  for (; id_iter != id_end; ++id_iter) {
+    const AttachmentId& id = *id_iter;
+    syncer::AttachmentMap::iterator attachment_iter =
+        attachments_.find(*id_iter);
+    if (attachment_iter != attachments_.end()) {
+      const Attachment& attachment = attachment_iter->second;
+      result_map->insert(std::make_pair(id, attachment));
+    } else {
+      result_code = UNSPECIFIED_ERROR;
+      break;
+    }
   }
   frontend_task_runner_->PostTask(
-      FROM_HERE, base::Bind(callback, result, base::Passed(&attachment)));
+      FROM_HERE, base::Bind(callback, result_code, base::Passed(&result_map)));
 }
 
-void FakeAttachmentStore::Backend::Write(
-    const scoped_refptr<base::RefCountedMemory>& bytes,
-    const WriteCallback& callback) {
-  Attachment attachment = Attachment::Create(bytes);
-  AttachmentId attachment_id(attachment.GetId());
-  attachments_.insert(AttachmentMap::value_type(attachment_id, attachment));
-  frontend_task_runner_->PostTask(FROM_HERE,
-                                  base::Bind(callback, SUCCESS, attachment_id));
+void FakeAttachmentStore::Backend::Write(const AttachmentList& attachments,
+                                         const WriteCallback& callback) {
+  AttachmentList::const_iterator iter = attachments.begin();
+  AttachmentList::const_iterator end = attachments.end();
+  for (; iter != end; ++iter) {
+    attachments_.insert(std::make_pair(iter->GetId(), *iter));
+  }
+  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
 }
 
-void FakeAttachmentStore::Backend::Drop(const AttachmentId& id,
+void FakeAttachmentStore::Backend::Drop(const AttachmentIdList& ids,
                                         const DropCallback& callback) {
-  Result result = NOT_FOUND;
-  AttachmentMap::iterator iter = attachments_.find(id);
-  if (iter != attachments_.end()) {
-    attachments_.erase(iter);
-    result = SUCCESS;
+  Result result = SUCCESS;
+  AttachmentIdList::const_iterator ids_iter = ids.begin();
+  AttachmentIdList::const_iterator ids_end = ids.end();
+  for (; ids_iter != ids_end; ++ids_iter) {
+    AttachmentMap::iterator attachments_iter = attachments_.find(*ids_iter);
+    if (attachments_iter != attachments_.end()) {
+      attachments_.erase(attachments_iter);
+    }
   }
   frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
 }
@@ -83,27 +94,28 @@ FakeAttachmentStore::FakeAttachmentStore(
 
 FakeAttachmentStore::~FakeAttachmentStore() {}
 
-void FakeAttachmentStore::Read(const AttachmentId& id,
+void FakeAttachmentStore::Read(const AttachmentIdList& ids,
                                const ReadCallback& callback) {
   backend_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&FakeAttachmentStore::Backend::Read, backend_, id, callback));
+      base::Bind(&FakeAttachmentStore::Backend::Read, backend_, ids, callback));
 }
 
-void FakeAttachmentStore::Write(
-    const scoped_refptr<base::RefCountedMemory>& bytes,
-    const WriteCallback& callback) {
+void FakeAttachmentStore::Write(const AttachmentList& attachments,
+                                const WriteCallback& callback) {
   backend_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(
-          &FakeAttachmentStore::Backend::Write, backend_, bytes, callback));
+      base::Bind(&FakeAttachmentStore::Backend::Write,
+                 backend_,
+                 attachments,
+                 callback));
 }
 
-void FakeAttachmentStore::Drop(const AttachmentId& id,
+void FakeAttachmentStore::Drop(const AttachmentIdList& ids,
                                const DropCallback& callback) {
   backend_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&FakeAttachmentStore::Backend::Drop, backend_, id, callback));
+      base::Bind(&FakeAttachmentStore::Backend::Drop, backend_, ids, callback));
 }
 
 }  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_store.h b/sync/api/attachments/fake_attachment_store.h
index 41fd927..35fad17 100644
--- a/sync/api/attachments/fake_attachment_store.h
+++ b/sync/api/attachments/fake_attachment_store.h
@@ -39,11 +39,11 @@ class SYNC_EXPORT FakeAttachmentStore : public AttachmentStore {
   virtual ~FakeAttachmentStore();
 
   // AttachmentStore implementation.
-  virtual void Read(const AttachmentId& id,
+  virtual void Read(const AttachmentIdList& id,
                     const ReadCallback& callback) OVERRIDE;
-  virtual void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
+  virtual void Write(const AttachmentList& attachments,
                      const WriteCallback& callback) OVERRIDE;
-  virtual void Drop(const AttachmentId& id,
+  virtual void Drop(const AttachmentIdList& id,
                     const DropCallback& callback) OVERRIDE;
 
  private:
diff --git a/sync/api/attachments/fake_attachment_store_unittest.cc b/sync/api/attachments/fake_attachment_store_unittest.cc
index e560697..b8dc2a1 100644
--- a/sync/api/attachments/fake_attachment_store_unittest.cc
+++ b/sync/api/attachments/fake_attachment_store_unittest.cc
@@ -14,124 +14,226 @@
 
 namespace syncer {
 
-const char kTestData[] = "some data";
+const char kTestData1[] = "test data 1";
+const char kTestData2[] = "test data 2";
 
 class FakeAttachmentStoreTest : public testing::Test {
  protected:
-  FakeAttachmentStoreTest() : id(AttachmentId::Create()) {}
+  base::MessageLoop message_loop;
+  FakeAttachmentStore store;
+  AttachmentStore::Result result;
+  scoped_ptr<AttachmentMap> attachments;
+
+  AttachmentStore::ReadCallback read_callback;
+  AttachmentStore::WriteCallback write_callback;
+  AttachmentStore::DropCallback drop_callback;
+
+  scoped_refptr<base::RefCountedString> some_data1;
+  scoped_refptr<base::RefCountedString> some_data2;
+
+  FakeAttachmentStoreTest() : store(base::MessageLoopProxy::current()) {}
 
   virtual void SetUp() {
     Clear();
-    read_callback =
-        base::Bind(&FakeAttachmentStoreTest::CopyAttachmentAndResult,
-                   base::Unretained(this),
-                   &result,
-                   &attachment);
-    write_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAndId,
-                                base::Unretained(this),
-                                &result,
-                                &id);
-    drop_callback = base::Bind(
+    read_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAttachments,
+                               base::Unretained(this),
+                               &result,
+                               &attachments);
+    write_callback = base::Bind(
         &FakeAttachmentStoreTest::CopyResult, base::Unretained(this), &result);
+    drop_callback = write_callback;
+
+    some_data1 = new base::RefCountedString;
+    some_data1->data() = kTestData1;
+
+    some_data2 = new base::RefCountedString;
+    some_data2->data() = kTestData2;
   }
 
   virtual void ClearAndPumpLoop() {
     Clear();
-    message_loop_.RunUntilIdle();
+    message_loop.RunUntilIdle();
   }
 
-  AttachmentStore::Result result;
-  AttachmentId id;
-  scoped_ptr<Attachment> attachment;
-
-  AttachmentStore::ReadCallback read_callback;
-  AttachmentStore::WriteCallback write_callback;
-  AttachmentStore::DropCallback drop_callback;
-
  private:
   void Clear() {
     result = AttachmentStore::UNSPECIFIED_ERROR;
-    attachment.reset();
+    attachments.reset();
   }
 
-  void CopyResult(AttachmentStore::Result* destination,
-                  const AttachmentStore::Result& source) {
-    *destination = source;
+  void CopyResult(AttachmentStore::Result* destination_result,
+                  const AttachmentStore::Result& source_result) {
+    *destination_result = source_result;
   }
 
-  void CopyResultAndId(AttachmentStore::Result* destination_result,
-                       AttachmentId* destination_id,
-                       const AttachmentStore::Result& source_result,
-                       const AttachmentId& source_id) {
+  void CopyResultAttachments(AttachmentStore::Result* destination_result,
+                             scoped_ptr<AttachmentMap>* destination_attachments,
+                             const AttachmentStore::Result& source_result,
+                             scoped_ptr<AttachmentMap> source_attachments) {
     CopyResult(destination_result, source_result);
-    *destination_id = source_id;
+    *destination_attachments = source_attachments.Pass();
   }
+};
 
-  void CopyAttachmentAndResult(AttachmentStore::Result* destination_result,
-                               scoped_ptr<Attachment>* destination_attachment,
-                               const AttachmentStore::Result& source_result,
-                               scoped_ptr<Attachment> source_attachment) {
-    CopyResult(destination_result, source_result);
-    *destination_attachment = source_attachment.Pass();
-  }
+// Verify that we do not overwrite existing attachments and that we do not treat
+// it as an error.
+TEST_F(FakeAttachmentStoreTest, Write_NoOverwriteNoError) {
+  // Create two attachments with the same id but different data.
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 =
+      Attachment::CreateWithId(attachment1.GetId(), some_data2);
+
+  // Write the first one.
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  store.Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
 
-  base::MessageLoop message_loop_;
-};
+  // Write the second one.
+  some_attachments.clear();
+  some_attachments.push_back(attachment2);
+  store.Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
 
-TEST_F(FakeAttachmentStoreTest, WriteReadRoundTrip) {
-  FakeAttachmentStore store(base::MessageLoopProxy::current());
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kTestData;
+  // Read it back and see that it was not overwritten.
+  AttachmentIdList some_attachment_ids;
+  some_attachment_ids.push_back(attachment1.GetId());
+  store.Read(some_attachment_ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  EXPECT_EQ(attachments->size(), 1U);
+  AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
+  EXPECT_TRUE(a1 != attachments->end());
+  EXPECT_TRUE(attachment1.GetData()->Equals(a1->second.GetData()));
+}
 
-  store.Write(some_data, write_callback);
+// Verify that we can write some attachments and read them back.
+TEST_F(FakeAttachmentStoreTest, Write_RoundTrip) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  some_attachments.push_back(attachment2);
+
+  store.Write(some_attachments, write_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  AttachmentId id_written(id);
 
-  store.Read(id_written, read_callback);
+  AttachmentIdList some_attachment_ids;
+  some_attachment_ids.push_back(attachment1.GetId());
+  some_attachment_ids.push_back(attachment2.GetId());
+  store.Read(some_attachment_ids, read_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(id_written, attachment->GetId());
-  EXPECT_EQ(some_data, attachment->GetData());
+  EXPECT_EQ(attachments->size(), 2U);
+
+  AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
+  EXPECT_TRUE(a1 != attachments->end());
+  EXPECT_TRUE(attachment1.GetData()->Equals(a1->second.GetData()));
+
+  AttachmentMap::const_iterator a2 = attachments->find(attachment2.GetId());
+  EXPECT_TRUE(a2 != attachments->end());
+  EXPECT_TRUE(attachment2.GetData()->Equals(a2->second.GetData()));
 }
 
-TEST_F(FakeAttachmentStoreTest, Read_NotFound) {
-  FakeAttachmentStore store(base::MessageLoopProxy::current());
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  Attachment some_attachment = Attachment::Create(some_data);
-  AttachmentId some_id = some_attachment.GetId();
-  store.Read(some_id, read_callback);
+// Try to read two attachments when only one exists.
+TEST_F(FakeAttachmentStoreTest, Read_OneNotFound) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+
+  AttachmentList some_attachments;
+  // Write attachment1 only.
+  some_attachments.push_back(attachment1);
+  store.Write(some_attachments, write_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // Try to read both attachment1 and attachment2.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  ids.push_back(attachment2.GetId());
+  store.Read(ids, read_callback);
   ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
-  EXPECT_EQ(NULL, attachment.get());
+
+  // See that only attachment1 was read.
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 1U);
 }
 
-TEST_F(FakeAttachmentStoreTest, Drop) {
-  FakeAttachmentStore store(base::MessageLoopProxy::current());
-  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
-  some_data->data() = kTestData;
-  store.Write(some_data, write_callback);
+// Try to drop two attachments when only one exists. Verify that no error occurs
+// and that the existing attachment was dropped.
+TEST_F(FakeAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
+  // First, create two attachments.
+  Attachment attachment1 = Attachment::Create(some_data1);
+  Attachment attachment2 = Attachment::Create(some_data2);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  some_attachments.push_back(attachment2);
+  store.Write(some_attachments, write_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  AttachmentId id_written(id);
 
-  // First drop.
-  store.Drop(id_written, drop_callback);
+  // Drop attachment1 only.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  store.Drop(ids, drop_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
 
-  store.Read(id_written, read_callback);
+  // See that attachment1 is gone.
+  store.Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+
+  // Drop both attachment1 and attachment2.
+  ids.clear();
+  ids.push_back(attachment1.GetId());
+  ids.push_back(attachment2.GetId());
+  store.Drop(ids, drop_callback);
   ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // See that attachment2 is now gone.
+  ids.clear();
+  ids.push_back(attachment2.GetId());
+  store.Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+}
 
-  // Second drop.
-  store.Drop(id_written, drop_callback);
+// Verify that attempting to drop an attachment that does not exist is not an
+// error.
+TEST_F(FakeAttachmentStoreTest, Drop_DoesNotExist) {
+  Attachment attachment1 = Attachment::Create(some_data1);
+  AttachmentList some_attachments;
+  some_attachments.push_back(attachment1);
+  store.Write(some_attachments, write_callback);
   ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
 
-  store.Read(id_written, read_callback);
+  // Drop the attachment.
+  AttachmentIdList ids;
+  ids.push_back(attachment1.GetId());
+  store.Drop(ids, drop_callback);
   ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
+
+  // See that it's gone.
+  store.Read(ids, read_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
+  EXPECT_EQ(attachments->size(), 0U);
+
+  // Drop again, see that no error occurs.
+  ids.clear();
+  ids.push_back(attachment1.GetId());
+  store.Drop(ids, drop_callback);
+  ClearAndPumpLoop();
+  EXPECT_EQ(result, AttachmentStore::SUCCESS);
 }
 
 }  // namespace syncer