Add AttachmentDownloader interface, change signature of AttachmentSto…

…re::Read - AttachmentDownloader interface is similar to AttachmentUploader interface. No implementation yet. - AttachmentStore::Read guarantee should be stronger. It should attempt to read all attachments and return list of attachment ids that can't be loaded locally. Those need to be downloaded from server. R=maniscalco@chromium.org BUG=376073 Review URL: https://codereview.chromium.org/293143002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@272585 0039d316-1c4b-4281-b951-d872f2087c98
SaschaDeWaal · May 23, 2014 · 1ee3768 · 1ee3768
1 parent c3dd338
commit 1ee3768
Show file tree

Hide file tree

Showing 9 changed files with 104 additions and 20 deletions.
diff --git a/sync/api/attachments/attachment_downloader.cc b/sync/api/attachments/attachment_downloader.cc
@@ -0,0 +1,12 @@
+// Copyright 2014 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.
+
+#include "sync/api/attachments/attachment_downloader.h"
+
+namespace syncer {
+
+AttachmentDownloader::~AttachmentDownloader() {
+}
+
+}  // namespace syncer
diff --git a/sync/api/attachments/attachment_downloader.h b/sync/api/attachments/attachment_downloader.h
@@ -0,0 +1,41 @@
+// Copyright 2014 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_ATTACHMENTS_ATTACHMENT_DOWNLOADER_H_
+#define SYNC_API_ATTACHMENTS_ATTACHMENT_DOWNLOADER_H_
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "sync/api/attachments/attachment.h"
+#include "sync/base/sync_export.h"
+
+namespace syncer {
+
+// AttachmentDownloader is responsible for downloading attachments from server.
+class SYNC_EXPORT AttachmentDownloader {
+ public:
+  // The result of a DownloadAttachment operation.
+  enum DownloadResult {
+    DOWNLOAD_SUCCESS,            // No error, attachment was downloaded
+                                 // successfully.
+    DOWNLOAD_UNSPECIFIED_ERROR,  // An unspecified error occurred.
+  };
+
+  typedef base::Callback<void(const DownloadResult&, scoped_ptr<Attachment>)>
+      DownloadCallback;
+
+  virtual ~AttachmentDownloader();
+
+  // Download attachment referred by |attachment_id| and invoke |callback| when
+  // done.
+  //
+  // |callback| will receive a DownloadResult code and an Attachment object. If
+  // DownloadResult is not DOWNLOAD_SUCCESS then attachment pointer is NULL.
+  virtual void DownloadAttachment(const AttachmentId& attachment_id,
+                                  const DownloadCallback& callback) = 0;
+};
+
+}  // namespace syncer
+
+#endif  // SYNC_API_ATTACHMENTS_ATTACHMENT_DOWNLOADER_H_
diff --git a/sync/api/attachments/attachment_service_impl.cc b/sync/api/attachments/attachment_service_impl.cc
@@ -95,9 +95,11 @@ void AttachmentServiceImpl::OnSyncDataUpdate(
   // attachments from local storage (bug 356351).
 }
 
