This CL integrates the new SafeBrowsing download service class

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index cd7052d7ed8e65c510d2fe76333da381c199b6a5..dac1c57d03a57ec5e5d22a8cf22951bd9d6e128b 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include "base/bind.h"
+#include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
@@ -112,12 +113,32 @@ DownloadProtectionService::DownloadInfo::DownloadInfo()
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
+std::string DownloadProtectionService::DownloadInfo::DebugString() const {
+  std::string chain = "";

[Brian Ryner, 2011/11/14 22:10:39]

Not necessary to initialize strings to empty.

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

Done.

+  for (size_t i = 0; i < download_url_chain.size(); ++i) {
+    chain += download_url_chain[i].spec();
+    if (i < download_url_chain.size() - 1) {
+      chain += " -> ";
+    }
+  }
+  return base::StringPrintf(
+      "DownloadInfo {download_url_chain:[%s], local_file:%s, "

[Brian Ryner, 2011/11/14 22:10:39]

It might be useful to include the address of the object (this) as well.

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

Done.

+      "referrer_url:%s, sha256_hash:%s, total_bytes:%" PRId64 ", "
+      "user_initiated: %s}",
+      chain.c_str(),
+      local_file.value().c_str(),
+      referrer_url.spec().c_str(),
+      "TODO",
+      total_bytes,
+      user_initiated ? "true" : "false");
+}
+
 // static
 DownloadProtectionService::DownloadInfo
 DownloadProtectionService::DownloadInfo::FromDownloadItem(
     const DownloadItem& item) {
   DownloadInfo download_info;
-  download_info.local_file = item.full_path();
+  download_info.local_file = item.GetTargetFilePath();

[Brian Ryner, 2011/11/14 22:10:39]

Can you explain what this change is for?

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

Woops.  I don't think this will work.  GetTargetFilePath() returns the file path
where the file will eventually written. full_path() is where there file is
written right now.  For exe those are different paths.

Changed since we need both.

   download_info.download_url_chain = item.url_chain();
   download_info.referrer_url = item.referrer_url();
   // TODO(bryner): Fill in the hash (we shouldn't compute it again)
@@ -161,7 +182,7 @@ class DownloadSBClient
                             FROM_HERE,
                             base::Bind(callback_, result));
     UpdateDownloadCheckStats(total_type_);
-    if (IsDangerous(sb_result)) {
+    if (sb_result != SafeBrowsingService::SAFE) {
       UpdateDownloadCheckStats(dangerous_type_);
       BrowserThread::PostTask(
           BrowserThread::UI,
@@ -269,7 +290,10 @@ class DownloadHashSBClient : public DownloadSBClient {
 
   virtual bool IsDangerous(
       SafeBrowsingService::UrlCheckResult result) const OVERRIDE {
-    return result == SafeBrowsingService::BINARY_MALWARE_HASH;
+    // We always return false here because we don't want to warn based on
+    // a match with the digest list.  However, for UMA users, we want to

[Brian Ryner, 2011/11/14 22:10:39]

I'm not sure I follow the comment -- what part of the code ensures that we are
reporting the malware URL for UMA users?

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

Updated the comment.  See CheckDone() above too.

+    // report the malware URL.
+    return false;
   }
 
   virtual void OnDownloadHashCheckResult(
@@ -309,6 +333,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
   }
 
   void Start() {
+    VLOG(2) << "Starting SafeBrowsing download check for: "
+            << info_.DebugString();
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     // TODO(noelutz): implement some cache to make sure we don't issue the same
     // request over and over again if a user downloads the same binary multiple
@@ -326,8 +352,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
     }
     RecordFileExtensionType(info_.local_file);
 
-    if (!final_url.SchemeIs("http") || !IsBinaryFile(info_.local_file)) {
-      RecordImprovedProtectionStats(!final_url.SchemeIs("http") ?
+    if (final_url.SchemeIs("https") || !IsBinaryFile(info_.local_file)) {

[Brian Ryner, 2011/11/14 22:10:39]

So this might cause us to start checking non-HTTP urls that are not HTTPS, like
FTP.  I assume that's ok though?

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

I think that's what we want.

+      RecordImprovedProtectionStats(final_url.SchemeIs("https") ?
                   REASON_HTTPS_URL : REASON_NOT_BINARY_FILE);
       BrowserThread::PostTask(
           BrowserThread::IO,