Experimental AutocompleteProvider for zerosuggest.

Experimental AutocompleteProvider for zerosuggest.

Zerosuggest is an experimental feature which would offer query
suggestions when a user focuses in the omnibox prior to typing any text.
The goal of this change is to allow us to do a limited dogfood of this
feature in canary to assess data quality and UX in situ.

This experimental implementation uses the existing AutocompleteProvider
framework and attempts to make minimal changes to omnibox interaction.
The provider is behind a new --zerosuggest-url-prefix flag which must be
explicitly pointed to a suggestion server for testing. The provider goes
to some pains to mimic the current focus behavior of the omnibox by
inserting a placeholder suggestion for the current URL which is filled
and completed inline.  "Enter" still refreshes the page, and the url can
still be copy+pasted or replaced; however, backspace does not work, and
it's complicated to deal with the case where the user unfocuses the
omnibox and then focuses again.

Original change by jered@chromium.org:
https://chromiumcodereview.appspot.com/10832256/

BUG=


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

[samarth, 2012-08-22 18:15:37]

Hi pkasting, sky,

Continuing review from https://chromiumcodereview.appspot.com/10832256/ since
Jered is out.

Thanks,
Samarth

[sky, 2012-08-22 19:38:08]

Peter should review this. I'm removing myself as a reviewer.

[samarth, 2012-08-27 21:41:55]

Hey Peter,

Have you had a chance to look at this? (It's in the same state right now as
Jered left it last week.)

I realize there's probably still a few edge cases to deal with but it would be
really nice to get this initial version in as soon as we can, so we can start
playing with the feature. Once we know we actually like it, we can work towards
re-factoring the code to make this all cleaner.

Thanks!
Samarth

[Peter Kasting, 2012-08-27 23:13:35]

LGTM

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_controller.cc:205: void
AutocompleteController::StartZeroSuggest(
Perhaps this function and StopZeroSuggest() should DCHECK that a query is not
running in general, as they're expected to be called only in that case?

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_controller.cc:244:
last_result.Swap(&result_);
Nit: Just call result_.Reset() instead.

http://codereview.chromium.org/10877021/diff/1/chrome/browser/ui/omnibox/omni...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10877021/diff/1/chrome/browser/ui/omnibox/omni...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:443: if (popup_->IsOpen()) {
Nit: Revert changes to this function

[samarth, 2012-08-28 23:18:46]

sky: I need your approval for chrome/OWNERS. Note that this is a continuation of
https://chromiumcodereview.appspot.com/10832256/ which you LGTM'd (and there
haven't been any major changes since I took over this patch).

Thanks,
Samarth

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_controller.cc:205: void
AutocompleteController::StartZeroSuggest(
On 2012/08/27 23:13:35, Peter Kasting wrote:
> Perhaps this function and StopZeroSuggest() should DCHECK that a query is not
> running in general, as they're expected to be called only in that case?

Done.

http://codereview.chromium.org/10877021/diff/1/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_controller.cc:244:
last_result.Swap(&result_);
On 2012/08/27 23:13:35, Peter Kasting wrote:
> Nit: Just call result_.Reset() instead.

Done.

http://codereview.chromium.org/10877021/diff/1/chrome/browser/ui/omnibox/omni...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10877021/diff/1/chrome/browser/ui/omnibox/omni...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:443: if (popup_->IsOpen()) {
On 2012/08/27 23:13:35, Peter Kasting wrote:
> Nit: Revert changes to this function

Done.

[samarth, 2012-08-31 22:13:31]

Scott: friendly ping

Thanks,
Samarth

[sky, 2012-08-31 23:54:43]

LGTM

[sreeram, 2012-09-03 22:37:07]

http://codereview.chromium.org/10877021/diff/7001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10877021/diff/7001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:532: << "things may be wrong.";
Add "match.provider->name()" to the debug message when the DCHECK fails, so that
you can tell from the output perhaps what went wrong. Otherwise, you'll have
little idea why this DCHECK failed, unless you are able to recreate the failure
scenario.

[samarth, 2012-09-04 15:48:34]

http://codereview.chromium.org/10877021/diff/7001/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10877021/diff/7001/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:532: << "things may be wrong.";
On 2012/09/03 22:37:07, sreeram wrote:
> Add "match.provider->name()" to the debug message when the DCHECK fails, so
that
> you can tell from the output perhaps what went wrong. Otherwise, you'll have
> little idea why this DCHECK failed, unless you are able to recreate the
failure
> scenario.

Done.

[commit-bot: I haz the power, 2012-09-05 20:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/10877021/10005

[commit-bot: I haz the power, 2012-09-05 23:53:09]

Change committed as 155057