Omnibox: Call Stop() on all Providers After Certain Amount of Time

Omnibox: Call Stop() on all Providers After Certain Amount of Time

This is to prevent belated omnibox suggestions from disrupting
the user's experience.

This changelist enables this behavior but controls it by a field
trial.  The field trial currently is not created; no users are in.
I will use the server-side variations server to turn it on and control
the size of the experiment population.

Tested by increasing the timer within search provider and then
using --force-fieldtrials to switch myself in and out of the
experiment group and verifying that search provider suggestions
were or were not displayed depending on how late they appeared.

BUG=165252
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195756

[Mark P, 2013-04-19 15:16:59]

Peter: primary review
Sreeram and Bart: FYI, review if you feel so inclined

thanks,
mark

[Peter Kasting, 2013-04-19 19:48:53]

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:613: // late
updates--some providers may ignore the Stop() call--but it
This last sentence worries me.  No one should be ignoring Stop().  Even if, say,
the HQP can't halt its in-process SQL queries, it should ignore the results they
return if things have been Stop()ed.

[Bart N., 2013-04-19 19:57:50]

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.h:228: bool
in_stop_timer_field_trial_;
Make it const.

[Mark P, 2013-04-19 20:20:40]

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:613: // late
updates--some providers may ignore the Stop() call--but it
On 2013/04/19 19:48:53, Peter Kasting wrote:
> This last sentence worries me.  No one should be ignoring Stop().  Even if,
say,
> the HQP can't halt its in-process SQL queries, it should ignore the results
they
> return if things have been Stop()ed.

HCP cannot halt its in-processe SQL queries.  It currently does not have any
special logic to prevent updating its results after stop is called.  Should I
file a bug for this?

Actually, it looks like HCP clears its results unconditionally (even if it is
done) when stop is called.  This looks like a bug to me.  Do you agree?

There are likely problems with other providers too.  In keyword provider I see
extensions that handle omnibox input get routed the OnInputCancelled event, but
I don't see any code that enforces that the keyword extension obeys that and
doesn't allow changes after the fact.  Should I file a bug for this one in
particular too?

Do you want me to revise this comment given that it's true at the moment (even
though you're not happy that it's true)?

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.h:228: bool
in_stop_timer_field_trial_;
On 2013/04/19 19:57:50, Bart N. wrote:
> Make it const.

Done.

[Peter Kasting, 2013-04-19 20:26:17]

LGTM

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:613: // late
updates--some providers may ignore the Stop() call--but it
On 2013/04/19 20:20:40, Mark P wrote:
> On 2013/04/19 19:48:53, Peter Kasting wrote:
> > This last sentence worries me.  No one should be ignoring Stop().  Even if,
> say,
> > the HQP can't halt its in-process SQL queries, it should ignore the results
> they
> > return if things have been Stop()ed.
> 
> HCP cannot halt its in-processe SQL queries.  It currently does not have any
> special logic to prevent updating its results after stop is called.  Should I
> file a bug for this?

Yes.  Once Stop() is called we should not make any changes.  This can cause
problems with cursor navigation.

> Actually, it looks like HCP clears its results unconditionally (even if it is
> done) when stop is called.  This looks like a bug to me.  Do you agree?

Yeah, that sounds wrong.

> There are likely problems with other providers too.  In keyword provider I see
> extensions that handle omnibox input get routed the OnInputCancelled event,
but
> I don't see any code that enforces that the keyword extension obeys that and
> doesn't allow changes after the fact.  Should I file a bug for this one in
> particular too?

Maybe; I'm less certain here because I don't know if such enforcement is
possible.

> Do you want me to revise this comment given that it's true at the moment (even
> though you're not happy that it's true)?

Can we fix any issues here so that we can falsify the comment, before
proceeding?  If not, we should make this comment be something like "NOTE:
Currently not all providers correctly obey Stop(); this is a bug that should be
fixed" or something like that.

[Mark P, 2013-04-19 21:11:25]

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/14366009/diff/3005/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_controller.cc:613: // late
updates--some providers may ignore the Stop() call--but it
On 2013/04/19 20:26:17, Peter Kasting wrote:
> On 2013/04/19 20:20:40, Mark P wrote:
> > On 2013/04/19 19:48:53, Peter Kasting wrote:
> > > This last sentence worries me.  No one should be ignoring Stop().  Even
if,
> > say,
> > > the HQP can't halt its in-process SQL queries, it should ignore the
results
> > they
> > > return if things have been Stop()ed.
> > 
> > HCP cannot halt its in-processe SQL queries.  It currently does not have any
> > special logic to prevent updating its results after stop is called.  Should
I
> > file a bug for this?
> 
> Yes.  Once Stop() is called we should not make any changes.  This can cause
> problems with cursor navigation.

Filed bug.
https://code.google.com/p/chromium/issues/detail?id=233727
Then, as we discussed on the bug, HCP is probably okay.
> 
> > Actually, it looks like HCP clears its results unconditionally (even if it
is
> > done) when stop is called.  This looks like a bug to me.  Do you agree?
> 
> Yeah, that sounds wrong.

Fixed with https://codereview.chromium.org/14267032/

> > There are likely problems with other providers too.  In keyword provider I
see
> > extensions that handle omnibox input get routed the OnInputCancelled event,
> but
> > I don't see any code that enforces that the keyword extension obeys that and
> > doesn't allow changes after the fact.  Should I file a bug for this one in
> > particular too?
> 
> Maybe; I'm less certain here because I don't know if such enforcement is
> possible.
> 
> > Do you want me to revise this comment given that it's true at the moment
(even
> > though you're not happy that it's true)?
> 
> Can we fix any issues here so that we can falsify the comment, before
> proceeding?  If not, we should make this comment be something like "NOTE:
> Currently not all providers correctly obey Stop(); this is a bug that should
be
> fixed" or something like that.

Actually, given our discussion thus far, it sounds like things may be okay. 
Removed the worrying sentence in the comment.

[commit-bot: I haz the power, 2013-04-23 00:17:22]

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

[commit-bot: I haz the power, 2013-04-23 07:46:40]

Change committed as 195756