Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index deb105362a9cd45f974c0adc4da5215d8926a8f8..ec4e36ae91c1f45e1754e01b30b0759c0ae74cca 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -184,7 +184,10 @@ void OmniboxEditModel::RestoreState(const State& state) {
   }
 }
 
-AutocompleteMatch OmniboxEditModel::CurrentMatch() {
+AutocompleteMatch OmniboxEditModel::CurrentMatch() const {
+  // If we have a valid match use it. Otherwise get one for the current text.
+  if (omnibox_controller_->current_match().destination_url.is_valid())

[Peter Kasting, 2013/06/11 22:42:55]

I think you can have a valid match without a valid destination URL.  Try
entering just "?" as the omnibox text, and you'll get a search provider match
with no destination URL.

I don't know if this is a relevant fact to this particular code, though.

[beaudoin, 2013/06/14 21:39:38]

Thanks for pointing this out. I don't think it has an impact here. In the long
run the goal would be to get rid of GetInfoForCurrentText. This will require
saving the AutocompleteMatch to some tab context so it can be restored when you
switch tab. (Right now, we hope that the current text is enough to recreate the
AutocompleteMatch but it's not necessarily the case with InstantController.)

+    return omnibox_controller_->current_match();
   AutocompleteMatch match;
   GetInfoForCurrentText(&match, NULL);
   return match;
@@ -220,6 +223,7 @@ GURL OmniboxEditModel::PermanentURL() {
 void OmniboxEditModel::SetUserText(const string16& text) {
   SetInputInProgress(true);
   InternalSetUserText(text);
+  omnibox_controller_->InvalidateCurrentMatch();
   paste_state_ = NONE;
   has_temporary_text_ = false;
   is_temporary_text_set_by_instant_ = false;
@@ -227,55 +231,9 @@ void OmniboxEditModel::SetUserText(const string16& text) {
   is_instant_temporary_text_a_search_query_ = false;
 }
 
-void OmniboxEditModel::FinalizeInstantQuery(
-    const string16& input_text,
-    const InstantSuggestion& suggestion) {
-  if (!popup_model()->result().empty()) {
-    // When a IME is active and a candidate window is open, we don't show
-    // the omnibox popup, though |result()| may be available.  Thus we check
-    // whether result().empty() or not instead of whether IsOpen() or not.
-    // We need the finalization of instant query when result() is available.
-    SearchProvider* search_provider =
-        autocomplete_controller()->search_provider();
-    // There may be no providers during testing; guard against that.
-    if (search_provider)
-      search_provider->FinalizeInstantQuery(input_text, suggestion);
-  }
-}
-
 void OmniboxEditModel::SetInstantSuggestion(
     const InstantSuggestion& suggestion) {
-  switch (suggestion.behavior) {
-    case INSTANT_COMPLETE_NOW:
-      view_->SetInstantSuggestion(string16());
-      if (!suggestion.text.empty())
-        FinalizeInstantQuery(view_->GetText(), suggestion);
-      break;
-
-    case INSTANT_COMPLETE_NEVER: {
-      DCHECK_EQ(INSTANT_SUGGESTION_SEARCH, suggestion.type);
-      view_->SetInstantSuggestion(suggestion.text);
-      autocomplete_controller()->search_provider()->ClearInstantSuggestion();
-      break;
-    }
-
-    case INSTANT_COMPLETE_REPLACE: {
-      const bool save_original_selection = !has_temporary_text_;
-      view_->SetInstantSuggestion(string16());
-      has_temporary_text_ = true;
-      is_temporary_text_set_by_instant_ = true;
-      selected_instant_autocomplete_match_index_ =
-          suggestion.autocomplete_match_index;
-      is_instant_temporary_text_a_search_query_ =
-          suggestion.type == INSTANT_SUGGESTION_SEARCH;
-      // Instant suggestions are never a keyword.
-      keyword_ = string16();
-      is_keyword_hint_ = false;
-      view_->OnTemporaryTextMaybeChanged(suggestion.text,
-                                         save_original_selection, true);
-      break;
-    }
-  }
+  omnibox_controller_->SetInstantSuggestion(suggestion);
 }
 
 bool OmniboxEditModel::CommitSuggestedText() {
@@ -339,7 +297,7 @@ void OmniboxEditModel::OnChanged() {
     view_->SetInstantSuggestion(string16());
 
     // No need to wait any longer for Instant.
-    FinalizeInstantQuery(string16(), InstantSuggestion());
+    omnibox_controller_->FinalizeInstantQuery(string16(), InstantSuggestion());
   }
 
   switch (recommended_action) {
@@ -367,8 +325,7 @@ void OmniboxEditModel::OnChanged() {
 void OmniboxEditModel::GetDataForURLExport(GURL* url,
                                            string16* title,
                                            gfx::Image* favicon) {
-  AutocompleteMatch match;
-  GetInfoForCurrentText(&match, NULL);
+  AutocompleteMatch match = CurrentMatch();

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Just inline into the next statement

[beaudoin, 2013/06/14 21:39:38]

Done.

   *url = match.destination_url;
   if (*url == URLFixerUpper::FixupURL(UTF16ToUTF8(permanent_text_),
                                       std::string())) {
@@ -390,15 +347,12 @@ bool OmniboxEditModel::CurrentTextIsURL() const {
   if (!user_input_in_progress_)
     return true;
 
-  AutocompleteMatch match;
-  GetInfoForCurrentText(&match, NULL);
+  AutocompleteMatch match = CurrentMatch();

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Here too

[beaudoin, 2013/06/14 21:39:38]

Done.

   return !AutocompleteMatch::IsSearchType(match.type);
 }
 
 AutocompleteMatch::Type OmniboxEditModel::CurrentTextType() const {
-  AutocompleteMatch match;
-  GetInfoForCurrentText(&match, NULL);
-  return match.type;
+  return CurrentMatch().type;
 }
 
 void OmniboxEditModel::AdjustTextForCopy(int sel_min,
@@ -556,9 +510,18 @@ bool OmniboxEditModel::IsPasteAndSearch(const string16& text) const {
 void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
                                    bool for_drop) {
   // Get the URL and transition type for the selected entry.
-  AutocompleteMatch match;
+  AutocompleteMatch match = omnibox_controller_->current_match();
   GURL alternate_nav_url;
-  GetInfoForCurrentText(&match, &alternate_nav_url);
+
+  if (!match.destination_url.is_valid()) {
+    GetInfoForCurrentText(&match, &alternate_nav_url);

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Maybe CurrentMatch() should take an |alternate_nav_url| arg (can be NULL),
so we can just use that in this function.

[beaudoin, 2013/06/14 21:39:38]

Done.

+  } else if (AutocompleteMatch::IsSearchType(match.type)) {
+    // Compute the |alternate_nav_url| if needed.

[Peter Kasting, 2013/06/11 22:42:55]

Why do we need this new block?  It worries me because passing the fill_into_edit
unconditionally to FixupURL() seems problematic.  I really think the providers,
not the edit model, should always be the ones to compute this.

[beaudoin, 2013/06/14 21:39:38]

The problem is that |alternate_nav_url| is computed by
AutocompleteResult::SortAndCull so it's not attached with every
AutocompleteMatch. In particular, we have nothing attached to the
AutocompleteMatch computed from the instant suggestion.

The code you see here is very similar to the one used by
AutocompleteInput::Parse() to compute |canonicalized_url| (which becomes
|alternate_nav_url|). The difference is that Parse() uses
AutocompleteInput.text_ where we use |fill_into_edit|. Do you see a problem with
this approach?

[Peter Kasting, 2013/06/15 00:29:21]

I see.  I think we should factor out the last piece of SortAndCull() into a
function somewhere that can take an AutocompleteInput and an AutocompleteMatch
and return an alternate_nav_url.  Then we could call that from here (well,
CurrentMatch(), now that you moved this).  The current AutocompleteInput is
available today on the AutocompleteController, I'm sure we can find a good
location to hold it so that we could use it here.

I'm not really happy today with the way the instant suggestion sort of bypasses
the entire controller->provider->match->result pipeline and gets used at the
very end of everything.  It would be nice to ultimately unify that pipeline
enough that something like what AutocompelteResult::SortAndCull() is doing only
needs to happen once to compute the right results for everything.

[beaudoin, 2013/06/17 17:22:33]

For the moment, the autocomplete controller is started/stopped from totally
different paths than instant, which makes it really annoying to reliably store
an AutocompleteInput that would be valid whenever CurrentMatch is called. See
Sreeram's earlier comment:
https://codereview.chromium.org/14698028/diff/33001/chrome/browser/ui/omnibox...

However, looking at autcomplete_controller.cc it appears |input_| is only set on
a call to Start() and Start() seems to be always called before DoInstant()
(seeing as OmniboxController::DoInstant relies on the
autocomplete_controller_->input()). I've therefore decided to simply rely on it
in OmniboxController::StartAutocomplete.
I'm annoyed by that too. I had many discussions with Sreeram on this, and I
think the final consensus was that it wasn't clear why instant was not conceived
as a Provider to begin with. Re-unifying the pipelines should definitely be a
prime focus of a future refactor once we re-enable instant suggestions.

That said, I think keeping an AutocompleteMatch (or two, if we support gray
completion) in OmniboxController makes sense. This way we can save it in
OmniboxEditModel::State so we can restore it on tab switch.

+    alternate_nav_url = URLFixerUpper::FixupURL(
+        UTF16ToUTF8(match.fill_into_edit), std::string());
+    if (alternate_nav_url == match.destination_url)
+      alternate_nav_url = GURL();
+  }
 
   // If CTRL is down it means the user wants to append ".com" to the text he
   // typed. If we can successfully generate a URL_WHAT_YOU_TYPED match doing
@@ -626,6 +589,7 @@ void OmniboxEditModel::OpenMatch(const AutocompleteMatch& match,
                                  WindowOpenDisposition disposition,
                                  const GURL& alternate_nav_url,
                                  size_t index) {
+

[Peter Kasting, 2013/06/11 22:42:55]

Nit: Extra newline

[beaudoin, 2013/06/14 21:39:38]

Done.

   // We only care about cases where there is a selection (i.e. the popup is
   // open).
   if (popup_model()->IsOpen()) {
@@ -695,11 +659,8 @@ void OmniboxEditModel::OpenMatch(const AutocompleteMatch& match,
     if (match.transition == content::PAGE_TRANSITION_KEYWORD) {
       // The user is using a non-substituting keyword or is explicitly in
       // keyword mode.
-
-      AutocompleteMatch current_match;
-      GetInfoForCurrentText(&current_match, NULL);
       const AutocompleteMatch& match = (index == OmniboxPopupModel::kNoMatch) ?
-          current_match : result().match_at(index);
+          CurrentMatch() : result().match_at(index);
 
       // Don't increment usage count for extension keywords.
       if (delegate_->ProcessExtensionKeyword(template_url, match,
@@ -884,9 +845,7 @@ void OmniboxEditModel::OnKillFocus() {
 
 bool OmniboxEditModel::OnEscapeKeyPressed() {
   if (has_temporary_text_) {
-    AutocompleteMatch match;
-    GetInfoForCurrentText(&match, NULL);
-    if (match.destination_url != original_url_) {
+    if (CurrentMatch().destination_url != original_url_) {
       RevertTemporaryText(true);
       return true;
     }
@@ -1160,7 +1119,37 @@ bool OmniboxEditModel::OnAfterPossibleChange(const string16& old_text,
            MaybeAcceptKeywordBySpace(user_text_));
 }
 
-void OmniboxEditModel::OnResultChanged(bool default_match_changed) {
+void OmniboxEditModel::OnCurrentMatchChanged(bool is_temporary_set_by_instant) {
+  const bool save_original_selection =
+      is_temporary_set_by_instant && !has_temporary_text_;
+  has_temporary_text_ = is_temporary_set_by_instant;
+  is_temporary_text_set_by_instant_ = is_temporary_set_by_instant;
+
+  const AutocompleteMatch& match = omnibox_controller_->current_match();
+  match.GetKeywordUIState(profile_, &keyword_, &is_keyword_hint_);
+
+  view_->SetInstantSuggestion(match.gray_suggestion);
+  string16 inline_autocomplete_text;
+  if ((match.inline_autocomplete_offset != string16::npos) &&

[Peter Kasting, 2013/06/11 22:42:55]

Nit: This first condition is unnecessary; in cases where it would fail, we will
always fail the second condition.

[beaudoin, 2013/06/14 21:39:38]

Done.

+      (match.inline_autocomplete_offset <
+       match.fill_into_edit.length())) {
+    // We have blue text, go through OnPopupDataChanged.
+    // TODO(beaudoin): Merge OnPopupDataChanged with this method once the popup
+    // handling has completely migrated to omnibox_controller.
+    inline_autocomplete_text =
+        match.fill_into_edit.substr(match.inline_autocomplete_offset);
+    popup_model()->OnResultChanged();
+    OnPopupDataChanged(inline_autocomplete_text, NULL, keyword_,
+                       is_keyword_hint_);
+  } else {
+    view_->OnTemporaryTextMaybeChanged(

[Peter Kasting, 2013/06/11 22:42:55]

I'm trying to figure out where this used to happen.  Was this the net result of
the code previously calling
popup_model()->OnResultChanged()/OnPopupDataChanged()?  Why don't we need to
handle any of the keyword state stuff that function does (or any other omitted
bits of it) in this case?

+        DisplayTextFromUserText(match.fill_into_edit), save_original_selection,
+        false);
+  }
+}
+
+string16 OmniboxEditModel::GetViewText() const {
+  return view_->GetText();
 }
 
 InstantController* OmniboxEditModel::GetInstantController() const {