Changes to sync/api/attachment in prep for integrating with sync API.

Make Attachment CopyAssignable so it can be used in STL containers.

Add some typedefs lists and maps of Attachments and AttachmentIds.

Make AttachmentStore::Write responsible for assigning/obtaining an
AttachmentId. This eliminates that case where a caller of
AttachmentStore can attempt to overwrite an existing attachment.

BUG=348624

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

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

7 files changed, 102 insertions, 123 deletions

diff --git a/sync/api/attachments/attachment.cc b/sync/api/attachments/attachment.cc
index 2591f51..896ff1f 100644
--- a/sync/api/attachments/attachment.cc
+++ b/sync/api/attachments/attachment.cc
@@ -5,7 +5,6 @@
 #include "sync/api/attachments/attachment.h"
 
 #include "base/logging.h"
-#include "base/memory/ref_counted_memory.h"
 #include "base/rand_util.h"
 
 namespace syncer {
@@ -14,34 +13,35 @@ Attachment::~Attachment() {}
 
 // Static.
 scoped_ptr<Attachment> Attachment::Create(
-    const scoped_refptr<base::RefCountedMemory>& bytes) {
-  return CreateWithId(CreateId(), bytes);
+    const scoped_refptr<base::RefCountedMemory>& data) {
+  return CreateWithId(CreateId(), data);
 }
 
 // Static.
 scoped_ptr<Attachment> Attachment::CreateWithId(
     const sync_pb::AttachmentId& id,
-    const scoped_refptr<base::RefCountedMemory>& bytes) {
-  return scoped_ptr<Attachment>(new Attachment(id, bytes)).Pass();
+    const scoped_refptr<base::RefCountedMemory>& data) {
+  return scoped_ptr<Attachment>(new Attachment(id, data)).Pass();
 }
 
 const sync_pb::AttachmentId& Attachment::GetId() const { return id_; }
 
-const scoped_refptr<base::RefCountedMemory>& Attachment::GetBytes() const {
-  return bytes_;
+const scoped_refptr<base::RefCountedMemory>& Attachment::GetData() const {
+  return data_;
 }
 
 Attachment::Attachment(const sync_pb::AttachmentId& id,
-                       const scoped_refptr<base::RefCountedMemory>& bytes)
-    : id_(id), bytes_(bytes) {
+                       const scoped_refptr<base::RefCountedMemory>& data)
+    : id_(id), data_(data) {
   DCHECK(!id.unique_id().empty());
-  DCHECK(bytes);
+  DCHECK(data);
 }
 
 // Static.
 sync_pb::AttachmentId Attachment::CreateId() {
   sync_pb::AttachmentId result;
   // Only requirement here is that this id must be globally unique.
+  // TODO(maniscalco): Consider making this base64 encoded.
   result.set_unique_id(base::RandBytesAsString(16));
   return result;
 }
diff --git a/sync/api/attachments/attachment.h b/sync/api/attachments/attachment.h
index 106fbc5..1f1dbd2 100644
--- a/sync/api/attachments/attachment.h
+++ b/sync/api/attachments/attachment.h
@@ -8,66 +8,64 @@
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_ptr.h"
 #include "sync/base/sync_export.h"
 #include "sync/protocol/sync.pb.h"
 
-namespace base {
-class RefCountedMemory;
-}  // namespace base
-
 namespace syncer {
 
-// An immutable blob of in-memory data attached to a sync item.
+// A blob of in-memory data attached to a sync item.
+//
+// While Attachment objects themselves aren't immutable (they are assignable)
+// the data they wrap is immutable.
 //
-// It is cheap to copy Attachments.
+// It is cheap to copy Attachments. Feel free to store and return by value.
 class SYNC_EXPORT Attachment {
  public:
   ~Attachment();
 
-  // Creates an attachment with a unique id and the supplied bytes.
+  // Default copy and assignment are welcome.
+
+  // Creates an attachment with a unique id and the supplied data.
   //
   // Used when creating a brand new attachment.
   static scoped_ptr<Attachment> Create(
-      const scoped_refptr<base::RefCountedMemory>& bytes);
+      const scoped_refptr<base::RefCountedMemory>& data);
 
-  // Creates an attachment with the supplied id and bytes.
+  // Creates an attachment with the supplied id and data.
   //
   // Used when you want to recreate a specific attachment. E.g. creating a local
   // copy of an attachment that already exists on the sync server.
   static scoped_ptr<Attachment> CreateWithId(
       const sync_pb::AttachmentId& id,
-      const scoped_refptr<base::RefCountedMemory>& bytes);
+      const scoped_refptr<base::RefCountedMemory>& data);
 
   // Returns this attachment's id.
   const sync_pb::AttachmentId& GetId() const;
 
-  // Returns this attachment's bytes.
-  const scoped_refptr<base::RefCountedMemory>& GetBytes() const;
+  // Returns this attachment's data.
+  const scoped_refptr<base::RefCountedMemory>& GetData() const;
 
  private:
-  const sync_pb::AttachmentId id_;
-  const scoped_refptr<base::RefCountedMemory> bytes_;
+  sync_pb::AttachmentId id_;
+  scoped_refptr<base::RefCountedMemory> data_;
 
   friend class AttachmentTest;
-  friend class FakeAttachmentStoreTest;
-
   FRIEND_TEST_ALL_PREFIXES(AttachmentTest, CreateId_UniqueIdIsUnique);
-  FRIEND_TEST_ALL_PREFIXES(FakeAttachmentStoreTest, WriteReadRoundTrip);
-  FRIEND_TEST_ALL_PREFIXES(FakeAttachmentStoreTest, Write_Overwrite);
-  FRIEND_TEST_ALL_PREFIXES(FakeAttachmentStoreTest, Read_NotFound);
-  FRIEND_TEST_ALL_PREFIXES(FakeAttachmentStoreTest, Drop);
 
   Attachment(const sync_pb::AttachmentId& id,
-             const scoped_refptr<base::RefCountedMemory>& bytes);
+             const scoped_refptr<base::RefCountedMemory>& data);
 
   // Creates a unique attachment id.
   static sync_pb::AttachmentId CreateId();
-
-  // Default copy ctor welcome.
-  DISALLOW_ASSIGN(Attachment);
 };
 
