First pass at experimental omnibox API. There are plenty of rough edges and

First pass at experimental omnibox API. There are plenty of rough edges and features to work on, but it's in a usable state.

When an extension is installed that specifies an omnibox keyword in its manifest, we add that keyword to the user's list of Search Engines. The user can then edit this keyword later.

I'm leveraging most of the original search engine keyword code. An extension keyword has a special URL that identifies it as an extension keyword. There is some special case code to treat these keywords slightly differently throughout
the omnibox code.

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

[Matt Perry, 2010-05-19 23:29:45]

Peter: please review the omnibox changes
Aaron: extension code

[Peter Kasting, 2010-05-20 00:19:31]

Despite the number of comments below, much of what's here looks OK to me.  My
biggest concern in this patch is the bits in the keyword provider that I don't
think properly update state from query to query.  The other big issue is the UI
for managing these keywords which can be punted into its own patch (although I
would at least filter these from the existing UI in the current patch).

http://codereview.chromium.org/2078021/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2078021/diff/1/2#newcode1044
chrome/app/generated_resources.grd:1044: Run <ph name="SITE_NAME">$1<ex>Google
Talk</ex></ph> command <ph
name="SEARCH_TERMS">$2<ex>mpcomplete@chromium.org</ex></ph>
Nit: Should SITE_NAME be EXTENSION_NAME?  (3 places)

http://codereview.chromium.org/2078021/diff/1/2#newcode1053
chrome/app/generated_resources.grd:1053: <message
name="IDS_AUTOCOMPLETE_EXTENSION_KEYWORD_DESCRIPTION" desc="Description for an
extension keyword match.">
Nit: Do you think this is a big advantage over just using the standard keyword
description?  Seems like just saying "Keyword" would be fine.

http://codereview.chromium.org/2078021/diff/1/2#newcode3777
chrome/app/generated_resources.grd:3777: Press <ph
name="SEARCH_KEY">$1<ex>Tab</ex></ph> to send commands to <ph
name="SITE_NAME">$2<ex>Google Talk Extension</ex></ph>
Nit: I wouldn't put "Extension" in your example command target.  (2 places)

