Collect some histograms about signed binary downloads.

chrome/browser/safe_browsing/download_protection_service.h

Index: chrome/browser/safe_browsing/download_protection_service.h
===================================================================
--- chrome/browser/safe_browsing/download_protection_service.h	(revision 107068)
+++ chrome/browser/safe_browsing/download_protection_service.h	(working copy)
@@ -9,36 +9,34 @@
 #define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_SERVICE_H_
 #pragma once
 
-#include <map>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/time.h"
-#include "content/public/common/url_fetcher_delegate.h"
 #include "googleurl/src/gurl.h"
 
 namespace net {
 class URLRequestContextGetter;
 class URLRequestStatus;
 }  // namespace net
+class DownloadItem;
 class SafeBrowsingService;
 
 namespace safe_browsing {
 
 // This class provides an asynchronous API to check whether a particular
 // client download is malicious or not.
-class DownloadProtectionService
-    : public base::RefCountedThreadSafe<DownloadProtectionService>,
-      public content::URLFetcherDelegate {
+class DownloadProtectionService {
  public:
-  // TODO(noelutz): we're missing some fields here: filename to get
-  // the signature, server IPs, tab URL redirect chain, ...
+  // TODO(noelutz): we're missing some fields here: server IPs,
+  // tab URL redirect chain, ...
   struct DownloadInfo {
+    FilePath local_file;
     std::vector<GURL> download_url_chain;
     GURL referrer_url;
     std::string sha256_hash;
@@ -46,6 +44,9 @@
     bool user_initiated;
     DownloadInfo();
     ~DownloadInfo();
+
+    // Creates a DownloadInfo from a DownloadItem object.
+    static DownloadInfo FromDownloadItem(const DownloadItem& item);
   };
 
   enum DownloadCheckResult {
@@ -59,32 +60,25 @@
   typedef base::Callback<void(DownloadCheckResult)> CheckDownloadCallback;
 
   // Creates a download service.  The service is initially disabled.  You need
-  // to call SetEnabled() to start it.  We keep scoped references to both of
-  // these objects.
+  // to call SetEnabled() to start it.  |sb_service| owns this object; we
+  // keep a reference to |request_context_getter|.
   DownloadProtectionService(
       SafeBrowsingService* sb_service,
       net::URLRequestContextGetter* request_context_getter);
 
-  // From the content::URLFetcherDelegate interface.
-  virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE;
+  virtual ~DownloadProtectionService();
 
-  // Checks whether the given client download is likely to be
-  // malicious or not.  If this method returns true it means the
-  // download is safe or we're unable to tell whether it is safe or
-  // not.  In this case the callback will never be called.  If this
-  // method returns false we will asynchronously check whether the
-  // download is safe and call the callback when we have the response.
-  // This method should be called on the UI thread.  The callback will
-  // be called on the UI thread and will always be called asynchronously.
-  virtual bool CheckClientDownload(const DownloadInfo& info,
+  // Checks whether the given client download is likely to be malicious or not.
+  // The result is delivered asynchronously via the given callback.  This
+  // method must be called on the UI thread, and the callback will also be
+  // invoked on the UI thread.
+  virtual void CheckClientDownload(const DownloadInfo& info,

[noelutz, 2011/10/25 21:43:50]

It would be nice to keep the previous API.  Eventually, we're going to call this
method from ShouldCompleteDownload which has the same API.

[Brian Ryner, 2011/10/25 22:03:07]

Is there a drawback to always returning false from ShouldCompleteDownload so
that we can proceed asynchronously?  I feel like it makes things somewhat
cleaner.

[noelutz, 2011/10/25 22:13:42]

Randy might have some opinion on that.  I don't know if it really matters.

                                    const CheckDownloadCallback& callback);
 
   // Enables or disables the service.  This is usually called by the
   // SafeBrowsingService, which tracks whether any profile uses these services
-  // at all.  Disabling cancels any pending requests; existing requests will
-  // have their callbacks called with "SAFE" results.  Note: SetEnabled() is
-  // asynchronous because this method is called on the UI thread but most
-  // everything else happens on the IO thread.
+  // at all.  Disabling causes any pending and future requests to have their
+  // callbacks called with "SAFE" results.
   void SetEnabled(bool enabled);
 
   bool enabled() const {
@@ -102,13 +96,12 @@
     REASON_INVALID_REQUEST_PROTO,
     REASON_SERVER_PING_FAILED,
     REASON_INVALID_RESPONSE_PROTO,
+    REASON_NOT_BINARY_FILE,
     REASON_MAX  // Always add new values before this one.
   };
 
-  virtual ~DownloadProtectionService();
-
  private:
-  friend class base::RefCountedThreadSafe<DownloadProtectionService>;
+  class CheckClientDownloadRequest;  // Per-request state
   friend class DownloadProtectionServiceTest;
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
                            CheckClientDownloadValidateRequest);
@@ -119,25 +112,16 @@
 
   static const char kDownloadRequestUrl[];
 
-  // Same as above but this method is called on the IO thread after we have
-  // done some basic checks to see whether the download is definitely not
-  // safe.
-  void StartCheckClientDownload(const DownloadInfo& info,
-                                const CheckDownloadCallback& callback);
+  // Called by a CheckClientDownloadRequest instance when it finishes, to
+  // remove it from |download_requests_|.
+  void RequestFinished(CheckClientDownloadRequest* request);
 
-  // This function must run on the UI thread and will invoke the callback
-  // with the given result.
-  void EndCheckClientDownload(DownloadCheckResult result,
-                              DownloadCheckResultReason reason,
-                              const CheckDownloadCallback& callback);
+  static void FillDownloadInfo(const DownloadItem& item,
+                               DownloadInfo* download_info);
 
-  void RecordStats(DownloadCheckResultReason reason);
-
-  // SetEnabled(bool) calls this method on the IO thread.
-  void SetEnabledOnIOThread(bool enableed);
-
   // This pointer may be NULL if SafeBrowsing is disabled.
-  scoped_refptr<SafeBrowsingService> sb_service_;
+  // The SafeBrowsingService owns us.
+  SafeBrowsingService* sb_service_;
 
   // The context we use to issue network requests.
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
@@ -145,7 +129,7 @@
   // Map of client download request to the corresponding callback that
   // has to be invoked when the request is done.  This map contains all
   // pending server requests.
-  std::map<const URLFetcher*, CheckDownloadCallback> download_requests_;
+  std::set<scoped_refptr<CheckClientDownloadRequest> > download_requests_;
 
   // Keeps track of the state of the service.
   bool enabled_;