Add support for omnibox.onInputStarted and onInputCancelled.

Add support for omnibox.onInputStarted and onInputCancelled.

Also fix a bug where we'd keep an extension's old keyword even if it changed
on upgrade/reload.

BUG=46475
BUG=48091
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51540

[Matt Perry, 2010-07-01 18:35:55]

Peter: *autocomplete*
Aaron: everything else

[Aaron Boodman, 2010-07-01 19:08:20]

lgtm

http://codereview.chromium.org/2807033/diff/4001/5007
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/2807033/diff/4001/5007#newcode905
chrome/browser/extensions/extensions_service.cc:905: if
(profile_->GetTemplateURLModel())
Hooray, I win. (OnInstalled->OnLoaded)

http://codereview.chromium.org/2807033/diff/4001/5009
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/2807033/diff/4001/5009#newcode3137
chrome/common/extensions/api/extension_api.json:3137: "description": "User has
started a keyword input session by typing the extension's keyword. This is
guaranteed to be sent exactly once per session, and before any onInputChanged
events.",
How about "input session" instead of just "session"?

http://codereview.chromium.org/2807033/diff/4001/5011
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/2807033/diff/4001/5011#newcode297
chrome/renderer/resources/extension_process_bindings.js:297:
chrome.experimental.omnibox.onInputStarted =
Sad that these require custom bindings. Is there something we can do to automate
it?

[Peter Kasting, 2010-07-01 19:41:31]

LGTM with a couple suggestions for simplification.  I didn't review the
extension files, just the autocomplete and template_url stuff.

http://codereview.chromium.org/2807033/diff/4001/5002
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/2807033/diff/4001/5002#newcode537
chrome/browser/autocomplete/autocomplete.h:537: // notification to the provider
to do any cleanup it needs.
It seems like it would be simpler to just change the controller to call Stop()
on all providers regardless of done() state, and then check to make sure the
providers all behave sanely in that case (they should).

This also better matches the model of e.g. Start(), where it can be called
whether or not a query is running.

Two questions I have with either method are whether you need to be called when a
query is interrupted by a new query, and whether you need to be called when a
query finishes normally (without cancelling).  I think the answer is "no" to
both, but I want to make sure.

http://codereview.chromium.org/2807033/diff/4001/5003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2807033/diff/4001/5003#newcode125
chrome/browser/autocomplete/keyword_provider.cc:125:
MaybeEndExtensionKeywordMode();
Because you insert this into so many exit points in this function, I suggest
changing MaybeEndExtensionKeywordMode() from a function into a (small, local to
this file) object that runs that code in its destructor.  Then simply
instantiate one of these at the top of this function, as well as NotifyStop()
(or Stop(), depending on what you do with my other suggestion).

