DownloadManagerDelegate::ShouldCompleteDownload(callback)

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 9bdb48aa7bfbe2382d1e3b34056dbf0f6c79d308..4ed59b845dc50b01d0205031ccd303676280fad0 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -639,7 +639,13 @@ void DownloadManagerImpl::MaybeCompleteDownload(DownloadItem* download) {
   DCHECK_EQ(1u, history_downloads_.count(download->GetDbHandle()));
 
   // Give the delegate a chance to override.
-  if (!delegate_->ShouldCompleteDownload(download))
+  // It's ok to keep re-setting the delegate's |maybe_complete_download| cb as
+  // long as there isn't another call-point trying to set it to a different cb.
+  // TODO(benjhayden): Change the callback to point directly to the item instead
+  // of |this| when DownloadItem supports weak-ptrs.
+  if (!delegate_->ShouldCompleteDownload(download, base::Bind(
+          &DownloadManagerImpl::MaybeCompleteDownloadById,

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

Hmmm.  I see why you're doing this.  Another alternative might be to pass in a
callback that expected to have a DownloadItem* passed into it, and rely on the
caller to provide that.  If they can't, they shouldn't be calling us, and all
current callers can (sorta obviously, since they already were.)  Neither
super-clean; your TODO points the way to good code.  But I wanted to at least
float the option that doesn't require adding a new routine.  Either way is fine.

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

The new routine with its lookup seems safer.

+          this, download->GetId())))
     return;
 
   VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
@@ -654,6 +660,12 @@ void DownloadManagerImpl::MaybeCompleteDownload(DownloadItem* download) {
   download->OnDownloadCompleting(file_manager_);
 }
 
+void DownloadManagerImpl::MaybeCompleteDownloadById(int download_id) {
+  DownloadItem* download_item = GetActiveDownload(download_id);
+  if (download_item != NULL)
+    MaybeCompleteDownload(download_item);
+}
+
 void DownloadManagerImpl::DownloadCompleted(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);