Fix Omnibox search provider's confusing internal variable semantics

Fix Omnibox search provider's confusing internal variable semantics

As a side effect, the count in the histogram Omnibox.SuggestRequests's number of requests invalidated bucket will be more accurate.

BUG=
TEST=by hand using about:histograms


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

[msw, 2012-08-21 17:12:28]

Perhaps add a private util SuggestResultsPending() or similar that checks both?

https://chromiumcodereview.appspot.com/10860068/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10860068/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/search_provider.h:340: bool in_timer_phase_;
Why not remove this and just use timer_.IsRunning()?

[Mark P, 2012-08-21 17:47:51]

Please take another look.

> Perhaps add a private util SuggestResultsPending()
> or similar that checks both?

If it checks both, then this is nearly as misleading as the current code.  For
instance, SuggestResultsPending() == true when we haven't sent off any requests.
 A different function name might make it make more sense but given that it's
only used in one place I don't see the need.

--mark

https://chromiumcodereview.appspot.com/10860068/diff/1/chrome/browser/autocom...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10860068/diff/1/chrome/browser/autocom...
chrome/browser/autocomplete/search_provider.h:340: bool in_timer_phase_;
On 2012/08/21 17:12:28, msw wrote:
> Why not remove this and just use timer_.IsRunning()?

No reason why not.  I didn't know there was that internal variable.

[msw, 2012-08-21 18:30:27]

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.cc:1239: // We're done when the
timer isn't running and there are no suggest queries
grammar nit: lists of 3+ clauses use comma(s) and drop the first "and".

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.h:340: bool in_timer_phase_;
Remove this.

[Mark P, 2012-08-21 19:13:41]

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.cc:1239: // We're done when the
timer isn't running and there are no suggest queries
On 2012/08/21 18:30:27, msw wrote:
> grammar nit: lists of 3+ clauses use comma(s) and drop the first "and".

Done.

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
File chrome/browser/autocomplete/search_provider.h (right):

https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/auto...
chrome/browser/autocomplete/search_provider.h:340: bool in_timer_phase_;
On 2012/08/21 18:30:27, msw wrote:
> Remove this.

Done.

[msw, 2012-08-21 19:16:18]

LGTM; thanks for the cleanup!

https://chromiumcodereview.appspot.com/10860068/diff/10001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10860068/diff/10001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:573:
LogOmniboxSuggestRequest(REQUEST_INVALIDATED);
FYI, we may see a reduction in REQUEST_INVALIDATED.

[commit-bot: I haz the power, 2012-08-21 19:19:51]

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

[commit-bot: I haz the power, 2012-08-21 19:19:54]

Presubmit check for 10860068-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/autocomplete

[Mark P, 2012-08-21 19:36:55]

Peter,

Can I get your OWNERS approval on this?

thanks,
mark

[Peter Kasting, 2012-08-25 17:45:51]

LGTM, but your change description is slightly inaccurate now.

[Mark P, 2012-08-26 01:24:56]

> LGTM, but your change description is
> slightly inaccurate now.

Revised description.

Submitting.

[commit-bot: I haz the power, 2012-08-26 01:25:09]

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

[commit-bot: I haz the power, 2012-08-26 12:21:37]

Change committed as 153416