http://codereview.chromium.org/2807033/diff/4001/5003#newcode179
chrome/browser/autocomplete/keyword_provider.cc:179: if
(current_keyword_extension_id_.empty()) {
Nit: No {}

[Matt Perry, 2010-07-01 23:47:04]

http://codereview.chromium.org/2807033/diff/4001/5002
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/2807033/diff/4001/5002#newcode537
chrome/browser/autocomplete/autocomplete.h:537: // notification to the provider
to do any cleanup it needs.
On 2010/07/01 19:41:32, Peter Kasting wrote:
> It seems like it would be simpler to just change the controller to call Stop()
> on all providers regardless of done() state, and then check to make sure the
> providers all behave sanely in that case (they should).
> 
> This also better matches the model of e.g. Start(), where it can be called
> whether or not a query is running.
 
Cool, I like that much better.

> Two questions I have with either method are whether you need to be called when
a
> query is interrupted by a new query, and whether you need to be called when a
> query finishes normally (without cancelling).  I think the answer is "no" to
> both, but I want to make sure.

Good catch. I had to add a notification from OnInputEntered so that we wouldn't
also send out OnInputCancelled. I'm not sure what you mean by "interrupted by a
new query" though. If the user starts typing something else, we should exit
"extension keyword" mode.

http://codereview.chromium.org/2807033/diff/4001/5003
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2807033/diff/4001/5003#newcode125
chrome/browser/autocomplete/keyword_provider.cc:125:
MaybeEndExtensionKeywordMode();
On 2010/07/01 19:41:32, Peter Kasting wrote:
> Because you insert this into so many exit points in this function, I suggest
> changing MaybeEndExtensionKeywordMode() from a function into a (small, local
to
> this file) object that runs that code in its destructor.  Then simply
> instantiate one of these at the top of this function, as well as NotifyStop()
> (or Stop(), depending on what you do with my other suggestion).

Done.

http://codereview.chromium.org/2807033/diff/4001/5003#newcode179
chrome/browser/autocomplete/keyword_provider.cc:179: if
(current_keyword_extension_id_.empty()) {
On 2010/07/01 19:41:32, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/2807033/diff/4001/5007
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/2807033/diff/4001/5007#newcode905
chrome/browser/extensions/extensions_service.cc:905: if
(profile_->GetTemplateURLModel())
On 2010/07/01 19:08:21, Aaron Boodman wrote:
> Hooray, I win. (OnInstalled->OnLoaded)

for now....

http://codereview.chromium.org/2807033/diff/4001/5009
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/2807033/diff/4001/5009#newcode3137
chrome/common/extensions/api/extension_api.json:3137: "description": "User has
started a keyword input session by typing the extension's keyword. This is
guaranteed to be sent exactly once per session, and before any onInputChanged
events.",
On 2010/07/01 19:08:21, Aaron Boodman wrote:
> How about "input session" instead of just "session"?

Done.

http://codereview.chromium.org/2807033/diff/4001/5011
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/2807033/diff/4001/5011#newcode297
chrome/renderer/resources/extension_process_bindings.js:297:
chrome.experimental.omnibox.onInputStarted =
On 2010/07/01 19:08:21, Aaron Boodman wrote:
> Sad that these require custom bindings. Is there something we can do to
automate
> it?

Good idea. Done.

[Peter Kasting, 2010-07-02 00:00:17]

On 2010/07/01 23:47:04, Matt Perry wrote:
> > Two questions I have with either method are whether you need to be called
when
> a
> > query is interrupted by a new query, and whether you need to be called when
a
> > query finishes normally (without cancelling).  I think the answer is "no" to
> > both, but I want to make sure.
> 
> Good catch. I had to add a notification from OnInputEntered so that we
wouldn't
> also send out OnInputCancelled. I'm not sure what you mean by "interrupted by
a
> new query" though. If the user starts typing something else, we should exit
> "extension keyword" mode.

I mean, each time you type a character, it calls Start() on all the providers
without calling Stop() first.  I just want to make sure this doesn't throw any
wrenches into your plans.

http://codereview.chromium.org/2807033/diff/7003/15002
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/2807033/diff/7003/15002#newcode131
chrome/browser/autocomplete/history_contents_provider.cc:131: if (done_)
Nit: I don't think you need this; the code below should be safe at any time.

http://codereview.chromium.org/2807033/diff/7003/15003
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/2807033/diff/7003/15003#newcode62
chrome/browser/autocomplete/history_url_provider.cc:62: if (done_)
Nit: I don't think you need this; the code below should be safe at any time.

http://codereview.chromium.org/2807033/diff/7003/15004
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/2807033/diff/7003/15004#newcode31
chrome/browser/autocomplete/keyword_provider.cc:31:
provider_->MaybeEndExtensionKeywordMode();
I think you can simplify even more with the following changes:

* Replace the stack object in Start() with a scoped_ptr<>.  Replace this:

keyword_mode_toggle.StayInKeywordMode();
if (extension->id() != current_keyword_extension_id_)
  MaybeEndExtensionKeywordMode();

With this:

if (extension->id() != current_keyword_extension_id_)
  keyword_mode_toggle.reset();
else
  keyword_mode_toggle->StayInKeywordMode();

* Replace the call in Stop() with one of these scoping objects.

* At this point only this object calls MaybeEndExtensionKeywordMode(), so pass
in current_keyword_extension_id_ (and the profile) instead of the provider, and
inline MaybeEndExtensionKeywordMode() into here.

Now you can get rid of the friend declaration too.

http://codereview.chromium.org/2807033/diff/7003/15006
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/2807033/diff/7003/15006#newcode147
chrome/browser/autocomplete/search_provider.cc:147: if (done_)
Nit: I don't think you need this; the code below should be safe at any time.

[Peter Kasting, 2010-07-02 00:02:14]

Oh, we should also probably say explicitly on the base class Stop() declaration
that it may be called even if the provider is already done, if we don't already.