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 56c36135e8651256f39704df4a547bdd4e5f4f24..d10c705f8cfd5a02cbc653dbfb134489019906fe 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_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)
@@ -204,48 +241,71 @@ void ChromeDownloadManagerDelegate::DisableSafeBrowsing(DownloadItem* item) {
       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
+}
+
+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)
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  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) {
+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)
-  // 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) {
+    // 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;
@@ -466,12 +526,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.