+typedef std::vector<syncer::Attachment> AttachmentList;
+typedef std::string AttachmentIdUniqueId;  // AttachmentId.unique_id()
+typedef std::map<AttachmentIdUniqueId, Attachment> AttachmentMap;
+typedef std::vector<sync_pb::AttachmentId> AttachmentIdList;
+
 }  // namespace syncer
 
 #endif  // SYNC_API_ATTACHMENTS_ATTACHMENT_H_
diff --git a/sync/api/attachments/attachment_store.h b/sync/api/attachments/attachment_store.h
index d4e951e..60a609a 100644
--- a/sync/api/attachments/attachment_store.h
+++ b/sync/api/attachments/attachment_store.h
@@ -39,7 +39,8 @@ class SYNC_EXPORT AttachmentStore {
 
   typedef base::Callback<void(const Result&, scoped_ptr<Attachment>)>
       ReadCallback;
-  typedef base::Callback<void(const Result&)> WriteCallback;
+  typedef base::Callback<void(const Result&, const sync_pb::AttachmentId& id)>
+      WriteCallback;
   typedef base::Callback<void(const Result&)> DropCallback;
 
   // Asynchronously reads the attachment identified by |id|.
@@ -50,12 +51,10 @@ class SYNC_EXPORT AttachmentStore {
   virtual void Read(const sync_pb::AttachmentId& id,
                     const ReadCallback& callback) = 0;
 
-  // Asynchronously writes |bytes| to the store under the given |id|.
+  // Asynchronously writes |bytes| to the store.
   //
-  // If the store already contains an attachment with |id| it will be
-  // overwritten. |callback| will be invoked when finished.
-  virtual void Write(const sync_pb::AttachmentId& id,
-                     const scoped_refptr<base::RefCountedMemory>& bytes,
+  // |callback| will be invoked when finished.
+  virtual void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
                      const WriteCallback& callback) = 0;
 
   // Asynchronously drops the attchment with the given id from this store.
diff --git a/sync/api/attachments/attachment_unittest.cc b/sync/api/attachments/attachment_unittest.cc
index a048c27..213d7f2 100644
--- a/sync/api/attachments/attachment_unittest.cc
+++ b/sync/api/attachments/attachment_unittest.cc
@@ -32,29 +32,29 @@ TEST_F(AttachmentTest, CreateId_UniqueIdIsUnique) {
 
 TEST_F(AttachmentTest, Create_UniqueIdIsUnique) {
   AttachmentId id;
-  scoped_refptr<base::RefCountedString> bytes(new base::RefCountedString);
-  bytes->data() = kAttachmentData;
-  scoped_ptr<Attachment> a1 = Attachment::Create(bytes);
-  scoped_ptr<Attachment> a2 = Attachment::Create(bytes);
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  some_data->data() = kAttachmentData;
+  scoped_ptr<Attachment> a1 = Attachment::Create(some_data);
+  scoped_ptr<Attachment> a2 = Attachment::Create(some_data);
   EXPECT_NE(a1->GetId().unique_id(), a2->GetId().unique_id());
-  EXPECT_EQ(a1->GetBytes(), a2->GetBytes());
+  EXPECT_EQ(a1->GetData(), a2->GetData());
 }
 
-TEST_F(AttachmentTest, Create_WithEmptyBytes) {
+TEST_F(AttachmentTest, Create_WithEmptyData) {
   AttachmentId id;
-  scoped_refptr<base::RefCountedString> emptyBytes(new base::RefCountedString);
-  scoped_ptr<Attachment> a = Attachment::Create(emptyBytes);
-  EXPECT_EQ(emptyBytes, a->GetBytes());
+  scoped_refptr<base::RefCountedString> emptyData(new base::RefCountedString);
+  scoped_ptr<Attachment> a = Attachment::Create(emptyData);
+  EXPECT_EQ(emptyData, a->GetData());
 }
 
 TEST_F(AttachmentTest, CreateWithId_HappyCase) {
   AttachmentId id;
   id.set_unique_id("3290482049832");
-  scoped_refptr<base::RefCountedString> bytes(new base::RefCountedString);
-  bytes->data() = kAttachmentData;
-  scoped_ptr<Attachment> a = Attachment::CreateWithId(id, bytes);
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  some_data->data() = kAttachmentData;
+  scoped_ptr<Attachment> a = Attachment::CreateWithId(id, some_data);
   EXPECT_EQ(id.unique_id(), a->GetId().unique_id());
-  EXPECT_EQ(bytes, a->GetBytes());
+  EXPECT_EQ(some_data, a->GetData());
 }
 
 }  // namespace syncer
diff --git a/sync/api/attachments/fake_attachment_store.cc b/sync/api/attachments/fake_attachment_store.cc
index 6d6b19a..99ee76a 100644
--- a/sync/api/attachments/fake_attachment_store.cc
+++ b/sync/api/attachments/fake_attachment_store.cc
@@ -23,8 +23,7 @@ class FakeAttachmentStore::Backend
       const scoped_refptr<base::SingleThreadTaskRunner>& frontend_task_runner);
 
   void Read(const sync_pb::AttachmentId& id, const ReadCallback& callback);
-  void Write(const sync_pb::AttachmentId& id,
-             const scoped_refptr<base::RefCountedMemory>& bytes,
+  void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
              const WriteCallback& callback);
   void Drop(const sync_pb::AttachmentId& id, const DropCallback& callback);
 
@@ -34,7 +33,7 @@ class FakeAttachmentStore::Backend
   typedef std::map<UniqueId, Attachment*> AttachmentMap;
 
   ~Backend();
-  Result RemoveAttachment(const sync_pb::AttachmentId& id);
+  Result Remove(const sync_pb::AttachmentId& id);
 
   scoped_refptr<base::SingleThreadTaskRunner> frontend_task_runner_;
   AttachmentMap attachments_;
@@ -62,23 +61,24 @@ void FakeAttachmentStore::Backend::Read(const sync_pb::AttachmentId& id,
 }
 
 void FakeAttachmentStore::Backend::Write(
-    const sync_pb::AttachmentId& id,
     const scoped_refptr<base::RefCountedMemory>& bytes,
     const WriteCallback& callback) {
-  scoped_ptr<Attachment> attachment = Attachment::CreateWithId(id, bytes);
-  RemoveAttachment(id);
-  attachments_.insert(
-      AttachmentMap::value_type(id.unique_id(), attachment.release()));
-  frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, SUCCESS));
+  scoped_ptr<Attachment> attachment = Attachment::Create(bytes);
+  DCHECK(attachment.get());
+  sync_pb::AttachmentId attachment_id(attachment->GetId());
+  attachments_.insert(AttachmentMap::value_type(attachment_id.unique_id(),
+                                                attachment.release()));
+  frontend_task_runner_->PostTask(FROM_HERE,
+                                  base::Bind(callback, SUCCESS, attachment_id));
 }
 
 void FakeAttachmentStore::Backend::Drop(const sync_pb::AttachmentId& id,
                                         const DropCallback& callback) {
-  Result result = RemoveAttachment(id);
+  Result result = Remove(id);
   frontend_task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
 }
 
