Adds UMA histogram metrics for SafeBrowsing PVer4 methods.

chrome/browser/safe_browsing/protocol_manager.cc

Index: chrome/browser/safe_browsing/protocol_manager.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index 51346e037eecad9022627137d3bcda776bdc0866..5a4858f52a80a22083ebb60e0394109c71a5542c 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -92,6 +92,8 @@ static const int kSbMaxUpdateWaitSec = 30;
 static const size_t kSbMaxBackOff = 8;
 
 const char kUmaHashResponseMetricName[] = "SB2.GetHashResponseOrErrorCode";
+const char kUmaV4ResponseMetricName[] =
+    "SafeBrowsing.V4HttpResponseOrErrorCode";

[Nathan Parker, 2016/01/07 05:28:29]

Should this have "Hash" in it somewhere? (since there will he other V4Http
Responses)

[kcarattini, 2016/01/07 06:06:13]

Done.

 
 // The V4 URL prefix where browser fetches hashes from the V4 server.
 const char kSbV4UrlPrefix[] = "https://safebrowsing.googleapis.com/v4";
@@ -186,6 +188,13 @@ void SafeBrowsingProtocolManager::RecordGetHashResult(bool is_download,
   }
 }
 
+// static
+void SafeBrowsingProtocolManager::RecordGetV4HashResult(
+    ResultType result_type) {
+  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.GetV4HashResult", result_type,
+                            GET_HASH_RESULT_MAX);
+}
+
 void SafeBrowsingProtocolManager::RecordHttpResponseOrErrorCode(
     const char* metric_name,
     const net::URLRequestStatus& status,
@@ -338,7 +347,11 @@ void SafeBrowsingProtocolManager::GetV4FullHashes(
   // proceed with the request. If not, we are required to return empty results
   // (i.e. treat the page as safe).
   if (Time::Now() <= next_gethash_v4_time_) {
-    // TODO(kcarattini): Add UMA recording.
+    if (gethash_v4_error_count_) {
+      RecordGetV4HashResult(GET_HASH_BACKOFF_ERROR);
+    } else {
+      RecordGetV4HashResult(GET_HASH_MIN_WAIT_DURATION_ERROR);
+    }
     std::vector<SBFullHashResult> full_hashes;
     callback.Run(full_hashes, base::TimeDelta());
     return;
@@ -394,10 +407,11 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
   HashRequests::iterator v4_it = v4_hash_requests_.find(source);
   int response_code = source->GetResponseCode();
   net::URLRequestStatus status = source->GetStatus();
-  RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
-                                response_code);
+
   if (it != hash_requests_.end()) {
     // GetHash response.
+    RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
+                                  response_code);
     fetcher.reset(it->first);
     const FullHashDetails& details = it->second;
     std::vector<SBFullHashResult> full_hashes;
@@ -443,27 +457,30 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
     hash_requests_.erase(it);
   } else if (v4_it != v4_hash_requests_.end()) {
     // V4 FindFullHashes response.
+    RecordHttpResponseOrErrorCode(kUmaV4ResponseMetricName, status,
+                                  response_code);
     fetcher.reset(v4_it->first);
     const FullHashDetails& details = v4_it->second;
     std::vector<SBFullHashResult> full_hashes;
     base::TimeDelta negative_cache_duration;
     if (status.is_success() && response_code == net::HTTP_OK) {
-      // TODO(kcarattini): Add UMA reporting.
+      RecordGetV4HashResult(GET_HASH_STATUS_200);
       gethash_v4_error_count_ = 0;
       gethash_v4_back_off_mult_ = 1;
       std::string data;
       source->GetResponseAsString(&data);
       if (!ParseV4HashResponse(data, &full_hashes, &negative_cache_duration)) {
         full_hashes.clear();
-        // TODO(kcarattini): Add UMA reporting.
+        RecordGetV4HashResult(GET_HASH_PARSE_ERROR);
       }
     } else {
       HandleGetHashV4Error(Time::Now());
-      // TODO(kcarattini): Add UMA reporting.
       if (status.status() == net::URLRequestStatus::FAILED) {
+        RecordGetV4HashResult(GET_HASH_NETWORK_ERROR);
         DVLOG(1) << "SafeBrowsing GetEncodedFullHashes request for: " <<
             source->GetURL() << " failed with error: " << status.error();
       } else {
+        RecordGetV4HashResult(GET_HASH_HTTP_ERROR);
         DVLOG(1) << "SafeBrowsing GetEncodedFullHashes request for: " <<
             source->GetURL() << " failed with error: " << response_code;
       }
@@ -477,6 +494,8 @@ void SafeBrowsingProtocolManager::OnURLFetchComplete(
     v4_hash_requests_.erase(it);
   } else {
     // Update or chunk response.

[Nathan Parker, 2016/01/07 05:28:29]

I always thought it was odd that this OnURLFetchComplete() handles both chunk
responses and hash responses, since they are quite different... Do you think
there is an advantage to having the v4 hash handling in here as well, rather
than in a separate class?  If there's no overlap, it could make both simpler by
creating a V4ProtocolManager or some such.  Just a thought.  You could land this
stuff and consider pull it out later.  Or we could leave it here till all the V3
code goes away (quarters).

[kcarattini, 2016/01/07 06:06:13]

Added a TODO to consider pulling this out in the future. There is a small amount
of method and type definition reuse that is useful in keeping these together
e.g. the ResultType definition).

+    RecordHttpResponseOrErrorCode(kUmaHashResponseMetricName, status,
+                                  response_code);
     fetcher.reset(request_.release());
 
     if (request_type_ == UPDATE_REQUEST ||