Add support for providers called when the omnibox is focused.

Add support for providers called when the omnibox is focused.

The feature described in https://docs.google.com/document/d/1vXHSui-1QYjemBHOpiLiAsIWEIoKuiw66xNcNVfUC3A/edit
requires an omnibox provider that is called when the omnibox is focused.
Right now, only the zero_suggest_provider_ can be called on focus. This CL
makes any provider returning true for |ActivatedOnOmniboxFocus| be called on
focus.

BUG=228111
TBR=miguelg

Committed: https://crrev.com/28e51b058b881eadb0829d8c31330cf85eb9251d
Cr-Commit-Position: refs/heads/master@{#310085}

[jif-google, 2014-12-23 15:53:40]

Patchset #1 (id:1) has been deleted

[jif, 2014-12-30 19:58:00]

Patchset #1 (id:20001) has been deleted

[jif, 2014-12-30 20:04:46]

Patchset #1 (id:40001) has been deleted

[jif, 2014-12-30 20:08:49]

Patchset #1 (id:60001) has been deleted

[jif, 2014-12-30 20:09:03]

jif@chromium.org changed reviewers:
+ pkasting@google.com

[jif, 2014-12-30 20:09:26]

ptal

[Peter Kasting, 2014-12-30 21:18:23]

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

[Peter Kasting, 2014-12-30 21:18:23]

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:226: for (auto& provider
: providers_) {
Just eliminate this loop and the variable it sets and run StartOnOmniboxFocus()
all the time; when there are no run-on-focus providers it shouldn't be harmful.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:358: if
(non_empty_matches)
I think you can eliminate this conditional and just always run UpdateResult(),
but test this in various cases after previously completing omnibox interaction
via hitting enter and/or escape.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.h:86: void
StartOnOmniboxFocus(const AutocompleteInput& input);
Nit: How about this comment and name:

  // Called when the omnibox is focused while no existing user input is in
  // progress.  This is used to trigger any providers which want to provide
  // matches in response to focus.  See
  // AutocompleteProvider::ProvidesMatchesOnOmniboxFocus().
  void OnOmniboxFocused(const AutocompleteInput& input);

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.h (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:40: // The controller will
call StartOnOmniboxFocus when the user focuses in the
Nit: While here, remove "in"

https://codereview.chromium.org/820063002/diff/80001/components/omnibox/autoc...
File components/omnibox/autocomplete_provider.h (right):

https://codereview.chromium.org/820063002/diff/80001/components/omnibox/autoc...
components/omnibox/autocomplete_provider.h:200: // Returns whether the provider
should be used when the omnibox is focused.
Nit: How about this comment and name:

  // Returns whether the provider wants to provide matches in response to the
  // omnibox being focused.
  virtual bool ProvidesMatchesOnOmniboxFocus() const;

[jif-google, 2014-12-31 09:49:06]

Patchset #2 (id:100001) has been deleted

[jif-google, 2014-12-31 10:21:55]

Patchset #2 (id:120001) has been deleted

[jif, 2014-12-31 19:01:48]

Patchset #2 (id:140001) has been deleted

[jif, 2014-12-31 19:02:26]

Thanks. PTAL.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:226: for (auto& provider
: providers_) {
On 2014/12/30 21:18:23, Peter Kasting wrote:
> Just eliminate this loop and the variable it sets and run
StartOnOmniboxFocus()
> all the time; when there are no run-on-focus providers it shouldn't be
harmful.

Done.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:358: if
(non_empty_matches)
On 2014/12/30 21:18:23, Peter Kasting wrote:
> I think you can eliminate this conditional and just always run UpdateResult(),
> but test this in various cases after previously completing omnibox interaction
> via hitting enter and/or escape.

Done.
Looks good.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.h:86: void
StartOnOmniboxFocus(const AutocompleteInput& input);
On 2014/12/30 21:18:23, Peter Kasting wrote:
> Nit: How about this comment and name:
> 
>   // Called when the omnibox is focused while no existing user input is in
>   // progress.  This is used to trigger any providers which want to provide
>   // matches in response to focus.  See
>   // AutocompleteProvider::ProvidesMatchesOnOmniboxFocus().
>   void OnOmniboxFocused(const AutocompleteInput& input);

Done.

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.h (right):

https://codereview.chromium.org/820063002/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:40: // The controller will
call StartOnOmniboxFocus when the user focuses in the
On 2014/12/30 21:18:23, Peter Kasting wrote:
> Nit: While here, remove "in"

Done.

https://codereview.chromium.org/820063002/diff/80001/components/omnibox/autoc...
File components/omnibox/autocomplete_provider.h (right):

https://codereview.chromium.org/820063002/diff/80001/components/omnibox/autoc...
components/omnibox/autocomplete_provider.h:200: // Returns whether the provider
should be used when the omnibox is focused.
On 2014/12/30 21:18:23, Peter Kasting wrote:
> Nit: How about this comment and name:
> 
>   // Returns whether the provider wants to provide matches in response to the
>   // omnibox being focused.
>   virtual bool ProvidesMatchesOnOmniboxFocus() const;

Done.

[jif, 2015-01-05 17:22:32]

jif@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[jif, 2015-01-05 17:23:08]

+mariakhomenko for Clank/Zerosuggest perspective.

[Maria, 2015-01-05 19:09:52]

lgtm

https://codereview.chromium.org/820063002/diff/160001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/820063002/diff/160001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:268: if (i->get() ==
zero_suggest_provider_)
This bit is a little strange now. Here we clear out zero suggest matches to make
sure they don't get mixed with prefix-based matches. 

This is ok for clipboard, which will run with prefix input as well as non-prefix
input and will update its matches_, but I am wondering if we get another
provider that decides to provide matches on focus, but not for prefixed input,
this will be off.

[Peter Kasting, 2015-01-05 19:31:07]

https://codereview.chromium.org/820063002/diff/160001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/820063002/diff/160001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.cc:268: if (i->get() ==
zero_suggest_provider_)
On 2015/01/05 19:09:52, Maria wrote:
> This bit is a little strange now. Here we clear out zero suggest matches to
make
> sure they don't get mixed with prefix-based matches. 
> 
> This is ok for clipboard, which will run with prefix input as well as
non-prefix
> input and will update its matches_, but I am wondering if we get another
> provider that decides to provide matches on focus, but not for prefixed input,
> this will be off.

Yeah, this is weird.  I kind of think the ZeroSuggest provider, and not the
controller, ought to be making this kind of decision.  Perhaps we should simply
ensure Start() passes along enough info for it to do so.

[Peter Kasting, 2015-01-05 19:48:11]

LGTM, I'm not sure whether the issue Maria raises needs to be addressed now or
not.  Let her make the call.

[Maria, 2015-01-05 20:00:24]

On 2015/01/05 19:48:11, Peter Kasting wrote:
> LGTM, I'm not sure whether the issue Maria raises needs to be addressed now or
> not.  Let her make the call.

I think it's ok to fix the issue separately. We would need additional
information in AutocompleteInput() to make this fix. I filed
https://code.google.com/p/chromium/issues/detail?id=446239 to track it.

[jif, 2015-01-06 10:02:40]

On 2015/01/05 20:00:24, Maria wrote:
> On 2015/01/05 19:48:11, Peter Kasting wrote:
> > LGTM, I'm not sure whether the issue Maria raises needs to be addressed now
or
> > not.  Let her make the call.
> 
> I think it's ok to fix the issue separately. We would need additional
> information in AutocompleteInput() to make this fix. I filed
> https://code.google.com/p/chromium/issues/detail?id=446239 to track it.

OK, thanks.

[jif, 2015-01-06 10:02:47]

The CQ bit was checked by jif@chromium.org

[commit-bot: I haz the power, 2015-01-06 10:03:32]

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

[commit-bot: I haz the power, 2015-01-06 10:08:57]

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

[commit-bot: I haz the power, 2015-01-06 10:08:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jif-google, 2015-01-06 12:52:14]

Patchset #3 (id:180001) has been deleted

[jif, 2015-01-06 12:54:04]

jif@chromium.org changed reviewers:
+ miguelg@chromium.org

[jif, 2015-01-06 12:57:08]

+miguelg OWNER of
    ../omnibox/AutocompleteController.java
    chrome/browser/android/omnibox/autocomplete_controller_android.cc
    chrome/browser/android/omnibox/autocomplete_controller_android.h

[jif-google, 2015-01-06 13:17:38]

The CQ bit was checked by jif@google.com

[commit-bot: I haz the power, 2015-01-06 13:18:40]

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

[commit-bot: I haz the power, 2015-01-06 14:12:51]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2015-01-06 14:13:27]

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