Add detailed histograms for DownloadProtectionService URLFetcher errors.

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 2c5f5901c2a16f35211e953f68a0f5a351bfe2ab..f111dd260c7dceb2ad23e7315c2109639c25721c 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -10,6 +10,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -388,6 +389,18 @@ class DownloadProtectionService::CheckClientDownloadRequest
             << item_->GetUrlChain().back() << ": success="
             << source->GetStatus().is_success() << " response_code="
             << source->GetResponseCode();
+    UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadRequestStatus",
+                              source->GetStatus().status(),
+                              net::URLRequestStatus::STATUS_MAX);

[mmenke, 2013/07/24 14:26:50]

I'm not sure this is needed - this seems pretty redundant with the NetError
histogram.  ERR_ABORTED is the same as CANCELED, OK is the same as SUCCESS.  All
other net errors map to failed.

So if we just have a single histogram with source->GetStatus().error(), that
should cover everything.  Separate histograms may be a bit easier to read, but I
think we suffer from histogram bloat enough as it is.

[mattm, 2013/07/24 22:39:27]

Sounds good, I didn't realize CANCELED would have an error code too.

+    if (source->GetStatus().is_success()) {

[mattm, 2013/07/24 02:19:00]

is "is_success()" right here, or should I check "status() ==
net::URLRequestStatus::SUCCESS" ?

[mmenke, 2013/07/24 14:26:50]

I think is_success() is the way to go.

+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "SBClientDownload.CheckDownloadRequestResponseCode",
+          source->GetResponseCode());
+    } else if (source->GetStatus().status() == net::URLRequestStatus::FAILED) {
+      UMA_HISTOGRAM_SPARSE_SLOWLY(
+          "SBClientDownload.CheckDownloadRequestNetError",
+          source->GetStatus().error());

[mmenke, 2013/07/24 14:26:50]

This should be "-source->GetStatus().error()".  Net error codes are all positive
(Except OK, which is 0), and the histogram code only logs positive values, so
all the histograms with net error codes have to flip the sign.

[mattm, 2013/07/24 22:39:27]

Done.

+    }
     DownloadCheckResultReason reason = REASON_SERVER_PING_FAILED;
     DownloadCheckResult result = SAFE;
     if (source->GetStatus().is_success() &&