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;
-  // The verdict that we got from calling CheckClientDownload.
-  safe_browsing::DownloadProtectionService::DownloadCheckResult verdict;
+  bool pending() const { return pending_; }
+
+  // The verdict that we got from calling CheckClientDownload. Only valid to
+  // call if |!pending()|.
+  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(
+    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;

[Randy Smith (Not in Mondays), 2012/05/14 18:30:31]

Isn't this a bug, if a not currently important one because Gdata and safe
browsing don't happen together?  I.e. if you enter this function when safe
browsing is happy and GData is not, won't you return true when you shouldn't?

[benjhayden, 2012/05/15 21:25:28]

Done.

+    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);
 }