Omnibox: search provider: fix timing histogram

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 93109ba5ab87143c76322df276d045c9fa68f8a6..db9633d91b94d79cc891acf36cef231b5a3aab23 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -339,16 +339,15 @@ void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
   SuggestResults* suggest_results =
       is_keyword ? &keyword_suggest_results_ : &default_suggest_results_;
 
-  std::string histogram_name =
-      "Omnibox.SuggestRequest.Failure.GoogleResponseTime";
+  bool request_succeeded = false;

[Ilya Sherman, 2012/05/17 22:53:15]

nit: bool request_succeeded = source->GetStatus().is_success() &&
source->GetResponseCode() == 200;

[Mark P, 2012/05/18 13:23:07]

Took that idea and brought it one step further to avoid the same test twice.

   if (source->GetStatus().is_success() && source->GetResponseCode() == 200) {
+    request_succeeded = true;
     JSONStringValueSerializer deserializer(json_data);
     deserializer.set_allow_trailing_comma(true);
     scoped_ptr<Value> root_val(deserializer.Deserialize(NULL, NULL));
     const string16& input = is_keyword ? keyword_input_text_ : input_.text();
     have_suggest_results_ = root_val.get() &&
         ParseSuggestResults(root_val.get(), is_keyword, input, suggest_results);
-    histogram_name = "Omnibox.SuggestRequest.Success.GoogleResponseTime";
   }
 
   // Record response time for suggest requests sent to Google.  We care
@@ -357,8 +356,12 @@ void SearchProvider::OnURLFetchComplete(const net::URLFetcher* source) {
   const TemplateURL* default_url = providers_.GetDefaultProviderURL();
   if (!is_keyword && default_url &&
       (default_url->prepopulate_id() == SEARCH_ENGINE_GOOGLE)) {
-    UMA_HISTOGRAM_TIMES(histogram_name,
-                        base::TimeTicks::Now() - time_suggest_request_sent_);
+    if (request_succeeded)
+      UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Failure.GoogleResponseTime",

[Mark P, 2012/05/18 13:23:07]

Also noticed I had histogram names backwards.

+                          base::TimeTicks::Now() - time_suggest_request_sent_);

[Ilya Sherman, 2012/05/17 22:53:15]

nit: Perhaps compute this once, outside of the UMA_HISTOGRAM_TIMES macros?

[Mark P, 2012/05/18 13:23:07]

Done.

+    else
+      UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Success.GoogleResponseTime",
+                          base::TimeTicks::Now() - time_suggest_request_sent_);

[Ilya Sherman, 2012/05/17 22:53:15]

nit: Please add curly braces to the if-else, since each spans multiple lines.

[Mark P, 2012/05/18 13:23:07]

Done.

   }
 
   ConvertResultsToAutocompleteMatches();