Centralize logic around Instant modes.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/omnibox/omnibox_edit_model.h"
 
 #include <string>
 
 #include "base/format_macros.h"
 #include "base/metrics/histogram.h"
@@ skipping 412 matching lines @@
 
 void OmniboxEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete) const {
   ClearPopupKeywordMode();
 
   bool keyword_is_selected = KeywordIsSelected();
   popup_->SetHoveredLine(OmniboxPopupModel::kNoMatch);
   // We don't explicitly clear OmniboxPopupModel::manually_selected_match, as
   // Start ends up invoking OmniboxPopupModel::OnResultChanged which clears it.
+  SearchProvider* search_provider = autocomplete_controller_->search_provider();

[Peter Kasting, 2012/08/24 22:29:52]

This seems like a strange place for this code.  It seems like instead it would
be better to set this when the instant mode changes, instead of setting it
before every single query.

[sreeram, 2012/08/24 23:18:12]

The Instant mode changes happen at the chrome/ui/browser/browser.cc level.
There's no direct coordination with the omnibox code. Of course, we can always
add a notification or observer or such, but that seems overkill.

This place seems logical to me because when we want to see if Instant will
autocomplete anything, it seems correct to do so when we are about to start
running the autocomplete system, because that's what is affected by Instant's
suggestion.

[Peter Kasting, 2012/08/24 23:25:44]

What about having the SearchProvider reach out and get the instant object
directly and ask it if suggest is enabled?  It's similar in terms of the
dependency added, it's just the opposite direction, which is probably more
appropriate (make the search provider know about instant rather than having
instant know about the search provider).

The reason I'm saying this is because there's really no reason why the
OmniboxEditModel should know or care about either the search provider or whether
instant suggest is enabled, it's just plumbing it because the info happens to be
available there.  And it's weird to call this sticky setter every time we ask
the AutocompleteController to run a query for various reasons: the setter
lifetime is really permanent but we're sort of implying it's for the life of the
query, but then we're not passing this as a query-scoped argument like the rest
of the args to Start(); this kind of relies on the implicit knowledge that
Start() will eventually call through to the search provider to ask it to do
something that depends on this state; etc.

[sreeram, 2012/08/24 23:38:58]

That's exactly what I had done in patchset 1. Scott pointed out, rightly I
think, that the rest of the autocomplete system (i.e., apart from
SearchProvider), which logically operates at a lower level, shouldn't have to
know about higher level stuff like Browser or Instant.
The OmniboxEditModel does deal with the search provider directly today, again
for Instant-related stuff (search_provider_->FinalizeInstantQuery()), so this
doesn't seem to be a novel overreaching in that sense.

However, I agree with you about its ideal role. In fact, that's exactly what we
are trying to do with the redesign to accommodate the non-native rendering of
the dropdown (the one Shishir is working on). In that design (which we hope to
have for review Real Soon Now), the omnibox has minimal knowledge of both the
autocomplete system as well as Instant.
I'm happy to plumb the bool through Start() and into the providers. Do let me
know if you prefer that.

+  if (search_provider) {
+    search_provider->set_instant_suggest_enabled(controller_->GetInstant() &&
+        controller_->GetInstant()->IsSuggestEnabled());
+  }
   autocomplete_controller_->Start(
       user_text_, GetDesiredTLD(),
       prevent_inline_autocomplete || just_deleted_text_ ||
       (has_selected_text && inline_autocomplete_text_.empty()) ||
       (paste_state_ != NONE), keyword_is_selected,
       keyword_is_selected || allow_exact_keyword_match_,
       AutocompleteInput::ALL_MATCHES);
 }
 
 void OmniboxEditModel::StopAutocomplete() {
@@ skipping 773 matching lines @@
 }
 
 void OmniboxEditModel::ClassifyStringForPasteAndGo(
     const string16& text,
     AutocompleteMatch* match,
     GURL* alternate_nav_url) const {
   DCHECK(match);
   AutocompleteClassifierFactory::GetForProfile(profile_)->Classify(text,
       string16(), false, false, match, alternate_nav_url);
 }