Assume all providers may give zero suggest responses.

Assume all providers may give zero suggest responses.

Passes an argument to Start() function to indicate whether it was
triggered by focus event. Allows any provider to return matches on
focus.

BUG=446239
Committed: https://crrev.com/3ef531d79354dcd495f252127f90b7f55a89a1e8
Cr-Commit-Position: refs/heads/master@{#310901}

[Maria, 2015-01-06 19:47:48]

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

[Maria, 2015-01-06 19:47:49]

Following up on https://codereview.chromium.org/820063002. I removed the call
from autocomplete provider on whether it supplies zero-suggestions. Instead, I
indicate in the Start() call whether it's triggered by a focus and let each
provider decide for themselves whether they want to provide zero suggestions.
This will allow us to write more extensible providers, I think.

[jif-google, 2015-01-07 09:22:25]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2015-01-07 09:22:25]

lgtm

[Peter Kasting, 2015-01-07 20:16:36]

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

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.h:84: // progress.  This is
used to trigger all providers with |on_focus| parameter
This comment refers to a variable or parameter that isn't actually present in
this function call.  Please clarify, as a reader of this won't necessarily know
what "on_focus" is.

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:70: if (minimal_changes ||
on_focus)
This isn't equivalent to the previous behavior, which would have cleared
|matches_| before returning.

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:74: if (on_focus)
Nit: I would just combine all these conditions here.

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

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_provider.h:162: // on focus.
How about this:

  // |called_due_to_focus| is true when Start() is being called in response to
  // the omnibox being focused, instead of due to e.g. user input changes.  Most
  // providers should not provide matches in this case.  Providers which want to
  // display matches on focus can use this flag to know when they can do so.
  virtual void Start(const AutocompleteInput& input,
                     bool minimal_changes,
                     bool called_due_to_focus);

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/keywo...
File components/omnibox/keyword_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/keywo...
components/omnibox/keyword_provider.cc:233: if (on_focus ||
!ExtractKeywordFromInput(input, &keyword, &remaining_input))
Nit: I would probably do this in a separate conditional above the comment above,
just because this isn't really related to that comment.

[Maria, 2015-01-08 07:52:38]

PTAL

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

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.h:84: // progress.  This is
used to trigger all providers with |on_focus| parameter
On 2015/01/07 20:16:36, Peter Kasting wrote:
> This comment refers to a variable or parameter that isn't actually present in
> this function call.  Please clarify, as a reader of this won't necessarily
know
> what "on_focus" is.

Done.

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:70: if (minimal_changes ||
on_focus)
On 2015/01/07 20:16:36, Peter Kasting wrote:
> This isn't equivalent to the previous behavior, which would have cleared
> |matches_| before returning.

Done.

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:74: if (on_focus)
On 2015/01/07 20:16:36, Peter Kasting wrote:
> Nit: I would just combine all these conditions here.

Done.

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

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/autoc...
components/omnibox/autocomplete_provider.h:162: // on focus.
On 2015/01/07 20:16:36, Peter Kasting wrote:
> How about this:
> 
>   // |called_due_to_focus| is true when Start() is being called in response to
>   // the omnibox being focused, instead of due to e.g. user input changes. 
Most
>   // providers should not provide matches in this case.  Providers which want
to
>   // display matches on focus can use this flag to know when they can do so.
>   virtual void Start(const AutocompleteInput& input,
>                      bool minimal_changes,
>                      bool called_due_to_focus);

Done.

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/keywo...
File components/omnibox/keyword_provider.cc (right):

https://codereview.chromium.org/836213002/diff/20001/components/omnibox/keywo...
components/omnibox/keyword_provider.cc:233: if (on_focus ||
!ExtractKeywordFromInput(input, &keyword, &remaining_input))
On 2015/01/07 20:16:36, Peter Kasting wrote:
> Nit: I would probably do this in a separate conditional above the comment
above,
> just because this isn't really related to that comment.

Done.

[Maria, 2015-01-08 19:44:06]

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

[Maria, 2015-01-08 19:44:07]

+oshima for athena/ OWNERS

[oshima, 2015-01-08 20:38:52]

athena lgtm

[Peter Kasting, 2015-01-09 22:37:25]

LGTM

[Maria, 2015-01-09 23:09:33]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2015-01-09 23:11:29]

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

[commit-bot: I haz the power, 2015-01-10 00:04:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-10 00:05:43]

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