http://codereview.chromium.org/2078021/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/2078021/diff/1/3#newcode273
chrome/browser/autocomplete/autocomplete_edit.cc:273: if (match.template_url &&
match.template_url->IsExtensionKeyword()) {
Nit: Should this block go above the |destination_url| check so extension URLs
don't have to be valid?

http://codereview.chromium.org/2078021/diff/1/4
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/1/4#newcode37
chrome/browser/autocomplete/keyword_provider.cc:37: model_(NULL),
next_extension_suggest_id_(0) {
Nit: One member per line (2 places)

http://codereview.chromium.org/2078021/diff/1/4#newcode90
chrome/browser/autocomplete/keyword_provider.cc:90: matches_.clear();
You need some more stuff here.

You're not resetting |done_| correctly when getting new queries after other
queries that requested things from the extension system.

I also think it would be useful for the extensions to get a "cancel" message. 
This might enable them to save significant amounts of processing or network
traffic in some scenarios.

Given the above, it makes sense to me to check if |!minimal_changes|, and cancel
the previous request, increment your suggest id, and set |done_| to true.

http://codereview.chromium.org/2078021/diff/1/4#newcode144
chrome/browser/autocomplete/keyword_provider.cc:144: // recent suggest results.
Nit: I would go ahead and do precisely that.

http://codereview.chromium.org/2078021/diff/1/4#newcode149
chrome/browser/autocomplete/keyword_provider.cc:149: bool have_listeners =
ExtensionOmniboxEventRouter::OnInputChanged(
Nit: Just avoid the temp.

if (OnInputChanged(...))
  done_ = false;

http://codereview.chromium.org/2078021/diff/1/4#newcode332
chrome/browser/autocomplete/keyword_provider.cc:332: if (type ==
NotificationType::EXTENSION_OMNIBOX_SUGGESTIONS_READY) {
Nit: Just DCHECK (or CHECK, if you must), to avoid indenting.

http://codereview.chromium.org/2078021/diff/1/4#newcode368
chrome/browser/autocomplete/keyword_provider.cc:368: done_ = true;
You can't early-return and skip these, or the provider will never be marked as
finishing.

http://codereview.chromium.org/2078021/diff/1/5
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/2078021/diff/1/5#newcode1
chrome/browser/autocomplete/keyword_provider.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/5#newcode109
chrome/browser/autocomplete/keyword_provider.h:109: // If |relevance| is
negative, calculate a relevance based on heuristics.
I don't think you need this odd negative relevance thing.  It looks to me like
the only reason you want to calculate your own relevance is to pass "true" to
the |complete| parameter.  This seems wrong.  If you're passing true for an
incomplete extension keyword, you're going to cause issues with the other result
ranking.

If you find you don't want the existing keyword scoring to apply to extension
keywords, then we should either improve the keywords scoring function in
general, or (more hacky) tell CalculateRelevance() which type of keyword it's
looking at.

http://codereview.chromium.org/2078021/diff/1/7
File chrome/browser/cocoa/location_bar_view_mac.mm (right):

http://codereview.chromium.org/2078021/diff/1/7#newcode272
chrome/browser/cocoa/location_bar_view_mac.mm:272: int message_id_hint =
is_extension_keyword ?
Nit: I'd roll |message_id_hint| into its lone use below.

http://codereview.chromium.org/2078021/diff/1/8
File chrome/browser/cocoa/location_bar_view_mac_unittest.mm (right):

http://codereview.chromium.org/2078021/diff/1/8#newcode1
chrome/browser/cocoa/location_bar_view_mac_unittest.mm:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/16
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/2078021/diff/1/16#newcode882
chrome/browser/gtk/location_bar_view_gtk.cc:882: int message_id =
is_extension_keyword ?
Nit: I'd just roll this into its lone usage below.

http://codereview.chromium.org/2078021/diff/1/17
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/2078021/diff/1/17#newcode1
chrome/browser/search_engines/template_url.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/18
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/2078021/diff/1/18#newcode1
chrome/browser/search_engines/template_url.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/19
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/2078021/diff/1/19#newcode1
chrome/browser/search_engines/template_url_model.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/19#newcode1087
chrome/browser/search_engines/template_url_model.cc:1087:
UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0);
I don't think these should get listed in the standard search engine list.

They aren't search engines, and the URL field (and probably Name as well) isn't
something that should really be viewed and edited.

Instead, it seems like maybe you should be able to change an extension's keyword
in the extension's options?  Or else there should be some list of extension
keywords separately from the search engines list...

http://codereview.chromium.org/2078021/diff/1/20
File chrome/browser/search_engines/template_url_model.h (right):

http://codereview.chromium.org/2078021/diff/1/20#newcode1
chrome/browser/search_engines/template_url_model.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

http://codereview.chromium.org/2078021/diff/1/21
File chrome/browser/views/location_bar/keyword_hint_view.cc (right):

http://codereview.chromium.org/2078021/diff/1/21#newcode62
chrome/browser/views/location_bar/keyword_hint_view.cc:62: int message_id =
is_extension_keyword ?
Nit: I'd just roll this into its lone usage below.

[Aaron Boodman, 2010-05-21 18:53:26]

Neat.

Some high level comments:

- There are no tests.
- You should add a sample (could be a separate cl).

I don't like the way the onInputChanged event gets passed a callback that you
call multiple times to suggest. This breaks with our own conventions and with
javascripty conventions.

If you really want to associate suggestions with input change events, I think it
would be more understandable to just pass the suggest_id parameter through to
the extension developer and let him pass it back to suggest().

But looking at this code, I don't see why we can't just eliminate this
association all together. It seems like the KeywordAutocompleteProvider could
just assume that all suggestions it gets are for the latest suggestion.

http://codereview.chromium.org/2078021/diff/1/5
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/2078021/diff/1/5#newcode118
chrome/browser/autocomplete/keyword_provider.h:118: void
AddAutocompleteMatchForExtension(
Need comment for this method.

http://codereview.chromium.org/2078021/diff/1/5#newcode131
chrome/browser/autocomplete/keyword_provider.h:131: int extension_suggest_id_;
Need comments for these members.

http://codereview.chromium.org/2078021/diff/1/9
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/2078021/diff/1/9#newcode230
chrome/browser/extensions/extension_function_dispatcher.cc:230:
RegisterFunction<RemoveAllContextMenusFunction>();
RemoveAllContextMenusFunction should go with the context menus group. Add an //
Omnibox section.

http://codereview.chromium.org/2078021/diff/1/12
File chrome/browser/extensions/extension_omnibox_api.cc (right):

http://codereview.chromium.org/2078021/diff/1/12#newcode19
chrome/browser/extensions/extension_omnibox_api.cc:19:
"experimental.omnibox.onInputChanged/" + extension_id;  // @@@MP
I don't see us use @@@<username> anywhere else. TODO(mpcomplete) ?

http://codereview.chromium.org/2078021/diff/1/13
File chrome/browser/extensions/extension_omnibox_api.h (right):

http://codereview.chromium.org/2078021/diff/1/13#newcode29
chrome/browser/extensions/extension_omnibox_api.h:29: // Base class for
clipboard function APIs.
Comment out of date?

http://codereview.chromium.org/2078021/diff/1/14
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/2078021/diff/1/14#newcode357
chrome/browser/extensions/extensions_service.cc:357:
profile_->GetTemplateURLModel()->UnregisterExtensionKeyword(extension);
It seems like UnloadExtension should do this.

http://codereview.chromium.org/2078021/diff/1/14#newcode925
chrome/browser/extensions/extensions_service.cc:925: if (success)
We are a bit schizophrenic, but we tend to hook up to other parts of the system
more often using notifications (those parts of the system listen to the
EXTENSION_LOADED and EXTENSION_UNLOADED notifications). Did you consider doing
that instead of directly calling the template url model?

[Peter Kasting, 2010-05-21 20:51:56]

On 2010/05/21 18:53:26, Aaron Boodman wrote:
> If you really want to associate suggestions with input change events, I think
it
> would be more understandable to just pass the suggest_id parameter through to
> the extension developer and let him pass it back to suggest().
> 
> But looking at this code, I don't see why we can't just eliminate this
> association all together. It seems like the KeywordAutocompleteProvider could
> just assume that all suggestions it gets are for the latest suggestion.

I don't think that's a good idea by itself.  If you look at my review comments,
I suggested that the API here should basically be that the omnibox notifies
extensions when a request is cancelled.  Then we enforce (perhaps in our
internal plumbing) that we drop results for cancelled request before they get
back to the controller.

This has a variety of benefits at different levels, and it avoids passing around
unique IDs (which seems kind of fragile and hacky to me), since at most one
request is in flight at once.

[Aaron Boodman, 2010-05-21 23:02:40]

On 2010/05/21 20:51:56, Peter Kasting wrote:
> I don't think that's a good idea by itself.  If you look at my review
comments,
> I suggested that the API here should basically be that the omnibox notifies
> extensions when a request is cancelled.  Then we enforce (perhaps in our
> internal plumbing) that we drop results for cancelled request before they get
> back to the controller.
> 
> This has a variety of benefits at different levels, and it avoids passing
around
> unique IDs (which seems kind of fragile and hacky to me), since at most one
> request is in flight at once.

I think we might be saying the same thing.

This is what I want:

chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput) {
  chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
});

Ideally, there is no need for the developer to use anything from onInputChanged
when it comes time to call suggest(). Because we can assume that the suggestion
should be for the current session. If there is no current session, it is an
error to call suggest().

Is this what you're saying? Sorry if I'm being dense.

[Peter Kasting, 2010-05-21 23:08:25]

On Fri, May 21, 2010 at 4:02 PM, <aa@chromium.org> wrote:

> I think we might be saying the same thing.
>
> This is what I want:
>
> chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput)
> {
>  chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
> });
>
> Ideally, there is no need for the developer to use anything from
> onInputChanged
> when it comes time to call suggest(). Because we can assume that the
> suggestion
> should be for the current session. If there is no current session, it is an
> error to call suggest().
>
> Is this what you're saying? Sorry if I'm being dense.


