DownloadManagerDelegate::ShouldCompleteDownload(callback)

chrome/browser/chromeos/gdata/gdata_download_observer.cc

Index: chrome/browser/chromeos/gdata/gdata_download_observer.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_download_observer.cc b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
index 3cb89fb37a46dfe357cda973f292628911e1781d..bb085c553df1be909d012e16d210d1257b5f8b4c 100644
--- a/chrome/browser/chromeos/gdata/gdata_download_observer.cc
+++ b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
@@ -34,7 +34,20 @@ class UploadingExternalData : public DownloadItem::ExternalData {
   }
   virtual ~UploadingExternalData() {}
 
-  void MarkAsComplete() { is_complete_ = true; }
+  // |cb| will be called when this Upload is marked as complete.
+  void set_complete_callback(const base::Closure& cb) {
+    // Be very careful to avoid calling cb twice, or more than one cb!
+    if (!is_complete_)
+      complete_cb_ = cb;
+  }
+
+  void CompleteDownload() {
+    is_complete_ = true;
+    if (!complete_cb_.is_null()) {
+      complete_cb_.Run();
+      complete_cb_.Reset();
+    }
+  }
 
   int upload_id() const { return upload_id_; }
   bool is_complete() const { return is_complete_; }
@@ -42,6 +55,7 @@ class UploadingExternalData : public DownloadItem::ExternalData {
 
  private:
   GDataUploader* uploader_;
+  base::Closure complete_cb_;
   int upload_id_;
   bool is_complete_;
 };
@@ -126,14 +140,22 @@ bool GDataDownloadObserver::IsGDataDownload(DownloadItem* download) {
 }
 
 // static
-bool GDataDownloadObserver::IsReadyToComplete(DownloadItem* download) {
+bool GDataDownloadObserver::IsReadyToComplete(
+    DownloadItem* download,
+    const base::Closure& maybe_complete_download) {

[Randy Smith (Not in Mondays), 2012/05/01 18:21:49]

Same naming issue.  The phrase "maybe complete download" I think only makes
sense if you're looking at it from the perspective of the download execution
flow, not from outside.

[benjhayden, 2012/05/01 19:00:30]

Done.

   // |download| is ready for completion (as far as GData is concerned) if:
   // 1. It's not a GData download.
   //  - or -
   // 2. The upload has completed.
+  if (!IsGDataDownload(download))
+    return true;
   UploadingExternalData* upload_data = GetUploadingExternalData(download);
-  return !IsGDataDownload(download) ||
-      (upload_data && upload_data->is_complete());
+  DCHECK(upload_data);
+  if (upload_data->is_complete())
+    return true;
+  if (!maybe_complete_download.is_null())

[Randy Smith (Not in Mondays), 2012/05/01 18:21:49]

Don't you want this to be unilateral?  Doing it this way means that the last one
passed won't win, if it's null.

[benjhayden, 2012/05/01 19:00:30]

I did this intentionally in case another caller calls
CDMD::ShouldCompleteDownload() for the same download. If the new caller needs to
pass a different complete_cb, then we'll need to extend the mechanism to N
callbacks, but if the new caller doesn't need to pass a different complete_cb,
then it can pass the null callback and we won't need to change anything.

[Randy Smith (Not in Mondays), 2012/05/02 18:14:07]

This feels like a hacky interface design--we're not designing for the double
callback case, we're designing for half of the double callback case.  I'd prefer
for us to have the simplest interface that meets our current requirements, and
wouldn't mind designing for the future if that's relatively simple and clean. 
But this seems like we're adding complexity to no good purpose.  If you don't
have strong feelings, I'd be more inclined to just allow null callbacks and have
them overwrite the previous callbacks.  

[benjhayden, 2012/05/03 19:21:14]

Done.

+    upload_data->set_complete_callback(maybe_complete_download);
+  return false;
 }
 
 // static
@@ -335,11 +357,10 @@ void GDataDownloadObserver::OnUploadComplete(int32 download_id,
     return;
   }
   DVLOG(1) << "Completing upload for download ID " << download_id;
-  DownloadItem* download = iter->second;
-  UploadingExternalData* upload_data = GetUploadingExternalData(download);
+  DownloadItem* download_item = iter->second;
+  UploadingExternalData* upload_data = GetUploadingExternalData(download_item);
   DCHECK(upload_data);
-  upload_data->MarkAsComplete();
-  download->MaybeCompleteDownload();
+  upload_data->CompleteDownload();
 }
 
 }  // namespace gdata