Omnibox - Make Omnibox Extensions Ignore Stop() Calls Due to User Idleness

Omnibox - Make Omnibox Extensions Ignore Stop() Calls Due to User Idleness

We normally send all providers a Stop() call after a certain amount of time
(1.5 seconds I think) under the rationale that revising the suggestions
list that late is disruptive to the user's experience.

This doesn't make sense for explicitly-invoke omnibox extensions, which
could be doing long-running processes under the hood.

This change revises the AutocompleteProvider API to allow providers to
differentiate these two types of Stop() calls.  It makes KeywordProvider
ignore the Stop() calls due to this inactivity timer.

BUG=462501
Committed: https://crrev.com/8a37c38f75c3f9c7542fc93d349af5fc3617753e
Cr-Commit-Position: refs/heads/master@{#319557}

[Mark P, 2015-03-05 20:41:07]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2015-03-05 20:43:18]

If you're okay with this approach, I'll convert the unit tests to reflect the
revised API.

--mark

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.cc (left):

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.cc:303: for
(Providers::const_iterator i(providers_.begin()); i != providers_.end();
This is a direct move below except for revising the Stop() call.

[Peter Kasting, 2015-03-05 23:27:28]

LGTM

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.h:205: bool
user_inactivity_timer);
Nit: How about "due_to_user_inactivity" or "from_inactivity_timer" or something?
 I mostly want to convert this arg from just the name of a timer into a
descriptive reason.

(Also applies to AutocompleteProvider::Stop())

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
File components/omnibox/autocomplete_provider.h (right):

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
components/omnibox/autocomplete_provider.h:156: // be sent.
Nit: How about:

Once Stop() has been called, no more notifications should usually be sent (see
comments on Stop() below).

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
components/omnibox/autocomplete_provider.h:176: // cache based on the value of
|clear_cached_results|.
Nit: How about:

Advises the provider to stop processing.  This may be called even if the
provider is already done.  If the provider caches any results, it should clear
the cache based on the value of |clear_cached_results|.  Normally, once this is
called, the provider should not send more notifications to the controller.

If |user_inactivity_timer| is true, Stop() is being called because it's been a
long time since the user started the current query, and returning further
asynchronous results would normally just be disruptive.  Most providers should
still stop processing in this case, but continuing is legal if there's a good
reason the user is likely to want even long-delayed asynchronous results, e.g.
the user has explicitly invoked a keyword extension and the extension is still
processing the request.

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
File components/omnibox/keyword_provider.cc (right):

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
components/omnibox/keyword_provider.cc:323: // Only end an extensions request if
the user did something explicitly
Nit: extension's; explicitly to -> to explicitly

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
components/omnibox/keyword_provider.cc:324: // to cancel it; the inactivity
timer does not count.
Nit: does not count -> shouldn't terminate long-running extension operations,
since the user likely explicity requested them

(at least, I think that's a good rationale)

[Mark P, 2015-03-06 21:24:34]

Please take another look.

Also, I converted the unit tests to use the new API.  (Surprising to me, only
one needed to be converted!)

Furthermore, I tested this interactively using the extensions provided in the
bug report.

--mark

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/985503002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.h:205: bool
user_inactivity_timer);
On 2015/03/05 23:27:28, Peter Kasting wrote:
> Nit: How about "due_to_user_inactivity" or "from_inactivity_timer" or
something?
>  I mostly want to convert this arg from just the name of a timer into a
> descriptive reason.

Good suggestions.  Both are better than what I had.  I prefer the former, so I
revised this changelist to use it everywhere.

> 
> (Also applies to AutocompleteProvider::Stop())

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
File components/omnibox/autocomplete_provider.h (right):

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
components/omnibox/autocomplete_provider.h:156: // be sent.
On 2015/03/05 23:27:28, Peter Kasting wrote:
> Nit: How about:
> 
> Once Stop() has been called, no more notifications should usually be sent (see
> comments on Stop() below).

Took (roughly) your suggestions.  I like removing the redundancy between
comments (which your revision does).

https://codereview.chromium.org/985503002/diff/1/components/omnibox/autocompl...
components/omnibox/autocomplete_provider.h:176: // cache based on the value of
|clear_cached_results|.
On 2015/03/05 23:27:28, Peter Kasting wrote:
> Nit: How about:
> 
> Advises the provider to stop processing.  This may be called even if the
> provider is already done.  If the provider caches any results, it should clear
> the cache based on the value of |clear_cached_results|.  Normally, once this
is
> called, the provider should not send more notifications to the controller.
> 
> If |user_inactivity_timer| is true, Stop() is being called because it's been a
> long time since the user started the current query, and returning further
> asynchronous results would normally just be disruptive.  Most providers should
> still stop processing in this case, but continuing is legal if there's a good
> reason the user is likely to want even long-delayed asynchronous results, e.g.
> the user has explicitly invoked a keyword extension and the extension is still
> processing the request.

Very nice.  Done.

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
File components/omnibox/keyword_provider.cc (right):

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
components/omnibox/keyword_provider.cc:323: // Only end an extensions request if
the user did something explicitly
On 2015/03/05 23:27:28, Peter Kasting wrote:
> Nit: extension's; explicitly to -> to explicitly

Did both.

https://codereview.chromium.org/985503002/diff/1/components/omnibox/keyword_p...
components/omnibox/keyword_provider.cc:324: // to cancel it; the inactivity
timer does not count.
On 2015/03/05 23:27:28, Peter Kasting wrote:
> Nit: does not count -> shouldn't terminate long-running extension operations,
> since the user likely explicity requested them
> 
> (at least, I think that's a good rationale)

Done (with minor edits).

[Peter Kasting, 2015-03-06 21:41:40]

LGTM

[Mark P, 2015-03-06 22:13:42]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2015-03-06 22:15:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985503002/40001

[commit-bot: I haz the power, 2015-03-07 00:16:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:16:15]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[Mark P, 2015-03-07 05:05:31]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2015-03-07 05:06:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/985503002/40001

[commit-bot: I haz the power, 2015-03-07 05:59:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-07 05:59:53]

Patchset 3 (id:??) landed as
https://crrev.com/8a37c38f75c3f9c7542fc93d349af5fc3617753e
Cr-Commit-Position: refs/heads/master@{#319557}