I don't know.  I don't really have a handle on the JS-side model, so I don't
know precisely what "current session" means, what the extension can get at,
etc.

All I am saying is that from the C++ side, we should try to just have one
query in flight at a time, and we shouldn't have results appearing from
out-of-date queries (this second bit is what I was worried about).  I assume
that it would be nice, from the extension side, to be able to know (if you
want) as soon as the query you're working on becomes out-of-date, so you can
stop work, since we're just going to throw your results away anyway.

PK

[Matt Perry, 2010-05-25 21:07:13]

Responses inline.

As for the onInputChanged event, let me try to convince you that my current way
is best :).

Aaron proposed:
chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput) {
  chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
});

With suggest() being invalid if you call it outside the event listener. This
prevents the extension from doing asynchronous processing, like network
requests, before calling suggest. IMO, that makes it a non-starter.

Aaron also proposed an explicit API with a suggest_id. This would work, but it
feels more cumbersome to me with no real benefit.

Peter suggested an explicit cancel event, so we know only 1 query is out at a
time. First, onInputChanged implicitly says "cancel the previous query", so
another event seems redundant to me. Second, results could already be on their
way back to the KeywordProvider by the time we cancel the query. The only way I
see to prevent us from using these stale results is to tag each suggest with an
ID and ignore stale IDs (which is what I did).

http://codereview.chromium.org/2078021/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2078021/diff/1/2#newcode1044
chrome/app/generated_resources.grd:1044: Run <ph name="SITE_NAME">$1<ex>Google
Talk</ex></ph> command <ph
name="SEARCH_TERMS">$2<ex>mpcomplete@chromium.org</ex></ph>
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: Should SITE_NAME be EXTENSION_NAME?  (3 places)

Done.

http://codereview.chromium.org/2078021/diff/1/2#newcode1053
chrome/app/generated_resources.grd:1053: <message
name="IDS_AUTOCOMPLETE_EXTENSION_KEYWORD_DESCRIPTION" desc="Description for an
extension keyword match.">
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: Do you think this is a big advantage over just using the standard keyword
> description?  Seems like just saying "Keyword" would be fine.

Sounds good. I'm not a fan of any of the strings I used, really, and I expect
they'll change before it's final.

http://codereview.chromium.org/2078021/diff/1/2#newcode3777
chrome/app/generated_resources.grd:3777: Press <ph
name="SEARCH_KEY">$1<ex>Tab</ex></ph> to send commands to <ph
name="SITE_NAME">$2<ex>Google Talk Extension</ex></ph>
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: I wouldn't put "Extension" in your example command target.  (2 places)

Done.

