DownloadManagerDelegate::ShouldCompleteDownload(callback)

chrome/browser/chromeos/gdata/gdata_download_observer.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/gdata/gdata_download_observer.h"
 
 #include "base/file_util.h"
 #include "chrome/browser/chromeos/gdata/gdata_uploader.h"
 #include "chrome/browser/chromeos/gdata/gdata_upload_file_info.h"
 #include "chrome/browser/chromeos/gdata/gdata_util.h"
@@ skipping 16 matching lines @@
 // External Data stored in DownloadItem for ongoing uploads.
 class UploadingExternalData : public DownloadItem::ExternalData {
  public:
   UploadingExternalData(GDataUploader* uploader, int upload_id)
       : uploader_(uploader),
         upload_id_(upload_id),
         is_complete_(false) {
   }
   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_; }
   GDataUploader* uploader() { return uploader_; }
 
  private:
   GDataUploader* uploader_;
+  base::Closure complete_cb_;
   int upload_id_;
   bool is_complete_;
 };
 
 // External Data stored in DownloadItem for gdata path.
 class GDataExternalData : public DownloadItem::ExternalData {
  public:
   explicit GDataExternalData(const FilePath& path) : file_path_(path) {}
   virtual ~GDataExternalData() {}
 
@@ skipping 64 matching lines @@
 }
 
 // static
 bool GDataDownloadObserver::IsGDataDownload(DownloadItem* download) {
   // We use the existence of the GDataExternalData object in download as a
   // signal that this is a GDataDownload.
   return !!GetGDataExternalData(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
 int64 GDataDownloadObserver::GetUploadedBytes(DownloadItem* download) {
   UploadingExternalData* upload_data = GetUploadingExternalData(download);
   if (!upload_data || !upload_data->uploader())
     return 0;
   return upload_data->uploader()->GetUploadedBytes(upload_data->upload_id());
 }
 
@@ skipping 181 matching lines @@
 void GDataDownloadObserver::OnUploadComplete(int32 download_id,
                                              base::PlatformFileError error,
                                              UploadFileInfo* upload_file_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator iter = pending_downloads_.find(download_id);
   if (iter == pending_downloads_.end()) {
     DVLOG(1) << "Pending download not found" << 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