Include high-fidelity metadata about a download in incident reports.

chrome/browser/safe_browsing/download_protection_service.h

 // Copyright (c) 2012 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.
 //
 // Helper class which handles communication with the SafeBrowsing servers for
 // improved binary download protection.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_SERVICE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_SERVICE_H_
 
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/callback_list.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "url/gurl.h"
 
 
 namespace content {
 class DownloadItem;
 class PageNavigator;
 }
 
 namespace net {
 class URLRequestContextGetter;
 class X509Certificate;
 }  // namespace net
 
 namespace safe_browsing {
+class BinaryFeatureExtractor;
+class ClientDownloadRequest;
 class DownloadFeedbackService;
-class BinaryFeatureExtractor;
 
 // This class provides an asynchronous API to check whether a particular
 // client download is malicious or not.
 class DownloadProtectionService {
  public:
   enum DownloadCheckResult {
     UNKNOWN,
     SAFE,
     DANGEROUS,
     UNCOMMON,
     DANGEROUS_HOST,
     POTENTIALLY_UNWANTED
   };
 
   // Callback type which is invoked once the download request is done.
   typedef base::Callback<void(DownloadCheckResult)> CheckDownloadCallback;
 
+  // A type of callback run on the main thread when a ClientDownloadRequest has
+  // been formed for a download, or when one has not been formed for a supported
+  // download.
+  typedef base::Callback<void(content::DownloadItem*,
+                              const ClientDownloadRequest*)>
+      ClientDownloadRequestCallback;

[robertshield, 2014/10/21 13:15:38]

nit: line-break here to make it clearer the comment doesn't apply to the next
two typedefs.

[grt (UTC plus 2), 2014/10/23 19:53:13]

Done.

+  typedef base::CallbackList<void(content::DownloadItem*,
+                                  const ClientDownloadRequest*)>
+      ClientDownloadRequestCallbackList;

[robertshield, 2014/10/21 13:15:38]

Does the list typedef need to be public?

[grt (UTC plus 2), 2014/10/23 19:53:13]

Yes, since the contained Subscription type is part of the "register a callback"
interface; see below.

[robertshield, 2014/10/23 20:30:27]

The Subscription is but not the SubscriptionList.

[grt (UTC plus 2), 2014/10/23 21:02:18]

The two ways I've found of not making the CallbackList public are to either
remove the ClientDownloadRequestCallbackList typedef altogether or to mix a
private: section in the public: section like this:

  // A type of callback run on the main thread when a ClientDownloadRequest has
  // been formed for a download, or when one has not been formed for a supported
  // download.
  typedef base::Callback<void(content::DownloadItem*,
                              const ClientDownloadRequest*)>
      ClientDownloadRequestCallback;

 private:
  // A list of ClientDownloadRequest callbacks.
  typedef base::CallbackList<void(content::DownloadItem*,
                                  const ClientDownloadRequest*)>
      ClientDownloadRequestCallbackList;

 public:
  // A subscription to a registered ClientDownloadRequest callback.
  typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription>
      ClientDownloadRequestSubscription;

Although ClientDownloadRequestCallbackList doesn't need to be public, I think
it's the more readable of the options that I can think of. Please give a
concrete suggestion if you know of a better way.

[robertshield, 2014/10/23 21:43:03]

Can't you move the list typedef down into the private section?

The style guide states that the typedefs-first ordering rule is per-visibility
section:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

[grt (UTC plus 2), 2014/10/24 01:51:30]

Didn't work when I tried it:

...\download_protection_service.h(66) : error C2653:
'ClientDownloadRequestCallbackList' : is not a class or namespace name

It did work when I defined it in a private: section that preceded its use for
the ClientDownloadRequestSubscription typedef (as above).

So whaddya think: leave it as I had it with it being overly public, introduce an
ugly private section up here for it, or the third thing that I haven't thought
of yet (have you thought of it?)?

[robertshield, 2014/10/24 03:05:46]

Huh, nm then, as is works for me.

+  typedef scoped_ptr<ClientDownloadRequestCallbackList::Subscription>
+      ClientDownloadRequestSubscription;
+
   // Creates a download service.  The service is initially disabled.  You need
   // 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);
 
   virtual ~DownloadProtectionService();
 
   // Checks whether the given client download is likely to be malicious or not.
@@ skipping 35 matching lines @@
 
   // Returns the timeout that is used by CheckClientDownload().
   int64 download_request_timeout_ms() const {
     return download_request_timeout_ms_;
   }
 
   DownloadFeedbackService* feedback_service() {
     return feedback_service_.get();
   }
 
+  // Registers a callback that will be run when a ClientDownloadRequest has
+  // been formed.
+  ClientDownloadRequestSubscription RegisterClientDownloadRequestCallback(
+      const ClientDownloadRequestCallback& callback);
+
  protected:
   // Enum to keep track why a particular download verdict was chosen.
   // This is used to keep some stats around.
   enum DownloadCheckResultReason {
     REASON_INVALID_URL,
     REASON_SB_DISABLED,
     REASON_WHITELISTED_URL,
     REASON_WHITELISTED_REFERRER,
     REASON_INVALID_REQUEST_PROTO,
     REASON_SERVER_PING_FAILED,
@@ skipping 71 matching lines @@
   // Keeps track of the state of the service.
   bool enabled_;
 
   // BinaryFeatureExtractor object, may be overridden for testing.
   scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor_;
 
   int64 download_request_timeout_ms_;
 
   scoped_ptr<DownloadFeedbackService> feedback_service_;
 
+  // A list of callbacks to be run on the main thread when a
+  // ClientDownloadRequest has been formed.
+  ClientDownloadRequestCallbackList client_download_request_callbacks_;
+
   DISALLOW_COPY_AND_ASSIGN(DownloadProtectionService);
 };
 }  // namespace safe_browsing
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_SERVICE_H_