DownloadManagerDelegate::ShouldCompleteDownload(callback)

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index e069eddcbe8445310dd4b054cd0f2370aa27cdcd..7992f196abcce7ee55a0abb859342d153dd577c5 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -69,13 +69,50 @@ namespace {
 static const char safe_browsing_id[] = "Safe Browsing ID";
 
 // The state of a safebrowsing check.
-struct SafeBrowsingState : public DownloadItem::ExternalData {
+class SafeBrowsingState : public DownloadItem::ExternalData {
+ public:
+  explicit SafeBrowsingState(const base::Closure& complete_cb)
+    : pending_(true),
+      verdict_(DownloadProtectionService::SAFE),
+      complete_cb_(complete_cb) {
+  }
+
+  virtual ~SafeBrowsingState();
+
   // If true the SafeBrowsing check is not done yet.
-  bool pending;
+  bool pending() const { return pending_; }
+
   // The verdict that we got from calling CheckClientDownload.
-  safe_browsing::DownloadProtectionService::DownloadCheckResult verdict;
+  DownloadProtectionService::DownloadCheckResult verdict() const {
+    return verdict_;
+  }
+
+  // |cb| will be run when CompleteDownload() is called.
+  void set_complete_callback(const base::Closure& cb) {
+    // Be very careful to avoid calling cb twice, or more than one cb!
+    if (pending())
+      complete_cb_ = cb;
+  }
+
+  void CompleteDownload(DownloadProtectionService::DownloadCheckResult result) {
+    pending_ = false;
+    verdict_ = result;
+    if (!complete_cb_.is_null()) {
+      complete_cb_.Run();
+      complete_cb_.Reset();
+    }
+  }
+
+ private:
+  bool pending_;
+  DownloadProtectionService::DownloadCheckResult verdict_;
+  base::Closure complete_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingState);
 };
 
+SafeBrowsingState::~SafeBrowsingState() {}
+
 }  // namespace
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
@@ -154,11 +191,11 @@ void ChromeDownloadManagerDelegate::ChooseDownloadPath(
     int32 download_id) {
   // Deletes itself.
 #if defined(OS_CHROMEOS)
-  new DownloadFilePickerChromeOS(
+  new DownloadFilePickerChromeOS
 #else
-  new DownloadFilePicker(
+  new DownloadFilePicker
 #endif
-      download_manager_, web_contents, suggested_path, download_id);
+      (download_manager_, web_contents, suggested_path, download_id);
 }
 
 FilePath ChromeDownloadManagerDelegate::GetIntermediatePath(
@@ -193,23 +230,29 @@ bool ChromeDownloadManagerDelegate::ShouldOpenFileBasedOnExtension(
   return download_prefs_->IsAutoOpenEnabledForExtension(extension);
 }
 
-bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
+bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(
+    DownloadItem* item,
+    const base::Closure& complete_cb) {
 #if defined(ENABLE_SAFE_BROWSING)
   // See if there is already a pending SafeBrowsing check for that download.
   SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
       item->GetExternalData(&safe_browsing_id));
-  if (state)
+  if (state) {
+    // Allow calling ShouldCompleteDownload() first with a null callback, then
+    // with a non-null callback.
+    if (!complete_cb.is_null())

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

Same comment as for gdata; why not pass along a null callback?  It feels like
you're violating your interface (that last complete_cb wins) if you don't.

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

"You can't unset it once you set it" was supposed to clarify the interface. See
my response to the other version of this comment.

+      state->set_complete_callback(complete_cb);
+
     // Don't complete the download until we have an answer.
-    return !state->pending;
+    return !state->pending();

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

nit, suggestion: Naively, I'd expect this code not to set complete_cb if it
returns true.  I don't think it matters in terms of behavior, but it seems a
little weird to do.

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

Done.

+  }
 
   // Begin the safe browsing download protection check.
   DownloadProtectionService* service = GetDownloadProtectionService();
   if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
             << item->DebugString(false);
-    state = new SafeBrowsingState();
-    state->pending = true;
-    state->verdict = DownloadProtectionService::SAFE;
+    state = new SafeBrowsingState(complete_cb);
     item->SetExternalData(&safe_browsing_id, state);
     service->CheckClientDownload(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
@@ -223,7 +266,8 @@ bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
 #if defined(OS_CHROMEOS)
   // If there's a GData upload associated with this download, we wait until that
   // is complete before allowing the download item to complete.
-  if (!gdata::GDataDownloadObserver::IsReadyToComplete(item))
+  if (!gdata::GDataDownloadObserver::IsReadyToComplete(
+        item, complete_cb))
     return false;
 #endif
   return true;
@@ -447,12 +491,7 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
 
   SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
       item->GetExternalData(&safe_browsing_id));
-  DCHECK(state);
-  if (state) {
-    state->pending = false;
-    state->verdict = result;
-  }
-  item->MaybeCompleteDownload();
+  state->CompleteDownload(result);
 }
 
 // content::NotificationObserver implementation.