This CL integrates the new SafeBrowsing download service class

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 b7b4af6fdca4bb335febbcf51ce79690289d6a27..5885f6778a7fec7f4626a7e3746928d7e64b67c6 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -11,13 +11,13 @@
 #include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
+#include "base/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_picker.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_prefs.h"
-#include "chrome/browser/download/download_safe_browsing_client.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/download/save_package_file_picker.h"
 #include "chrome/browser/extensions/crx_installer.h"
@@ -41,6 +41,7 @@
 #include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
+using safe_browsing::DownloadProtectionService;
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
     : profile_(profile),
@@ -86,17 +87,22 @@ bool ChromeDownloadManagerDelegate::ShouldStartDownload(int32 download_id) {
     return false;
 
 #if defined(ENABLE_SAFE_BROWSING)

[Randy Smith (Not in Mondays), 2011/11/14 19:43:00]

Remind me what the context around this define is?  Under what circumstances do
we want to compile out safe browsing?  (Just asking for context).

[noelutz, 2011/11/14 20:30:42]

SafeBrowsing is currently disabled (and compiled out) for ChromeOs.  There might
also be other targets that do not compile SafeBrowsing but ChromeOs is the main
one.

-  // Create a client to verify download URL with safebrowsing.
-  // It deletes itself after the callback.
-  scoped_refptr<DownloadSBClient> sb_client = new DownloadSBClient(
-      download_id, download->url_chain(), download->referrer_url(),
-          profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled));
-  sb_client->CheckDownloadUrl(
-      base::Bind(&ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
-                 base::Unretained(this)));
-#else
-  CheckDownloadUrlDone(download_id, false);
+  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 URL check for download = "
+            << download->DebugString(false);
+    sb_service->download_protection_service()->CheckDownloadUrl(
+        DownloadProtectionService::DownloadInfo::FromDownloadItem(*download),
+        base::Bind(
+            &ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
+            this,
+            download->id()));
+    return false;
+  }
 #endif
+  CheckDownloadUrlDone(download_id, DownloadProtectionService::SAFE);
   return false;
 }
 
@@ -147,29 +153,40 @@ 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;
+  }
+  // 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;
+    safe_browsing_state_[item->id()] = state;
+    return false;
+  }
+#endif
   return true;
 }
 
 bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
   if (!IsExtensionDownload(item)) {
-#if defined(ENABLE_SAFE_BROWSING)
-    // 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)) {
-      using safe_browsing::DownloadProtectionService;
-      sb_service->download_protection_service()->CheckClientDownload(
-          DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
-          base::Bind(
-              &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
-              this, item->id()));
-      // For now, we won't delay the download for this.
-    }
-#else
-    // Assume safe.
-#endif
-
     return true;
   }
 
@@ -201,24 +218,7 @@ bool ChromeDownloadManagerDelegate::GenerateFileHash() {
 }
 
 void ChromeDownloadManagerDelegate::OnResponseCompleted(DownloadItem* item) {
-#if defined(ENABLE_SAFE_BROWSING)
-  // When hash is not available, it means either it is not calculated
-  // or there is error while it is calculated. We will skip the download hash
-  // check in that case.
-  if (item->hash().empty())
-    return;
-
-  scoped_refptr<DownloadSBClient> sb_client =
-      new DownloadSBClient(item->id(),
-                           item->url_chain(),
-                           item->referrer_url(),
-                           profile_->GetPrefs()->GetBoolean(
-                               prefs::kSafeBrowsingEnabled));
-  sb_client->CheckDownloadHash(
-      item->hash(),
-      base::Bind(&ChromeDownloadManagerDelegate::CheckDownloadHashDone,
-                 base::Unretained(this)));
-#endif
+  // TODO(noelutz): remove this method from the delegate API.
 }
 
 void ChromeDownloadManagerDelegate::AddItemToPersistentStore(
@@ -239,12 +239,6 @@ void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore(
   download_history_->UpdateDownloadPath(item, new_path);
 }
 
-void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
-    int32 download_id,
-    safe_browsing::DownloadProtectionService::DownloadCheckResult result) {
-  // TODO(bryner): notify the user based on this result
-}
-
 void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore(
     DownloadItem* item) {
   download_history_->RemoveEntry(item);
@@ -306,15 +300,16 @@ void ChromeDownloadManagerDelegate::DownloadProgressUpdated() {
 
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
-    bool is_dangerous_url) {
+    DownloadProtectionService::DownloadCheckResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return;
 
-  if (is_dangerous_url)
+  VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false)
+          << " verdict = " << result;
+  if (result == DownloadProtectionService::DANGEROUS)
     download->MarkUrlDangerous();
 
   download_history_->CheckVisitedReferrerBefore(
@@ -323,6 +318,29 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
                  base::Unretained(this)));
 }
 
+void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
+    int32 download_id,
+    DownloadProtectionService::DownloadCheckResult result) {
+  DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
+  if (!item)

[Randy Smith (Not in Mondays), 2011/11/14 22:24:22]

There's a (minor) leak here if the DownloadItem's gone away; you haven't cleaned
up the state in the map.  That'll fix itself when we switch to the new storage
model, but you might want to fix it here as well.

[noelutz, 2011/11/14 22:26:35]

I think we're OK. There aren't any pointers in the map.  Everything should be
deleted by the destructor as far as I know.

[noelutz, 2011/11/15 00:14:10]

Fixed.

+    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;
+  }
+  download_manager_->MaybeCompleteDownload(item);
+}
+
 // content::NotificationObserver implementation.
 void ChromeDownloadManagerDelegate::Observe(
     int type,
@@ -554,14 +572,3 @@ void ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore(
     db_handle = download_history_->GetNextFakeDbHandle();
   download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
 }
-
-// TODO(noelutz): This function currently works as a callback place holder.
-// Once we decide the hash check is reliable, we could move the
-// MaybeCompleteDownload in OnAllDataSaved to this function.
-void ChromeDownloadManagerDelegate::CheckDownloadHashDone(
-    int32 download_id,
-    bool is_dangerous_hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DVLOG(1) << "CheckDownloadHashDone, download_id: " << download_id
-           << " is dangerous_hash: " << is_dangerous_hash;
-}