http://codereview.chromium.org/2078021/diff/1/3
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/2078021/diff/1/3#newcode273
chrome/browser/autocomplete/autocomplete_edit.cc:273: if (match.template_url &&
match.template_url->IsExtensionKeyword()) {
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: Should this block go above the |destination_url| check so extension URLs
> don't have to be valid?

Done.

http://codereview.chromium.org/2078021/diff/1/4
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/1/4#newcode37
chrome/browser/autocomplete/keyword_provider.cc:37: model_(NULL),
next_extension_suggest_id_(0) {
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: One member per line (2 places)

Done.

http://codereview.chromium.org/2078021/diff/1/4#newcode90
chrome/browser/autocomplete/keyword_provider.cc:90: matches_.clear();
On 2010/05/20 00:19:31, Peter Kasting wrote:
> You need some more stuff here.
> 
> You're not resetting |done_| correctly when getting new queries after other
> queries that requested things from the extension system.
> 
> I also think it would be useful for the extensions to get a "cancel" message. 
> This might enable them to save significant amounts of processing or network
> traffic in some scenarios.
> 
> Given the above, it makes sense to me to check if |!minimal_changes|, and
cancel
> the previous request, increment your suggest id, and set |done_| to true.

Done.

I don't see a need for a cancel message though. Cancel (of the previous query)
is implicit in each onInputChanged event.

http://codereview.chromium.org/2078021/diff/1/4#newcode144
chrome/browser/autocomplete/keyword_provider.cc:144: // recent suggest results.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: I would go ahead and do precisely that.

Done.

http://codereview.chromium.org/2078021/diff/1/4#newcode332
chrome/browser/autocomplete/keyword_provider.cc:332: if (type ==
NotificationType::EXTENSION_OMNIBOX_SUGGESTIONS_READY) {
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: Just DCHECK (or CHECK, if you must), to avoid indenting.

Done.

http://codereview.chromium.org/2078021/diff/1/4#newcode368
chrome/browser/autocomplete/keyword_provider.cc:368: done_ = true;
On 2010/05/20 00:19:31, Peter Kasting wrote:
> You can't early-return and skip these, or the provider will never be marked as
> finishing.

Done.

http://codereview.chromium.org/2078021/diff/1/5
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/2078021/diff/1/5#newcode1
chrome/browser/autocomplete/keyword_provider.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2078021/diff/1/5#newcode109
chrome/browser/autocomplete/keyword_provider.h:109: // If |relevance| is
negative, calculate a relevance based on heuristics.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> I don't think you need this odd negative relevance thing.  It looks to me like
> the only reason you want to calculate your own relevance is to pass "true" to
> the |complete| parameter.  This seems wrong.  If you're passing true for an
> incomplete extension keyword, you're going to cause issues with the other
result
> ranking.
> 
> If you find you don't want the existing keyword scoring to apply to extension
> keywords, then we should either improve the keywords scoring function in
> general, or (more hacky) tell CalculateRelevance() which type of keyword it's
> looking at.

Actually, the reason I pass in relevance is so that the async suggest results
would be ordered correctly:
 1. what you typed (the first sync result)
 2. async result 1
 3. async result 2...

Though now I see I made a mistake and always used (CalcRelevance - 1) instead of
(CalcRelevance - i).

As for |complete| = true, this will always be the case for the async results,
since we only hear from the extension when the full keyword is typed.

I'll add some comments to explain this.

http://codereview.chromium.org/2078021/diff/1/5#newcode118
chrome/browser/autocomplete/keyword_provider.h:118: void
AddAutocompleteMatchForExtension(
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> Need comment for this method.

Unused remnant. Deleted.

http://codereview.chromium.org/2078021/diff/1/5#newcode131
chrome/browser/autocomplete/keyword_provider.h:131: int extension_suggest_id_;
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> Need comments for these members.

Done.

http://codereview.chromium.org/2078021/diff/1/8
File chrome/browser/cocoa/location_bar_view_mac_unittest.mm (right):

http://codereview.chromium.org/2078021/diff/1/8#newcode1
chrome/browser/cocoa/location_bar_view_mac_unittest.mm:1: // Copyright (c) 2009
The Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2078021/diff/1/9
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/2078021/diff/1/9#newcode230
chrome/browser/extensions/extension_function_dispatcher.cc:230:
RegisterFunction<RemoveAllContextMenusFunction>();
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> RemoveAllContextMenusFunction should go with the context menus group. Add an
//
> Omnibox section.

Done.

http://codereview.chromium.org/2078021/diff/1/12
File chrome/browser/extensions/extension_omnibox_api.cc (right):

http://codereview.chromium.org/2078021/diff/1/12#newcode19
chrome/browser/extensions/extension_omnibox_api.cc:19:
"experimental.omnibox.onInputChanged/" + extension_id;  // @@@MP
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> I don't see us use @@@<username> anywhere else. TODO(mpcomplete) ?

Oops. I use that for things I should fix before sending for review. Looks like
it didn't help this time :).

Fixed.

http://codereview.chromium.org/2078021/diff/1/13
File chrome/browser/extensions/extension_omnibox_api.h (right):

http://codereview.chromium.org/2078021/diff/1/13#newcode29
chrome/browser/extensions/extension_omnibox_api.h:29: // Base class for
clipboard function APIs.
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> Comment out of date?

Done.

http://codereview.chromium.org/2078021/diff/1/14
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/2078021/diff/1/14#newcode357
chrome/browser/extensions/extensions_service.cc:357:
profile_->GetTemplateURLModel()->UnregisterExtensionKeyword(extension);
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> It seems like UnloadExtension should do this.

I don't want this called on reload or disable. This is to clean up on uninstall
only. I have a TODO to disable keywords while an extension is unloaded.

The reason is so that a user can change the keyword, and we remember it until
they uninstall.

http://codereview.chromium.org/2078021/diff/1/14#newcode925
chrome/browser/extensions/extensions_service.cc:925: if (success)
On 2010/05/21 18:53:26, Aaron Boodman wrote:
> We are a bit schizophrenic, but we tend to hook up to other parts of the
system
> more often using notifications (those parts of the system listen to the
> EXTENSION_LOADED and EXTENSION_UNLOADED notifications). Did you consider doing
> that instead of directly calling the template url model?

Yeah. I flipped a coin and went with this way for no real reason. It's a little
more explicit I guess. This also doesn't require adding a new uninstall
notification.

http://codereview.chromium.org/2078021/diff/1/17
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/2078021/diff/1/17#newcode1
chrome/browser/search_engines/template_url.cc:1: // Copyright (c) 2006-2008 The
Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2078021/diff/1/18
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/2078021/diff/1/18#newcode1
chrome/browser/search_engines/template_url.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2078021/diff/1/19
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/2078021/diff/1/19#newcode1
chrome/browser/search_engines/template_url_model.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

http://codereview.chromium.org/2078021/diff/1/19#newcode1087
chrome/browser/search_engines/template_url_model.cc:1087:
UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0);
On 2010/05/20 00:19:31, Peter Kasting wrote:
> I don't think these should get listed in the standard search engine list.
> 
> They aren't search engines, and the URL field (and probably Name as well)
isn't
> something that should really be viewed and edited.
> 
> Instead, it seems like maybe you should be able to change an extension's
keyword
> in the extension's options?  Or else there should be some list of extension
> keywords separately from the search engines list...

Yeah, maybe. I had planned on allowing the user to change the keyword but making
the URL uneditable. I'm not sure what the best UI is.

http://codereview.chromium.org/2078021/diff/1/20
File chrome/browser/search_engines/template_url_model.h (right):

http://codereview.chromium.org/2078021/diff/1/20#newcode1
chrome/browser/search_engines/template_url_model.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/05/20 00:19:31, Peter Kasting wrote:
> Nit: 2010

Done.

[Matt Perry, 2010-05-25 21:10:07]

Oh, and I'll work on adding some tests.

[Aaron Boodman, 2010-05-25 22:06:50]

On Tue, May 25, 2010 at 2:07 PM,  <mpcomplete@chromium.org> wrote:
> Responses inline.
>
> As for the onInputChanged event, let me try to convince you that my current
> way
> is best :).
>
> Aaron proposed:
> chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput)
> {
>  chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
> });
>
> With suggest() being invalid if you call it outside the event listener. This
> prevents the extension from doing asynchronous processing, like network
> requests, before calling suggest. IMO, that makes it a non-starter.

