DownloadManagerDelegate::ShouldCompleteDownload(callback)

content/browser/download/download_manager_impl.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 "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 621 matching lines @@
 
   // Confirm we're in the proper set of states to be here;
   // in in_progress_, have all data, have a history handle, (validated or safe).
   DCHECK_NE(DownloadItem::DANGEROUS, download->GetSafetyState());
   DCHECK_EQ(1u, in_progress_.count(download->GetId()));
   DCHECK(download->AllDataSaved());
   DCHECK(download->IsPersisted());
   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 = "
            << download->DebugString(false);
 
   // Remove the id from in_progress
   in_progress_.erase(download->GetId());
 
   delegate_->UpdateItemInPersistentStore(download);
 
   // Finish the 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);
   delegate_->UpdateItemInPersistentStore(download);
   active_downloads_.erase(download->GetId());
   AssertStateConsistent(download);
 }
 
 void DownloadManagerImpl::OnDownloadRenamedToFinalName(
     int download_id,
@@ skipping 541 matching lines @@
     if (it->second->IsComplete() && !it->second->GetOpened())
       ++num_unopened;
   }
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
 void DownloadManagerImpl::SetFileManagerForTesting(
       DownloadFileManager* file_manager) {
   file_manager_ = file_manager;
 }