Omnibox: search provider: fix timing histogram

Omnibox: search provider: fix timing histogram

The current implementation of the timing histogram causes a DCHECK when search provider suggest requests switch from success to failure or vice versa.  This fixes the problem.

I cannot test this to see if it works.  I think it should.  (The only machine I have configured to build chrome on is remote, so I cannot play with whether its network is enabled or not lest I lose my connection to it permanently.)

BUG=128613
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137943

[Mark P, 2012-05-17 22:26:20]

Also, a bigger favor: can you patch this and test it for me?  I think it works
but I'm not set up to test it myself.

thanks,
mark

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

LGTM (with nits).  Tested on my Mac, no DCHECKs hit.

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:342: bool request_succeeded =
false;
nit: bool request_succeeded = source->GetStatus().is_success() &&
source->GetResponseCode() == 200;

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:361: base::TimeTicks::Now() -
time_suggest_request_sent_);
nit: Perhaps compute this once, outside of the UMA_HISTOGRAM_TIMES macros?

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:364: base::TimeTicks::Now() -
time_suggest_request_sent_);
nit: Please add curly braces to the if-else, since each spans multiple lines.

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

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:342: bool request_succeeded =
false;
On 2012/05/17 22:53:15, Ilya Sherman wrote:
> nit: bool request_succeeded = source->GetStatus().is_success() &&
> source->GetResponseCode() == 200;

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

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:360:
UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Failure.GoogleResponseTime",
Also noticed I had histogram names backwards.

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:361: base::TimeTicks::Now() -
time_suggest_request_sent_);
On 2012/05/17 22:53:15, Ilya Sherman wrote:
> nit: Perhaps compute this once, outside of the UMA_HISTOGRAM_TIMES macros?

Done.

http://codereview.chromium.org/10382224/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:364: base::TimeTicks::Now() -
time_suggest_request_sent_);
On 2012/05/17 22:53:15, Ilya Sherman wrote:
> nit: Please add curly braces to the if-else, since each spans multiple lines.

Done.

[Ilya Sherman, 2012-05-18 18:41:13]

(Still LGTM)

[commit-bot: I haz the power, 2012-05-18 18:45:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/10382224/4001

[commit-bot: I haz the power, 2012-05-18 20:27:12]

Change committed as 137943