-void AttachmentServiceImpl::ReadDone(const GetOrDownloadCallback& callback,
-                                     const AttachmentStore::Result& result,
-                                     scoped_ptr<AttachmentMap> attachments) {
+void AttachmentServiceImpl::ReadDone(
+    const GetOrDownloadCallback& callback,
+    const AttachmentStore::Result& result,
+    scoped_ptr<AttachmentMap> attachments,
+    scoped_ptr<AttachmentIdList> unavailable_attachment_ids) {
   AttachmentService::GetOrDownloadResult get_result =
       AttachmentService::GET_UNSPECIFIED_ERROR;
   if (result == AttachmentStore::SUCCESS) {

diff --git a/sync/api/attachments/attachment_service_impl.h b/sync/api/attachments/attachment_service_impl.h
@@ -45,7 +45,8 @@ class SYNC_EXPORT AttachmentServiceImpl : public AttachmentService,
  private:
   void ReadDone(const GetOrDownloadCallback& callback,
                 const AttachmentStore::Result& result,
-                scoped_ptr<AttachmentMap> attachments);
+                scoped_ptr<AttachmentMap> attachments,
+                scoped_ptr<AttachmentIdList> unavailable_attachment_ids);
   void DropDone(const DropCallback& callback,
                 const AttachmentStore::Result& result);
   void WriteDone(const StoreCallback& callback,

diff --git a/sync/api/attachments/attachment_store.h b/sync/api/attachments/attachment_store.h
@@ -38,17 +38,21 @@ class SYNC_EXPORT AttachmentStore {
     UNSPECIFIED_ERROR,  // An unspecified error occurred for one or more items.
   };
 
-  typedef base::Callback<void(const Result&, scoped_ptr<AttachmentMap>)>
-      ReadCallback;
+  typedef base::Callback<void(const Result&,
+                              scoped_ptr<AttachmentMap>,
+                              scoped_ptr<AttachmentIdList>)> ReadCallback;
   typedef base::Callback<void(const Result&)> WriteCallback;
   typedef base::Callback<void(const Result&)> DropCallback;
 
   // Asynchronously reads the attachments identified by |ids|.
   //
-  // |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.
+  // |callback| will be invoked when finished. AttachmentStore will attempt to
+  // read all attachments specified in ids. If any of the attachments do not
+  // exist or could not be read, |callback|'s Result will be UNSPECIFIED_ERROR.
+  // Callback's AttachmentMap will contain all attachments that were
+  // successfully read, AttachmentIdList will contain attachment ids of
+  // attachments that are unavailable in attachment store, these need to be
+  // downloaded from server.
   //
   // Reads on individual attachments are treated atomically; |callback| will not
   // read only part of an attachment.

diff --git a/sync/api/attachments/attachment_uploader.h b/sync/api/attachments/attachment_uploader.h
@@ -8,7 +8,6 @@
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/base/sync_export.h"
 
@@ -36,7 +35,7 @@ class SYNC_EXPORT AttachmentUploader {
   // or otherwise).
   //
   // |callback| will receive an UploadResult code and an updated AttachmentId
-  // |containing the server address of the newly uploaded attachment.
+  // containing the server address of the newly uploaded attachment.
   virtual void UploadAttachment(const Attachment& attachment,
                                 const UploadCallback& callback) = 0;
 };

diff --git a/sync/internal_api/attachments/fake_attachment_store.cc b/sync/internal_api/attachments/fake_attachment_store.cc
@@ -47,6 +47,7 @@ void FakeAttachmentStore::Backend::Read(const AttachmentIdList& ids,
   AttachmentIdList::const_iterator id_iter = ids.begin();
   AttachmentIdList::const_iterator id_end = ids.end();
   scoped_ptr<AttachmentMap> result_map(new AttachmentMap);
+  scoped_ptr<AttachmentIdList> unavailable_attachments(new AttachmentIdList);
   for (; id_iter != id_end; ++id_iter) {
     const AttachmentId& id = *id_iter;
     syncer::AttachmentMap::iterator attachment_iter =
@@ -55,12 +56,18 @@ void FakeAttachmentStore::Backend::Read(const AttachmentIdList& ids,
       const Attachment& attachment = attachment_iter->second;
       result_map->insert(std::make_pair(id, attachment));
     } else {
-      result_code = UNSPECIFIED_ERROR;
-      break;
+      unavailable_attachments->push_back(id);
     }
   }
+  if (!unavailable_attachments->empty()) {
+    result_code = UNSPECIFIED_ERROR;
+  }
   frontend_task_runner_->PostTask(
-      FROM_HERE, base::Bind(callback, result_code, base::Passed(&result_map)));
+      FROM_HERE,
+      base::Bind(callback,
+                 result_code,
+                 base::Passed(&result_map),
+                 base::Passed(&unavailable_attachments)));
 }
 
 void FakeAttachmentStore::Backend::Write(const AttachmentList& attachments,

diff --git a/sync/internal_api/attachments/fake_attachment_store_unittest.cc b/sync/internal_api/attachments/fake_attachment_store_unittest.cc
@@ -23,6 +23,7 @@ class FakeAttachmentStoreTest : public testing::Test {
   FakeAttachmentStore store;
   AttachmentStore::Result result;
   scoped_ptr<AttachmentMap> attachments;
+  scoped_ptr<AttachmentIdList> failed_attachment_ids;
 
   AttachmentStore::ReadCallback read_callback;
   AttachmentStore::WriteCallback write_callback;
@@ -38,7 +39,8 @@ class FakeAttachmentStoreTest : public testing::Test {
     read_callback = base::Bind(&FakeAttachmentStoreTest::CopyResultAttachments,
                                base::Unretained(this),
                                &result,
-                               &attachments);
+                               &attachments,
+                               &failed_attachment_ids);
     write_callback = base::Bind(
         &FakeAttachmentStoreTest::CopyResult, base::Unretained(this), &result);
     drop_callback = write_callback;
@@ -59,19 +61,24 @@ class FakeAttachmentStoreTest : public testing::Test {
   void Clear() {
     result = AttachmentStore::UNSPECIFIED_ERROR;
     attachments.reset();
+    failed_attachment_ids.reset();
   }
 
   void CopyResult(AttachmentStore::Result* destination_result,
                   const AttachmentStore::Result& source_result) {
     *destination_result = source_result;
   }
 
-  void CopyResultAttachments(AttachmentStore::Result* destination_result,
-                             scoped_ptr<AttachmentMap>* destination_attachments,
-                             const AttachmentStore::Result& source_result,
-                             scoped_ptr<AttachmentMap> source_attachments) {
+  void CopyResultAttachments(
+      AttachmentStore::Result* destination_result,
+      scoped_ptr<AttachmentMap>* destination_attachments,
+      scoped_ptr<AttachmentIdList>* destination_failed_attachment_ids,
+      const AttachmentStore::Result& source_result,
+      scoped_ptr<AttachmentMap> source_attachments,
+      scoped_ptr<AttachmentIdList> source_failed_attachment_ids) {
     CopyResult(destination_result, source_result);
     *destination_attachments = source_attachments.Pass();
+    *destination_failed_attachment_ids = source_failed_attachment_ids.Pass();
   }
 };
 
@@ -104,6 +111,7 @@ TEST_F(FakeAttachmentStoreTest, Write_NoOverwriteNoError) {
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
   EXPECT_EQ(attachments->size(), 1U);
+  EXPECT_EQ(failed_attachment_ids->size(), 0U);
   AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
   EXPECT_TRUE(a1 != attachments->end());
   EXPECT_TRUE(attachment1.GetData()->Equals(a1->second.GetData()));
@@ -128,6 +136,7 @@ TEST_F(FakeAttachmentStoreTest, Write_RoundTrip) {
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::SUCCESS);
   EXPECT_EQ(attachments->size(), 2U);
+  EXPECT_EQ(failed_attachment_ids->size(), 0U);
 
   AttachmentMap::const_iterator a1 = attachments->find(attachment1.GetId());
   EXPECT_TRUE(a1 != attachments->end());
@@ -160,6 +169,7 @@ TEST_F(FakeAttachmentStoreTest, Read_OneNotFound) {
   // See that only attachment1 was read.
   EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
   EXPECT_EQ(attachments->size(), 1U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
 }
 
 // Try to drop two attachments when only one exists. Verify that no error occurs
@@ -187,6 +197,8 @@ TEST_F(FakeAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
   EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment1.GetId());
 
   // Drop both attachment1 and attachment2.
   ids.clear();
@@ -203,6 +215,8 @@ TEST_F(FakeAttachmentStoreTest, Drop_DropTwoButOnlyOneExists) {
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
   EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment2.GetId());
 }
 
 // Verify that attempting to drop an attachment that does not exist is not an
@@ -227,6 +241,8 @@ TEST_F(FakeAttachmentStoreTest, Drop_DoesNotExist) {
   ClearAndPumpLoop();
   EXPECT_EQ(result, AttachmentStore::UNSPECIFIED_ERROR);
   EXPECT_EQ(attachments->size(), 0U);
+  EXPECT_EQ(failed_attachment_ids->size(), 1U);
+  EXPECT_EQ((*failed_attachment_ids)[0], attachment1.GetId());
 
   // Drop again, see that no error occurs.
   ids.clear();

diff --git a/sync/sync_api.gypi b/sync/sync_api.gypi
@@ -15,6 +15,8 @@
   'sources': [
     'api/attachments/attachment.cc',
     'api/attachments/attachment.h',
+    'api/attachments/attachment_downloader.cc',
+    'api/attachments/attachment_downloader.h',
     'api/attachments/attachment_id.cc',
     'api/attachments/attachment_id.h',
     'api/attachments/attachment_service.cc',