Implemented ExternalData interface on DownloadItem and used it for SafeBrowsing.

chrome/browser/download/chrome_download_manager_delegate.cc

 // Copyright (c) 2011 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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/file_util.h"
@@ skipping 25 matching lines @@
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_status_updater.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/notification_source.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 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())) {
 }
 
 ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
 }
 
 bool ChromeDownloadManagerDelegate::IsExtensionDownload(
     const DownloadItem* item) {
@@ skipping 92 matching lines @@
   if (Extension::IsExtension(path))
     return false;
   DCHECK(extension[0] == FilePath::kExtensionSeparator);
   extension.erase(0, 1);
   return download_prefs_->IsAutoOpenEnabledForExtension(extension);
 }
 
 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();
   if (sb_service && sb_service->download_protection_service() &&
       profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
     VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
             << item->DebugString(false);
     sb_service->download_protection_service()->CheckClientDownload(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
         base::Bind(
             &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
   return true;
 }
 
 bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
   if (!IsExtensionDownload(item)) {
     return true;
   }
@@ skipping 123 matching lines @@
   download_history_->CheckVisitedReferrerBefore(
       download_id, download->referrer_url(),
       base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
                  base::Unretained(this)));
 }
 
 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;
   // TODO(noelutz):
   //  1) display a warning if the result is DANGEROUS.
   //  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);
 }
 
 // content::NotificationObserver implementation.
 void ChromeDownloadManagerDelegate::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(type == chrome::NOTIFICATION_CRX_INSTALLER_DONE);
 
@@ skipping 218 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);
 }