No, not invalid if called outside the event listener. Invalid if
called when no keyword-inputting session is in progress. You can do
asynchronous processing. Any calls to suggest() are assumed to go with
the input session which is in progress.

> Aaron also proposed an explicit API with a suggest_id. This would work, but
> it
> feels more cumbersome to me with no real benefit.
>
> Peter suggested an explicit cancel event, so we know only 1 query is out at
> a
> time. First, onInputChanged implicitly says "cancel the previous query", so
> another event seems redundant to me. Second, results could already be on
> their
> way back to the KeywordProvider by the time we cancel the query. The only
> way I
> see to prevent us from using these stale results is to tag each suggest with
> an
> ID and ignore stale IDs (which is what I did).

Cancel would help in the case where the user completely abandons the
query, right (rather than adding more input)?

> http://codereview.chromium.org/2078021/diff/1/14#newcode357
> chrome/browser/extensions/extensions_service.cc:357:
> profile_->GetTemplateURLModel()->UnregisterExtensionKeyword(extension);
> On 2010/05/21 18:53:26, Aaron Boodman wrote:
>>
>> It seems like UnloadExtension should do this.
>
> I don't want this called on reload or disable. This is to clean up on
> uninstall only. I have a TODO to disable keywords while an extension is
> unloaded.
>
> The reason is so that a user can change the keyword, and we remember it
> until they uninstall.

Does this all work properly with extensions that are loaded unpacked?
If so, I am happy. Usually when we do things in response to
install/uninstall, we eventually end up backing them out and doing
them in load/unload.

- a

[Matt Perry, 2010-05-25 22:14:50]

On 2010/05/25 22:06:50, Aaron Boodman wrote:
> On Tue, May 25, 2010 at 2:07 PM,  <mailto:mpcomplete@chromium.org> wrote:
> > Responses inline.
> >
> > As for the onInputChanged event, let me try to convince you that my current
> > way
> > is best :).
> >
> > Aaron proposed:
> >
chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput)
> > {
> > &nbsp;chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
> > });
> >
> > With suggest() being invalid if you call it outside the event listener. This
> > prevents the extension from doing asynchronous processing, like network
> > requests, before calling suggest. IMO, that makes it a non-starter.
> 
> No, not invalid if called outside the event listener. Invalid if
> called when no keyword-inputting session is in progress. You can do
> asynchronous processing. Any calls to suggest() are assumed to go with
> the input session which is in progress.

I see. But this would mean that results could be out of date by the time they
arrive. If I type "<keyword> google" and I get a result for "<keyword>
goodyear", I'm going to be confused. The rest of the omnibox system discards
stale results as the user types.

> Cancel would help in the case where the user completely abandons the
> query, right (rather than adding more input)?

Right. I haven't implemented that Cancel yet.

> > http://codereview.chromium.org/2078021/diff/1/14#newcode357
> > chrome/browser/extensions/extensions_service.cc:357:
> > profile_->GetTemplateURLModel()->UnregisterExtensionKeyword(extension);
> > On 2010/05/21 18:53:26, Aaron Boodman wrote:
> >>
> >> It seems like UnloadExtension should do this.
> >
> > I don't want this called on reload or disable. This is to clean up on
> > uninstall only. I have a TODO to disable keywords while an extension is
> > unloaded.
> >
> > The reason is so that a user can change the keyword, and we remember it
> > until they uninstall.
> 
> Does this all work properly with extensions that are loaded unpacked?

Yes.

[Aaron Boodman, 2010-05-25 22:32:26]

