Instantiate AttachmentDownloader and use it in AttachmentServiceImpl

sync/api/attachments/attachment_service_impl.cc

Index: sync/api/attachments/attachment_service_impl.cc
diff --git a/sync/api/attachments/attachment_service_impl.cc b/sync/api/attachments/attachment_service_impl.cc
index af2dea7ccc60fa3d7955d21281e456760da7f11d..0570fd407f58588dce43197df5d92979fd0f55de 100644
--- a/sync/api/attachments/attachment_service_impl.cc
+++ b/sync/api/attachments/attachment_service_impl.cc
@@ -7,17 +7,104 @@
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
 #include "sync/api/attachments/attachment.h"
+#include "sync/internal_api/public/attachments/fake_attachment_downloader.h"
 #include "sync/internal_api/public/attachments/fake_attachment_store.h"
 #include "sync/internal_api/public/attachments/fake_attachment_uploader.h"
 
 namespace syncer {
 
+// GetOrDownloadAttachments starts multiple parallel DownloadAttachment calls.
+// GetOrDownloadState tracks completion of these calls and posts callback for
+// consumer once all attachments are either retrieved or reported unavailable.
+class AttachmentServiceImpl::GetOrDownloadState
+    : public base::RefCounted<GetOrDownloadState>,
+      public base::NonThreadSafe {
+ public:
+  GetOrDownloadState(const AttachmentIdList& attachment_ids,

[maniscalco, 2014/05/29 22:35:54]

Can you document the parameters?

[pavely, 2014/05/30 00:50:07]

Done.

+                     const GetOrDownloadCallback& callback);
+  virtual ~GetOrDownloadState();

[maniscalco, 2014/05/29 22:35:54]

Probably should make the dtor private.  Hmm, I thought we'd have received a
compiler warning/error about this... maybe that's only with
RefCountedThreadSafe?

[pavely, 2014/05/30 00:50:07]

Done.

+
+  // Attachment was just retrieved. Add it to retrieved attachments.
+  void AddAttachment(Attachment attachment);

[maniscalco, 2014/05/29 22:35:54]

attachment can be made const ref: const Attachment&.

[pavely, 2014/05/30 00:50:07]

Done.

+
+  // Both reading from local store and downloading attachment failed.
+  // Add it to unavailable set.
+  void AddUnavailableAttachmentId(const AttachmentId& attachment_id);
+
+ private:
+  // Request regarding attachment id completed. There is going to be no more
+  // calls about this attachment. If all attachment requests completed then post
+  // callback to consumer with results.
+  void AttachmentRequestCompleted(const AttachmentId& attachment_id);
+
+  GetOrDownloadCallback callback_;
+
+  // Requests for these attachments are still in porgress.

[maniscalco, 2014/05/29 22:35:54]

s/porgress/progress/

[pavely, 2014/05/30 00:50:07]

Done.

+  AttachmentIdSet incomplete_attachments_;

[maniscalco, 2014/05/29 22:35:54]

Consider renaming to in_progress_attachments_ for clarity.  I don't want anyone
to think "incomplete attachment" means "part of an attachment".

[pavely, 2014/05/30 00:50:07]

in_progress_attachments_ is better.
I first renamed variables and then wrote comments. If I did it the other way I
would have come up with the same name.

Done.
On 2014/05/29 22:35:54, maniscalco wrote:

+
+  AttachmentIdSet unavailable_attachments_;
+  scoped_ptr<AttachmentMap> retrieved_attachments_;
+
+  DISALLOW_COPY_AND_ASSIGN(GetOrDownloadState);
+};
+
+AttachmentServiceImpl::GetOrDownloadState::GetOrDownloadState(
+    const AttachmentIdList& attachment_ids,
+    const GetOrDownloadCallback& callback)
+    : callback_(callback), retrieved_attachments_(new AttachmentMap()) {
+  std::copy(
+      attachment_ids.begin(),
+      attachment_ids.end(),
+      std::inserter(incomplete_attachments_, incomplete_attachments_.end()));

[maniscalco, 2014/05/29 22:35:54]

Should this be std::back_inserter instead?  I can't remember if std::inserter
will end up inserting if AttachmentIdList is a vector.  Either way, using
back_inserter would make it clear what's going on.

[pavely, 2014/05/30 00:50:07]

back_inserter needs push_back(). inserter needs insert() with hint.

inserter will work on both set and vector while back_inserter only works on
vector.
On 2014/05/29 22:35:54, maniscalco wrote:

[maniscalco, 2014/05/30 16:08:14]

SGTM, std::inserter seems like the way to go.

On 2014/05/30 00:50:07, pavely wrote:

+}
+
+AttachmentServiceImpl::GetOrDownloadState::~GetOrDownloadState() {
+  DCHECK(CalledOnValidThread());
+}
+
+void AttachmentServiceImpl::GetOrDownloadState::AddAttachment(
+    Attachment attachment) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(retrieved_attachments_->find(attachment.GetId()) ==

[maniscalco, 2014/05/29 22:35:54]

FYI, there is a DCHECK_EQ macro.  It may or may not work here.

[pavely, 2014/05/30 00:50:07]

The reason for DCHECK_EQ macro is that it will also print values of left and
right side. It helps when you not only know what assumption failed, but also can
learn actual values from log line.
Here both values are meaningless iterators.
On 2014/05/29 22:35:54, maniscalco wrote:

+         retrieved_attachments_->end());
+  retrieved_attachments_->insert(
+      std::make_pair(attachment.GetId(), attachment));
+  AttachmentRequestCompleted(attachment.GetId());
+}
+
+void AttachmentServiceImpl::GetOrDownloadState::AddUnavailableAttachmentId(
+    const AttachmentId& attachment_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(unavailable_attachments_.find(attachment_id) ==
+         unavailable_attachments_.end());
+  unavailable_attachments_.insert(attachment_id);
+  AttachmentRequestCompleted(attachment_id);
+}
+
+void AttachmentServiceImpl::GetOrDownloadState::AttachmentRequestCompleted(
+    const AttachmentId& attachment_id) {
+  DCHECK(incomplete_attachments_.find(attachment_id) !=
+         incomplete_attachments_.end());
+  incomplete_attachments_.erase(attachment_id);
+
+  if (incomplete_attachments_.empty()) {
+    // All requests completed. Let's notify consumer.
+    GetOrDownloadResult result =
+        unavailable_attachments_.empty() ? GET_SUCCESS : GET_UNSPECIFIED_ERROR;
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback_, result, base::Passed(&retrieved_attachments_)));
+  }
+}
+
 AttachmentServiceImpl::AttachmentServiceImpl(
     scoped_ptr<AttachmentStore> attachment_store,
     scoped_ptr<AttachmentUploader> attachment_uploader,
+    scoped_ptr<AttachmentDownloader> attachment_downloader,
     Delegate* delegate)
     : attachment_store_(attachment_store.Pass()),
       attachment_uploader_(attachment_uploader.Pass()),
