Omnibox: search provider: fix timing histogram

chrome/browser/autocomplete/search_provider.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 321 matching lines @@
                                 base::OnStringConversionError::FAIL,
                                 &data_16))
         json_data = UTF16ToUTF8(data_16);
     }
   }
 
   bool is_keyword = (source == keyword_fetcher_.get());
   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
   // only about the common case: the Google default provider used in
   // non-keyword mode.
   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();
   listener_->OnProviderUpdate(!suggest_results->empty());
 }
 
 SearchProvider::~SearchProvider() {
 }
 
 void SearchProvider::DoHistoryQuery(bool minimal_changes) {
@@ skipping 612 matching lines @@
 
   return match;
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when there are no more suggest queries pending (this is set to 1
   // when the timer is started) and we're not waiting on instant.
   done_ = ((suggest_results_pending_ == 0) &&
            (instant_finalized_ || !InstantController::IsEnabled(profile_)));
 }