Instantiate AttachmentDownloader and use it in AttachmentServiceImpl

sync/api/attachments/attachment_service_impl.cc

 // 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_service_impl.h"
 
 #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());
   DCHECK(attachment_store_);
   DCHECK(attachment_uploader_);
 }
 
 AttachmentServiceImpl::~AttachmentServiceImpl() {
   DCHECK(CalledOnValidThread());
 }
 
 // Static.
 scoped_ptr<syncer::AttachmentService> AttachmentServiceImpl::CreateForTest() {
   scoped_ptr<syncer::AttachmentStore> attachment_store(
       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();
 }
 
 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(
     const AttachmentIdList& attachment_ids,
     const DropCallback& callback) {
   DCHECK(CalledOnValidThread());
   attachment_store_->Drop(attachment_ids,
                           base::Bind(&AttachmentServiceImpl::DropDone,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      callback));
@@ skipping 27 matching lines @@
     const AttachmentIdList& old_attachment_ids,
     const SyncData& updated_sync_data) {
   DCHECK(CalledOnValidThread());
   // TODO(maniscalco): At this point we need to ensure we write all new
   // attachments referenced by updated_sync_data to local storage and schedule
   // them up upload to the server. We also need to remove any no unreferenced
   // attachments from local storage (bug 356351).
 }
 
 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);
   }
-  // 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)));
+  // 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));
+    ;
+  }
 }
 
 void AttachmentServiceImpl::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).
@@ skipping 17 matching lines @@
     const AttachmentUploader::UploadResult& result,
     const AttachmentId& attachment_id) {
   // TODO(pavely): crbug/372622: Deal with UploadAttachment failures.
   if (result != AttachmentUploader::UPLOAD_SUCCESS)
     return;
   if (delegate_) {
     delegate_->OnAttachmentUploaded(attachment_id);
   }
 }
 
+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