-AttachmentStore::Result FakeAttachmentStore::Backend::RemoveAttachment(
+AttachmentStore::Result FakeAttachmentStore::Backend::Remove(
     const sync_pb::AttachmentId& id) {
   Result result = NOT_FOUND;
   AttachmentMap::iterator iter = attachments_.find(id.unique_id());
@@ -105,13 +105,12 @@ void FakeAttachmentStore::Read(const sync_pb::AttachmentId& id,
 }
 
 void FakeAttachmentStore::Write(
-    const sync_pb::AttachmentId& id,
     const scoped_refptr<base::RefCountedMemory>& bytes,
     const WriteCallback& callback) {
   backend_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(
-          &FakeAttachmentStore::Backend::Write, backend_, id, bytes, callback));
+          &FakeAttachmentStore::Backend::Write, backend_, bytes, callback));
 }
 
 void FakeAttachmentStore::Drop(const sync_pb::AttachmentId& id,
diff --git a/sync/api/attachments/fake_attachment_store.h b/sync/api/attachments/fake_attachment_store.h
index 6f04478..b5d96b3 100644
--- a/sync/api/attachments/fake_attachment_store.h
+++ b/sync/api/attachments/fake_attachment_store.h
@@ -35,19 +35,20 @@ class SYNC_EXPORT FakeAttachmentStore : public AttachmentStore {
   // |backend_task_runner|.
   FakeAttachmentStore(
       const scoped_refptr<base::SequencedTaskRunner>& backend_task_runner);
+
   virtual ~FakeAttachmentStore();
 
   // AttachmentStore implementation.
   virtual void Read(const sync_pb::AttachmentId& id,
                     const ReadCallback& callback) OVERRIDE;
-  virtual void Write(const sync_pb::AttachmentId& id,
-                     const scoped_refptr<base::RefCountedMemory>& bytes,
+  virtual void Write(const scoped_refptr<base::RefCountedMemory>& bytes,
                      const WriteCallback& callback) OVERRIDE;
   virtual void Drop(const sync_pb::AttachmentId& id,
                     const DropCallback& callback) OVERRIDE;
 
  private:
   class Backend;
+
   scoped_refptr<Backend> backend_;
   scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
 
diff --git a/sync/api/attachments/fake_attachment_store_unittest.cc b/sync/api/attachments/fake_attachment_store_unittest.cc
index 5225e50..650452c 100644
--- a/sync/api/attachments/fake_attachment_store_unittest.cc
+++ b/sync/api/attachments/fake_attachment_store_unittest.cc
@@ -16,8 +16,7 @@ using sync_pb::AttachmentId;
 
 namespace syncer {
 
-const char kTestData1[] = "some data";
-const char kTestData2[] = "some other data";
+const char kTestData[] = "some data";
 
 class FakeAttachmentStoreTest : public testing::Test {
  protected:
@@ -28,9 +27,12 @@ class FakeAttachmentStoreTest : public testing::Test {
                    base::Unretained(this),
                    &result,
                    &attachment);
-    write_callback = base::Bind(
+    write_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAndId,
+                                base::Unretained(this),
+                                &result,
+                                &id);
+    drop_callback = base::Bind(
         &FakeAttachmentStoreTest::CopyResult, base::Unretained(this), &result);
-    drop_callback = write_callback;
   }
 
   virtual void ClearAndPumpLoop() {
@@ -39,6 +41,7 @@ class FakeAttachmentStoreTest : public testing::Test {
   }
 
   AttachmentStore::Result result;
+  AttachmentId id;
   scoped_ptr<Attachment> attachment;
 
   AttachmentStore::ReadCallback read_callback;
@@ -48,6 +51,7 @@ class FakeAttachmentStoreTest : public testing::Test {
  private:
   void Clear() {
     result = AttachmentStore::UNSPECIFIED_ERROR;
+    id.Clear();
     attachment.reset();
   }
 
@@ -56,6 +60,14 @@ class FakeAttachmentStoreTest : public testing::Test {
     *destination = source;
   }
 
+  void CopyResultAndId(AttachmentStore::Result* destination_result,
+                       AttachmentId* destination_id,
+                       const AttachmentStore::Result& source_result,
+                       const AttachmentId& source_id) {
+    CopyResult(destination_result, source_result);
+    *destination_id = source_id;
+  }
+
   void CopyAttachmentAndResult(AttachmentStore::Result* destination_result,
                                scoped_ptr<Attachment>* destination_attachment,
                                const AttachmentStore::Result& source_result,
@@ -69,59 +81,28 @@ class FakeAttachmentStoreTest : public testing::Test {
 
 TEST_F(FakeAttachmentStoreTest, WriteReadRoundTrip) {
   FakeAttachmentStore store(base::MessageLoopProxy::current());
-  AttachmentId id = Attachment::CreateId();
-  scoped_refptr<base::RefCountedString> bytes(new base::RefCountedString);
-  bytes->data() = kTestData1;
-
-  store.Write(id, bytes, write_callback);
-  ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::SUCCESS);
-
-  store.Read(id, read_callback);
-  ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(id.unique_id(), attachment->GetId().unique_id());
-  EXPECT_EQ(bytes, attachment->GetBytes());
-
-  store.Read(id, read_callback);
-  ClearAndPumpLoop();
-  EXPECT_EQ(id.unique_id(), attachment->GetId().unique_id());
-  EXPECT_EQ(bytes, attachment->GetBytes());
-}
-
-TEST_F(FakeAttachmentStoreTest, Write_Overwrite) {
-  FakeAttachmentStore store(base::MessageLoopProxy::current());
-  AttachmentId id = Attachment::CreateId();
-  scoped_refptr<base::RefCountedString> bytes(new base::RefCountedString);
-  bytes->data() = kTestData1;
-
-  store.Write(id, bytes, write_callback);
-  ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::SUCCESS);
-
-  store.Read(id, read_callback);
-  ClearAndPumpLoop();
-  EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(id.unique_id(), attachment->GetId().unique_id());
-  EXPECT_EQ(bytes, attachment->GetBytes());
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  some_data->data() = kTestData;
 
-  scoped_refptr<base::RefCountedString> bytes2(new base::RefCountedString);
-  bytes2->data() = kTestData2;
-  store.Write(id, bytes2, write_callback);
+  store.Write(some_data, write_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  EXPECT_TRUE(id.has_unique_id());
+  AttachmentId id_written(id);
 
-  store.Read(id, read_callback);
+  store.Read(id_written, read_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
-  EXPECT_EQ(id.unique_id(), attachment->GetId().unique_id());
-  EXPECT_EQ(bytes2, attachment->GetBytes());
+  EXPECT_EQ(id_written.unique_id(), attachment->GetId().unique_id());
+  EXPECT_EQ(some_data, attachment->GetData());
 }
 
 TEST_F(FakeAttachmentStoreTest, Read_NotFound) {
   FakeAttachmentStore store(base::MessageLoopProxy::current());
-  AttachmentId id = Attachment::CreateId();
-  store.Read(id, read_callback);
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  scoped_ptr<Attachment> some_attachment = Attachment::Create(some_data);
+  AttachmentId some_id = some_attachment->GetId();
+  store.Read(some_id, read_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
   EXPECT_EQ(NULL, attachment.get());
@@ -129,28 +110,29 @@ TEST_F(FakeAttachmentStoreTest, Read_NotFound) {
 
 TEST_F(FakeAttachmentStoreTest, Drop) {
   FakeAttachmentStore store(base::MessageLoopProxy::current());
-  scoped_refptr<base::RefCountedString> bytes(new base::RefCountedString);
-  bytes->data() = kTestData1;
-  AttachmentId id = Attachment::CreateId();
-  store.Write(id, bytes, write_callback);
+  scoped_refptr<base::RefCountedString> some_data(new base::RefCountedString);
+  some_data->data() = kTestData;
+  store.Write(some_data, write_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
+  EXPECT_TRUE(id.has_unique_id());
+  AttachmentId id_written(id);
 
   // First drop.
-  store.Drop(id, drop_callback);
+  store.Drop(id_written, drop_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
 
-  store.Read(id, read_callback);
+  store.Read(id_written, read_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
 
   // Second drop.
-  store.Drop(id, drop_callback);
+  store.Drop(id_written, drop_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
 
-  store.Read(id, read_callback);
+  store.Read(id_written, read_callback);
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::NOT_FOUND);
 }