+      attachment_downloader_(attachment_downloader.Pass()),
       delegate_(delegate),
       weak_ptr_factory_(this) {
   DCHECK(CalledOnValidThread());
@@ -35,9 +122,13 @@ scoped_ptr<syncer::AttachmentService> AttachmentServiceImpl::CreateForTest() {
       new syncer::FakeAttachmentStore(base::MessageLoopProxy::current()));
   scoped_ptr<AttachmentUploader> attachment_uploader(
       new FakeAttachmentUploader);
+  scoped_ptr<AttachmentDownloader> attachment_downloader(
+      new FakeAttachmentDownloader());
   scoped_ptr<syncer::AttachmentService> attachment_service(
-      new syncer::AttachmentServiceImpl(
-          attachment_store.Pass(), attachment_uploader.Pass(), NULL));
+      new syncer::AttachmentServiceImpl(attachment_store.Pass(),
+                                        attachment_uploader.Pass(),
+                                        attachment_downloader.Pass(),
+                                        NULL));
   return attachment_service.Pass();
 }
 
@@ -45,10 +136,12 @@ void AttachmentServiceImpl::GetOrDownloadAttachments(
     const AttachmentIdList& attachment_ids,
     const GetOrDownloadCallback& callback) {
   DCHECK(CalledOnValidThread());
+  scoped_refptr<GetOrDownloadState> state(
+      new GetOrDownloadState(attachment_ids, callback));
   attachment_store_->Read(attachment_ids,
                           base::Bind(&AttachmentServiceImpl::ReadDone,
                                      weak_ptr_factory_.GetWeakPtr(),
-                                     callback));
+                                     state));
 }
 
 void AttachmentServiceImpl::DropAttachments(
@@ -96,18 +189,29 @@ void AttachmentServiceImpl::OnSyncDataUpdate(
 }
 
 void AttachmentServiceImpl::ReadDone(
-    const GetOrDownloadCallback& callback,
+    const scoped_refptr<GetOrDownloadState>& state,
     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) {
-    get_result = AttachmentService::GET_SUCCESS;
+  // Add read attachments to result.
+  for (AttachmentMap::const_iterator iter = attachments->begin();
+       iter != attachments->end();
+       ++iter) {
+    state->AddAttachment(iter->second);
+  }
+  // Try to download locally unavailable attachments.
+  for (AttachmentIdList::const_iterator iter =
+           unavailable_attachment_ids->begin();
+       iter != unavailable_attachment_ids->end();
+       ++iter) {
+    attachment_downloader_->DownloadAttachment(
+        *iter,
+        base::Bind(&AttachmentServiceImpl::DownloadDone,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   state,
+                   *iter));
+    ;
   }
-  // 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 AttachmentServiceImpl::DropDone(const DropCallback& callback,
@@ -145,4 +249,16 @@ void AttachmentServiceImpl::UploadDone(
   }
 }
 
+void AttachmentServiceImpl::DownloadDone(
+    const scoped_refptr<GetOrDownloadState>& state,
+    const AttachmentId& attachment_id,
+    const AttachmentDownloader::DownloadResult& result,
+    scoped_ptr<Attachment> attachment) {
+  if (result == AttachmentDownloader::DOWNLOAD_SUCCESS) {
+    state->AddAttachment(*attachment.get());
+  } else {
+    state->AddUnavailableAttachmentId(attachment_id);
+  }
+}
+
 }  // namespace syncer