On Tue, May 25, 2010 at 3:14 PM,  <mpcomplete@chromium.org> wrote:
> On 2010/05/25 22:06:50, Aaron Boodman wrote:
>>
>> On Tue, May 25, 2010 at 2:07 PM,  <mailto:mpcomplete@chromium.org> wrote:
>> > Responses inline.
>> >
>> > As for the onInputChanged event, let me try to convince you that my
>> > current
>> > way
>> > is best :).
>> >
>> > Aaron proposed:
>> >
>
> chrome.experimental.omnibox.onInputChanged.addListener(function(currentInput)
>>
>> > {
>> > &nbsp;chrome.experimental.omnibox.suggest(["foo", "bar", "baz"]);
>> > });
>> >
>> > With suggest() being invalid if you call it outside the event listener.
>> > This
>> > prevents the extension from doing asynchronous processing, like network
>> > requests, before calling suggest. IMO, that makes it a non-starter.
>
>> No, not invalid if called outside the event listener. Invalid if
>> called when no keyword-inputting session is in progress. You can do
>> asynchronous processing. Any calls to suggest() are assumed to go with
>> the input session which is in progress.
>
> I see. But this would mean that results could be out of date by the time
> they
> arrive. If I type "<keyword> google" and I get a result for "<keyword>
> goodyear", I'm going to be confused. The rest of the omnibox system discards
> stale results as the user types.

You've convinced me.

- a

[Peter Kasting, 2010-05-26 01:10:41]

http://codereview.chromium.org/2078021/diff/1/19
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/2078021/diff/1/19#newcode1087
chrome/browser/search_engines/template_url_model.cc:1087:
UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0);
On 2010/05/25 21:07:13, Matt Perry wrote:
> On 2010/05/20 00:19:31, Peter Kasting wrote:
> > I don't think these should get listed in the standard search engine list.
> 
> Yeah, maybe. I had planned on allowing the user to change the keyword but
making
> the URL uneditable. I'm not sure what the best UI is.

I don't think that's a good idea.  These aren't "search engines", they're
extension keywords, so they shouldn't be listed in a dialog about "search
engines".  How about for now you make it so they're not listed, and we can try
to come up with a UI for editing in the next patch?

http://codereview.chromium.org/2078021/diff/23001/24001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2078021/diff/23001/24001#newcode1053
chrome/app/generated_resources.grd:1053: <message
name="IDS_AUTOCOMPLETE_EXTENSION_KEYWORD_DESCRIPTION" desc="Description for an
extension keyword match.">
Nit: Oh, I was thinking that you'd just ditch the separate message entirely for
now.

http://codereview.chromium.org/2078021/diff/23001/24003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/23001/24003#newcode94
chrome/browser/autocomplete/keyword_provider.cc:94: matches_.clear();
Nit: You can omit matches_.clear(); now.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode142
chrome/browser/autocomplete/keyword_provider.cc:142: const TemplateURL*
template_url(model->GetTemplateURLForKeyword(keyword));
Nit: Push this below the next statement

