Consolidate some safe-browsing histograms.

chrome/browser/safe_browsing/protocol_manager.h

 // Copyright (c) 2010 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.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_PROTOCOL_MANAGER_H_
 #define CHROME_BROWSER_SAFE_BROWSING_PROTOCOL_MANAGER_H_
 #pragma once
 
 // A class that implements Chrome's interface with the SafeBrowsing protocol.
 // The SafeBrowsingProtocolManager handles formatting and making requests of,
@@ skipping 140 matching lines @@
   // be changed in the middle of an update, caller (e.g.: SafeBrowsingService)
   // should call this after callbacks triggered in UpdateFinished() or before
   // IssueUpdateRequest().
   void set_additional_query(const std::string& query) {
     additional_query_ = query;
   }
   const std::string& additional_query() const {
     return additional_query_;
   }
 
+  // Enumerate failures for histogramming purposes.  DO NOT CHANGE THE
+  // ORDERING OF THESE VALUES.
+  enum ResultType {
+    GET_HASH_STATUS_200,

[Erik does not do reviews, 2010/12/22 19:29:19]

Is it worth tracking other status codes here?  It might be interesting to get an
idea of how often people are being affected by odd errors (proxy auth, 500s,
etc.).  Or maybe that should be a separate histogram?

[Scott Hess - ex-Googler, 2010/12/22 20:41:51]

Hmm.  I vote for a separate histogram.  It can enumerate the codes in a generic
fashion so the we could either have a histogram for all safe-browsing requests,
or one for each different request, all using the same layout.  Also, it's really
more of a transit-level bit of information, whereas this is more of a
protocol-level item.

+    GET_HASH_STATUS_204,
+    GET_HASH_NO_HITS,  // None of the full hashes matched.

[lzheng, 2010/12/22 00:24:58]

Can you add some more comments to clarify that GET_HASH_NO_HITS could be hit for
both GET_HASH_STATUS_200 and GET_HASH_STATUS_204? Otherwise, it could be
misunderstood that the three enum covers three exclusive sets.

[Scott Hess - ex-Googler, 2010/12/22 20:41:51]

In thinking this through, my understanding is that 204 has an empty body, so
full_hashes will be empty, so the current value will be GET_HASH_STATUS_204 +
epsilon.  WDYT of inverting this to instead measure the number of hits, which
will be strictly a subset of GET_HASH_STATUS_200?

[Scott Hess - ex-Googler, 2010/12/22 20:49:36]

Glad I thought more, because GET_HASH_STATUS_200 can also return no full hashes.
 This is the case we were recently in, I think, where the prefix was recognized
but had been logically deleted on the server side.  I think that's reasonable to
track separately from real misses.

+
+    // Memory space for histograms is determined by the max.  ALWAYS
+    // ADD NEW VALUES BEFORE THIS ONE.
+    GET_HASH_RESULT_MAX
+  };
+
+  // Record a GetHash result.
+  static void RecordGetHashResult(ResultType result_type);
+
  protected:
   // Constructs a SafeBrowsingProtocolManager for |sb_service| that issues
   // network requests using |request_context_getter|. When |disable_auto_update|
   // is true, protocol manager won't schedule next update until
   // ForceScheduleNextUpdate is called.
   SafeBrowsingProtocolManager(SafeBrowsingService* sb_service,
                               const std::string& client_name,
                               const std::string& client_key,
                               const std::string& wrapped_key,
                               URLRequestContextGetter* request_context_getter,
@@ skipping 199 matching lines @@
   std::string https_url_prefix_;
 
   // When true, protocol manager will not start an update unless
   // ForceScheduleNextUpdate() is called. This is set for testing purpose.
   bool disable_auto_update_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingProtocolManager);
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_PROTOCOL_MANAGER_H_