Implemented ExternalData interface on DownloadItem and used it for SafeBrowsing.

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 3fec07e7020ead50e96076ac5894ddd46e259064..e93f886072e1512a846d0f472b86f327e85b8f71 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -43,6 +43,22 @@
 using content::BrowserThread;
 using safe_browsing::DownloadProtectionService;
 
+namespace {
+
+// String pointer used for identifying safebrowing data associated with
+// a download item.
+static char safe_browsing_id[] = "Safe Browsing ID";

[noelutz, 2011/11/16 02:59:38]

nit: make that const?

[Randy Smith (Not in Mondays), 2011/11/18 02:02:57]

Done.

+
+// The state of a safebrowsing check.  If there is no data associated with the
+struct SafeBrowsingState : public DownloadItem::ExternalData {
+  // If true the SafeBrowsing check is not done yet.
+  bool pending;
+  // The verdict that we got from calling CheckClientDownload.
+  safe_browsing::DownloadProtectionService::DownloadCheckResult verdict;
+};
+
+}
+
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
     : profile_(profile),
       download_prefs_(new DownloadPrefs(profile->GetPrefs())) {
@@ -155,14 +171,12 @@ bool ChromeDownloadManagerDelegate::ShouldOpenFileBasedOnExtension(
 bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
 #if defined(ENABLE_SAFE_BROWSING)
   // See if there is already a pending SafeBrowsing check for that download.
-  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
-  if (it != safe_browsing_state_.end()) {
-    SafeBrowsingState state = it->second;
-    if (!state.pending) {
-      safe_browsing_state_.erase(it);
-    }
-    return !state.pending;
-  }
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  if (state)
+    // Don't complete the download until we have an answer.
+    return !state->pending;
+
   // Begin the safe browsing download protection check.
   SafeBrowsingService* sb_service =
       g_browser_process->safe_browsing_service();
@@ -176,10 +190,10 @@ bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
             &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
             this,
             item->id()));
-    SafeBrowsingState state;
-    state.pending = true;
-    state.verdict = DownloadProtectionService::SAFE;
-    safe_browsing_state_[item->id()] = state;
+    SafeBrowsingState* state(new SafeBrowsingState());
+    state->pending = true;
+    state->verdict = DownloadProtectionService::SAFE;
+    item->SetExternalData(&safe_browsing_id, state);
     return false;
   }
 #endif
@@ -323,10 +337,8 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
-  if (!item) {
-    safe_browsing_state_.erase(download_id);  // Just in case.
+  if (!item)
     return;
-  }
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
           << " verdict = " << result;
@@ -335,12 +347,11 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
   //  2) make sure we haven't already displayed a warning for the URL.
   //  3) disable the existing dangerous file warning for executables.
 
-  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
-  DCHECK(it != safe_browsing_state_.end() && it->second.pending);
-  if (it != safe_browsing_state_.end()) {
-    it->second.pending = false;
-    it->second.verdict = result;
-  }
+  SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
+      item->GetExternalData(&safe_browsing_id));
+  DCHECK(state);
+  state->pending = false;

[noelutz, 2011/11/16 02:59:38]

nit: add an if here?  Otherwise, this will crash in the release build if state
is NULL.

[Randy Smith (Not in Mondays), 2011/11/18 02:02:57]

I wince, just because when I plot that case forward, the download will hang
(keeps sending out safe browsing requests from ShouldCompleteDownload).  But
that's probably better than crashing.  But it probably can't happen.  Hmmm.  Ok,
if statement.

+  state->verdict = result;
   download_manager_->MaybeCompleteDownload(item);
 }