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

chrome/browser/safe_browsing/download_protection_service.h

Index: chrome/browser/safe_browsing/download_protection_service.h
diff --git a/chrome/browser/safe_browsing/download_protection_service.h b/chrome/browser/safe_browsing/download_protection_service.h
index 070c9a388d60f8f306e2634c0d0b10ec49cb0483..2d4db50170db6cfab42e9540e453a24c05a31e2c 100644
--- a/chrome/browser/safe_browsing/download_protection_service.h
+++ b/chrome/browser/safe_browsing/download_protection_service.h
@@ -14,9 +14,11 @@
 
 #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"
@@ -33,8 +35,9 @@ class X509Certificate;
 }  // namespace net
 
 namespace safe_browsing {
-class DownloadFeedbackService;
 class BinaryFeatureExtractor;
+class ClientDownloadRequest;
+class DownloadFeedbackService;
 
 // This class provides an asynchronous API to check whether a particular
 // client download is malicious or not.
@@ -52,6 +55,18 @@ class DownloadProtectionService {
   // 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|.
@@ -107,6 +122,11 @@ class DownloadProtectionService {
     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.
@@ -198,6 +218,10 @@ class DownloadProtectionService {
 
   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