http://codereview.chromium.org/2078021/diff/23001/24003#newcode147
chrome/browser/autocomplete/keyword_provider.cc:147: // TODO(mpcomplete): we
probably don't need to dispatch the event if
This comment seems out of date.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode152
chrome/browser/autocomplete/keyword_provider.cc:152:
matches_.insert(matches_.begin(),
This insert() seems suspicious.  Won't both |matches| and |old_matches| have an
autocomplete match in them, resulting in a duplicate entry?  Did you test this?

http://codereview.chromium.org/2078021/diff/23001/24003#newcode155
chrome/browser/autocomplete/keyword_provider.cc:155: extension_suggest_id_ =
++next_extension_suggest_id_;
I think it's wrong that we only increment the suggest ID when we send a request
to an extension.  I think it would be clearer and safer to update it every time
Start() is called and |minimal_changes| is false.  Then you could also avoid
storing the entire |input|, and instead simply store the |remaining_input| on a
member, because then we'd only do anything in Observe() when nothing has
changed.  That will save a block of code in Observe().  It will also clearly
guarantee that we'll always set |done_| to true.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode162
chrome/browser/autocomplete/keyword_provider.cc:162: // If no one is listening
to this event, then we don't have to wait
Nit: For clarity (since the comment and code are opposite ways of saying the
same thing), how about "We only have to wait for suggest results if there are
actually extensions listening for changes" or something to that effect.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode344
chrome/browser/autocomplete/keyword_provider.cc:344: int suggest_id =
Details<std::pair<int, ListValue*> >(details).ptr()->first;
Nit: Seems like this std::pair<> should be typedefed somewhere.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode379
chrome/browser/autocomplete/keyword_provider.cc:379: if (!description.empty()) {
Nit: Should we just overwrite the match description?

http://codereview.chromium.org/2078021/diff/23001/24003#newcode384
chrome/browser/autocomplete/keyword_provider.cc:384: match->contents.append(L" "
+ UTF16ToWide(description));
If you end up keeping this route, you need a localized string (like "$1 $2")
instead of just raw string concatenation.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode389
chrome/browser/autocomplete/keyword_provider.cc:389:
listener_->OnProviderUpdate(true);
Nit: You should only pass "true" if you actually changed the matches.

[Matt Perry, 2010-05-26 01:30:12]

Tests added. Will get to Peter's comments next.

[Matt Perry, 2010-05-26 22:04:23]

http://codereview.chromium.org/2078021/diff/1/19
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/2078021/diff/1/19#newcode1087
chrome/browser/search_engines/template_url_model.cc:1087:
UTF8ToWide(extension->id()) + L"/?q={searchTerms}", 0, 0);
On 2010/05/26 01:10:42, Peter Kasting wrote:
> On 2010/05/25 21:07:13, Matt Perry wrote:
> > On 2010/05/20 00:19:31, Peter Kasting wrote:
> > > I don't think these should get listed in the standard search engine list.
> > 
> > Yeah, maybe. I had planned on allowing the user to change the keyword but
> making
> > the URL uneditable. I'm not sure what the best UI is.
> 
> I don't think that's a good idea.  These aren't "search engines", they're
> extension keywords, so they shouldn't be listed in a dialog about "search
> engines".  How about for now you make it so they're not listed, and we can try
> to come up with a UI for editing in the next patch?

Done.

http://codereview.chromium.org/2078021/diff/23001/24003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/23001/24003#newcode94
chrome/browser/autocomplete/keyword_provider.cc:94: matches_.clear();
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: You can omit matches_.clear(); now.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode142
chrome/browser/autocomplete/keyword_provider.cc:142: const TemplateURL*
template_url(model->GetTemplateURLForKeyword(keyword));
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: Push this below the next statement

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode147
chrome/browser/autocomplete/keyword_provider.cc:147: // TODO(mpcomplete): we
probably don't need to dispatch the event if
On 2010/05/26 01:10:42, Peter Kasting wrote:
> This comment seems out of date.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode152
chrome/browser/autocomplete/keyword_provider.cc:152:
matches_.insert(matches_.begin(),
On 2010/05/26 01:10:42, Peter Kasting wrote:
> This insert() seems suspicious.  Won't both |matches| and |old_matches| have
an
> autocomplete match in them, resulting in a duplicate entry?  Did you test
this?

I'm not really sure how it's supposed to work, but when I tested it, matches_
always contained only the last asynchronous results from the extension.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode155
chrome/browser/autocomplete/keyword_provider.cc:155: extension_suggest_id_ =
++next_extension_suggest_id_;
On 2010/05/26 01:10:42, Peter Kasting wrote:
> I think it's wrong that we only increment the suggest ID when we send a
request
> to an extension.  I think it would be clearer and safer to update it every
time
> Start() is called and |minimal_changes| is false.  Then you could also avoid
> storing the entire |input|, and instead simply store the |remaining_input| on
a
> member, because then we'd only do anything in Observe() when nothing has
> changed.  That will save a block of code in Observe().  It will also clearly
> guarantee that we'll always set |done_| to true.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode162
chrome/browser/autocomplete/keyword_provider.cc:162: // If no one is listening
to this event, then we don't have to wait
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: For clarity (since the comment and code are opposite ways of saying the
> same thing), how about "We only have to wait for suggest results if there are
> actually extensions listening for changes" or something to that effect.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode344
chrome/browser/autocomplete/keyword_provider.cc:344: int suggest_id =
Details<std::pair<int, ListValue*> >(details).ptr()->first;
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: Seems like this std::pair<> should be typedefed somewhere.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode379
chrome/browser/autocomplete/keyword_provider.cc:379: if (!description.empty()) {
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: Should we just overwrite the match description?

You mean in place of "(Keyword: foobar)" ? Maybe. I'll go with this for now and
see how it feels.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode384
chrome/browser/autocomplete/keyword_provider.cc:384: match->contents.append(L" "
+ UTF16ToWide(description));
On 2010/05/26 01:10:42, Peter Kasting wrote:
> If you end up keeping this route, you need a localized string (like "$1 $2")
> instead of just raw string concatenation.

Done.

http://codereview.chromium.org/2078021/diff/23001/24003#newcode389
chrome/browser/autocomplete/keyword_provider.cc:389:
listener_->OnProviderUpdate(true);
On 2010/05/26 01:10:42, Peter Kasting wrote:
> Nit: You should only pass "true" if you actually changed the matches.

Done.

[Peter Kasting, 2010-05-26 22:45:43]

http://codereview.chromium.org/2078021/diff/23001/24003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/23001/24003#newcode152
chrome/browser/autocomplete/keyword_provider.cc:152:
matches_.insert(matches_.begin(),
On 2010/05/26 22:04:25, Matt Perry wrote:
> On 2010/05/26 01:10:42, Peter Kasting wrote:
> > This insert() seems suspicious.  Won't both |matches| and |old_matches| have
> an
> > autocomplete match in them, resulting in a duplicate entry?  Did you test
> this?
> 
> I'm not really sure how it's supposed to work, but when I tested it, matches_
> always contained only the last asynchronous results from the extension.

It seems like typing in "extension_keyword foo" and then pressing ctrl would
result in this duplication.  Did you test that case?

http://codereview.chromium.org/2078021/diff/39002/44003#newcode98
chrome/browser/autocomplete/keyword_provider.cc:98: //
I think you shouldn't increment this if |minimal_changes| is true.

http://codereview.chromium.org/2078021/diff/39002/44003#newcode156
chrome/browser/autocomplete/keyword_provider.cc:156:
extension_suggest_last_input_ = input;
You only half-implemented the fix here.  I think what you want is to eliminate
|extension_suggest_input_id_| entirely, pass |current_input_id_| below, and
check against |current_input_id_| in Observe().

http://codereview.chromium.org/2078021/diff/39002/44003#newcode386
chrome/browser/autocomplete/keyword_provider.cc:386: }
Nit: It seems like just DCHECK(offsets.size() == 2) would be sufficient, I think
that's what we do elsewhere in the autocomplete code.

http://codereview.chromium.org/2078021/diff/39002/44012
File chrome/browser/extensions/extension_omnibox_api.h (right):

http://codereview.chromium.org/2078021/diff/39002/44012#newcode37
chrome/browser/extensions/extension_omnibox_api.h:37: typedef std::pair<int,
ListValue*> OmniboxSuggestions;
Nit: This is a pretty generic name to not have scoped to any class, in that it
doesn't really note that this has anything to do with extensions.  Maybe
OmniboxExtensionMatches or something?

[Matt Perry, 2010-05-26 23:08:56]

http://codereview.chromium.org/2078021/diff/23001/24003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/23001/24003#newcode152
chrome/browser/autocomplete/keyword_provider.cc:152:
matches_.insert(matches_.begin(),
On 2010/05/26 22:45:43, Peter Kasting wrote:
> On 2010/05/26 22:04:25, Matt Perry wrote:
> > On 2010/05/26 01:10:42, Peter Kasting wrote:
> > > This insert() seems suspicious.  Won't both |matches| and |old_matches|
have
> > an
> > > autocomplete match in them, resulting in a duplicate entry?  Did you test
> > this?
> > 
> > I'm not really sure how it's supposed to work, but when I tested it,
matches_
> > always contained only the last asynchronous results from the extension.
> 
> It seems like typing in "extension_keyword foo" and then pressing ctrl would
> result in this duplication.  Did you test that case?

Sorry, I was mistaken. The sync result (what the user typed) is present as well.
It looks like it gets culled later for being a dupe of another entry though.

http://codereview.chromium.org/2078021/diff/39002/44003#newcode156
chrome/browser/autocomplete/keyword_provider.cc:156:
extension_suggest_last_input_ = input;
On 2010/05/26 22:45:43, Peter Kasting wrote:
> You only half-implemented the fix here.  I think what you want is to eliminate
> |extension_suggest_input_id_| entirely, pass |current_input_id_| below, and
> check against |current_input_id_| in Observe().

Done.

http://codereview.chromium.org/2078021/diff/39002/44003#newcode386
chrome/browser/autocomplete/keyword_provider.cc:386: }
On 2010/05/26 22:45:43, Peter Kasting wrote:
> Nit: It seems like just DCHECK(offsets.size() == 2) would be sufficient, I
think
> that's what we do elsewhere in the autocomplete code.
> 

Done.

http://codereview.chromium.org/2078021/diff/39002/44012
File chrome/browser/extensions/extension_omnibox_api.h (right):

http://codereview.chromium.org/2078021/diff/39002/44012#newcode37
chrome/browser/extensions/extension_omnibox_api.h:37: typedef std::pair<int,
ListValue*> OmniboxSuggestions;
On 2010/05/26 22:45:43, Peter Kasting wrote:
> Nit: This is a pretty generic name to not have scoped to any class, in that it
> doesn't really note that this has anything to do with extensions.  Maybe
> OmniboxExtensionMatches or something?

Done.

[Peter Kasting, 2010-05-26 23:19:43]

LGTM pending implementation of the minimal_changes fix we discussed in person;
you should also consider clamping to AutocompleteProvider::kMaxMatches (possibly
+ 1), although it's possible you don't want to do that.  (See the search
provider for sample usage.)

[Aaron Boodman, 2010-05-27 22:26:06]

LGTM

[Peter Kasting, 2010-05-27 23:42:14]

http://codereview.chromium.org/2078021/diff/64001/65003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/64001/65003#newcode155
chrome/browser/autocomplete/keyword_provider.cc:155: done_ = true;
We shouldn't set done_ = true here (if it's not true already, then doing this
would be wrong, as the extension query is still running).

http://codereview.chromium.org/2078021/diff/64001/65003#newcode156
chrome/browser/autocomplete/keyword_provider.cc:156: if (input.prefer_keyword())
{
I don't think we should be checking input.prefer_keyword() here.  We didn't
create the original matches conditionally on this.  This check would break calls
with minimal_changes true that weren't caused by hitting tab (e.g. hitting ctrl
instead).

[Matt Perry, 2010-05-27 23:49:29]

http://codereview.chromium.org/2078021/diff/64001/65003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2078021/diff/64001/65003#newcode155
chrome/browser/autocomplete/keyword_provider.cc:155: done_ = true;
On 2010/05/27 23:42:14, Peter Kasting wrote:
> We shouldn't set done_ = true here (if it's not true already, then doing this
> would be wrong, as the extension query is still running).

Done.

http://codereview.chromium.org/2078021/diff/64001/65003#newcode156
chrome/browser/autocomplete/keyword_provider.cc:156: if (input.prefer_keyword())
{
On 2010/05/27 23:42:14, Peter Kasting wrote:
> I don't think we should be checking input.prefer_keyword() here.  We didn't
> create the original matches conditionally on this.  This check would break
calls
> with minimal_changes true that weren't caused by hitting tab (e.g. hitting
ctrl
> instead).

I was attempting to fix another issue - the relevance could be out of date - but
I think I have a better fix now.

[Peter Kasting, 2010-05-27 23:55:53]

LGTM