DownloadManagerDelegate::ShouldCompleteDownload(callback)

chrome/browser/download/chrome_download_manager_delegate.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/download/chrome_download_manager_delegate.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 51 matching lines @@
 using content::WebContents;
 using safe_browsing::DownloadProtectionService;
 
 namespace {
 
 // String pointer used for identifying safebrowing data associated with
 // a download item.
 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_callback)
+    : pending_(true),
+      verdict_(DownloadProtectionService::SAFE),
+      complete_callback_(complete_callback) {
+  }
+
+  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.

[Randy Smith (Not in Mondays), 2012/05/09 00:50:56]

nit, suggestion: Add "Only valid to call if |!pending()|".

[benjhayden, 2012/05/14 15:35:05]

Done.

-  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_callback_ = cb;
+  }
+
+  void CompleteDownload(DownloadProtectionService::DownloadCheckResult result) {
+    pending_ = false;
+    verdict_ = result;
+    if (!complete_callback_.is_null()) {
+      complete_callback_.Run();
+      complete_callback_.Reset();
+    }
+  }
+
+ private:
+  bool pending_;
+  DownloadProtectionService::DownloadCheckResult verdict_;
+  base::Closure complete_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingState);
 };
 
+SafeBrowsingState::~SafeBrowsingState() {}
+
 }  // namespace
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
     : profile_(profile),
       next_download_id_(0),
       download_prefs_(new DownloadPrefs(profile->GetPrefs())) {
 }
 
 ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
 }
@@ skipping 108 matching lines @@
   return download_prefs_->IsAutoOpenEnabledForExtension(extension);
 }
 
 // static
 void ChromeDownloadManagerDelegate::DisableSafeBrowsing(DownloadItem* item) {
 #if defined(ENABLE_SAFE_BROWSING)
   SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
       item->GetExternalData(&safe_browsing_id));
   DCHECK(state == NULL);
   if (state == NULL) {
-    state = new SafeBrowsingState();
-    item->SetExternalData(&safe_browsing_id, state);
+    state = new SafeBrowsingState(base::Closure());
   }
-  state->pending = false;
-  state->verdict = DownloadProtectionService::SAFE;
+  state->CompleteDownload(DownloadProtectionService::SAFE);
 #endif
 }
 
-bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
+void ChromeDownloadManagerDelegate::ShouldCompleteDownloadInternal(

[Randy Smith (Not in Mondays), 2012/05/09 00:50:56]

The splits of callback setup code for the subsystems between these two functions
feels weird to me.  We pass callbacks to GData in both functions, and safe
browsing only in ShouldCompleteDownload.  and there's a reasonable amount of
code and ifdef duplication.  Can we refactor this to be a bit cleaner?  I'm
thinking something like:

IsDownloadReadyForCompletion: Returns true if ready, otherwise setups up passed
in callback to be called back (by whichever subsystem is responsible for the
block).

ShouldCompleteDownloadInternal: Probes IsDownloadReadyForCompletion() and calls
the user callback if it returns true.  

Possibly IsDownloadReadyForCompletion could be spelled ShouldCompleteDownload. 
This all might require storing the user complete callback in the external data
on the download item, or possibly there's callback magic that could get around
that.

[benjhayden, 2012/05/14 15:35:05]

Sorry, I don't see how that's different from what's already here.

WDYT of an ExternalData intermediate base class, and maybe a shadow
GDataDownloadObserver::IsReadyToComplete() that does not take a callback?

+    DownloadItem* item,
+    const base::Closure& user_complete_callback) {
 #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));
-  // Don't complete the download until we have an answer.
-  if (state && state->pending)
+  if (state && state->pending())
+    return;
+#endif
+#if defined(OS_CHROMEOS)
+  const base::Closure& internal_complete_callback = base::Bind(
+      &ChromeDownloadManagerDelegate::ShouldCompleteDownloadInternal, this,
+      item, user_complete_callback);
+  if (!gdata::GDataDownloadObserver::IsReadyToComplete(
+        item, internal_complete_callback))
+    return;
+#endif
+  user_complete_callback.Run();
+}
+
+bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(
+    DownloadItem* item,
+    const base::Closure& user_complete_callback) {
+  const base::Closure& internal_complete_callback = base::Bind(
+      &ChromeDownloadManagerDelegate::ShouldCompleteDownloadInternal, this,
+      item, user_complete_callback);
+#if defined(ENABLE_SAFE_BROWSING)
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  if (state) {
+    // Don't complete the download until we have an answer.
+    if (!state->pending())
+      return true;
+    state->set_complete_callback(internal_complete_callback);
     return false;
+  }
 
-  if (state == NULL) {
-    // 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;
-      item->SetExternalData(&safe_browsing_id, state);
-      service->CheckClientDownload(
-          DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
-          base::Bind(
-              &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
-              this,
-              item->GetId()));
-      return false;
-    }
+  // 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(internal_complete_callback);
+    item->SetExternalData(&safe_browsing_id, state);
+    service->CheckClientDownload(
+        DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
+        base::Bind(
+            &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
+            this,
+            item->GetId()));
+    return false;
   }
 #endif
 
 #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, internal_complete_callback))
     return false;
 #endif
   return true;
 }
 
 bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
   if (IsExtensionDownload(item)) {
     scoped_refptr<CrxInstaller> crx_installer =
         download_crx_util::OpenChromeExtension(profile_, *item);
 
@@ skipping 200 matching lines @@
         item->SetDangerType(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT);
         break;
       case DownloadProtectionService::UNCOMMON:
         item->SetDangerType(content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT);
         break;
     }
   }
 
   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.
 void ChromeDownloadManagerDelegate::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(type == chrome::NOTIFICATION_CRX_INSTALLER_DONE);
 
   registrar_.Remove(this,
@@ skipping 234 matching lines @@
     int32 download_id, int64 db_handle) {
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadItem::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
   download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
 }