Disable most of search provider for Instant Extended

Disable most of search provider for Instant Extended

Limit the search provider to do almost nothing if the Instant Extended API is enabled.  We only need it as the gateway for incoming API-provided suggestions.


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

[David Black, 2012-09-19 23:02:52]

Ready for review.

[David Black, 2012-09-19 23:03:47]

Whoops, meant to send to pkasting instead of sky.  Fixed.

[sreeram, 2012-09-19 23:07:41]

lgtm

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:280: // TODO: once we are done
refactoring the omnibox so we don't need to use
TODOs should have names, so put some name here (pick from dcblack, shishir,
sreeram).

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:283: if
(!chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
Consider using InstantController::IsExtendedAPIEnabled instead (since it also
checks for whether or not Instant itself is enabled).

[David Black, 2012-09-19 23:31:31]

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:280: // TODO: once we are done
refactoring the omnibox so we don't need to use
On 2012/09/19 23:07:41, sreeram wrote:
> TODOs should have names, so put some name here (pick from dcblack, shishir,
> sreeram).

Done.

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:283: if
(!chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
We should really not have two "are we active" functions that appear identical
like this but actually aren't.  There needs to be one canonical way to determine
if we're doing all our extended API mojo - one of these functions should be
deleted.

In the long run, whenever the Extended API is enabled we will show the overlay,
and the Instant preference toggle will just control whether or not results are
displayed.  As such, unless I'm missing something, I'll keep it as-is.

On 2012/09/19 23:07:41, sreeram wrote:
> Consider using InstantController::IsExtendedAPIEnabled instead (since it also
> checks for whether or not Instant itself is enabled).

[Peter Kasting, 2012-09-19 23:46:05]

LGTM

[commit-bot: I haz the power, 2012-09-19 23:48:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcblack@chromium.org/10957005/5001

[sreeram, 2012-09-19 23:51:26]

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:283: if
(!chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
On 2012/09/19 23:31:32, David Black wrote:
> We should really not have two "are we active" functions that appear identical
> like this but actually aren't.  There needs to be one canonical way to
determine
> if we're doing all our extended API mojo - one of these functions should be
> deleted.
> 
> In the long run, whenever the Extended API is enabled we will show the
overlay,
> and the Instant preference toggle will just control whether or not results are
> displayed.  As such, unless I'm missing something, I'll keep it as-is.

Well, we'll need some way for the search_provider to distinguish between "the
renderer will talk to google.com and fetch suggestions and search history"
versus "the renderer can't talk to any servers, so I better fetch at least the
search history queries". But that only comes into play when we have the offline
omnibox ready, at which time, we'll fix it all up. (For example, the
chrome://settings checkbox for Instant doesn't behave the way you envision it,
so that also needs to be fixed.)

[David Black, 2012-09-20 00:01:27]

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/10957005/diff/1/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:283: if
(!chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
On 2012/09/19 23:51:26, sreeram wrote:
> On 2012/09/19 23:31:32, David Black wrote:
> > We should really not have two "are we active" functions that appear
identical
> > like this but actually aren't.  There needs to be one canonical way to
> determine
> > if we're doing all our extended API mojo - one of these functions should be
> > deleted.
> > 
> > In the long run, whenever the Extended API is enabled we will show the
> overlay,
> > and the Instant preference toggle will just control whether or not results
are
> > displayed.  As such, unless I'm missing something, I'll keep it as-is.
> 
> Well, we'll need some way for the search_provider to distinguish between "the
> renderer will talk to http://google.com and fetch suggestions and search
history"
> versus "the renderer can't talk to any servers, so I better fetch at least the
> search history queries". But that only comes into play when we have the
offline
> omnibox ready, at which time, we'll fix it all up. (For example, the
> chrome://settings checkbox for Instant doesn't behave the way you envision it,
> so that also needs to be fixed.)

Ah, fair enough.  I forgot about offline mode.  Yeah, we'll fix this logic up
then.

[commit-bot: I haz the power, 2012-09-20 03:25:21]

Change committed as 157691