Omnibox refactor. OmniboxController now holds an AutocompleteMatch.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

 // Copyright 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/auto_reset.h"
 #include "base/format_macros.h"
@@ skipping 166 matching lines @@
     // NOTE: Be sure and set keyword-related state BEFORE invoking
     // DisplayTextFromUserText(), as its result depends upon this state.
     keyword_ = state.keyword;
     is_keyword_hint_ = state.is_keyword_hint;
     view_->SetUserText(state.user_text,
         DisplayTextFromUserText(state.user_text), false);
     view_->SetInstantSuggestion(state.instant_suggestion);
   }
 }
 
-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;
 }
 
 bool OmniboxEditModel::UpdatePermanentText(const string16& new_permanent_text) {
   // When there's a new URL, and the user is not editing anything or the edit
   // doesn't have focus, we want to revert the edit to show the new URL.  (The
   // common case where the edit doesn't have focus is when the user has started
   // an edit and then abandoned it and clicked a link on the page.)
@@ skipping 15 matching lines @@
   return visibly_changed_permanent_text;
 }
 
 GURL OmniboxEditModel::PermanentURL() {
   return URLFixerUpper::FixupURL(UTF16ToUTF8(permanent_text_), std::string());
 }
 
 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;
   selected_instant_autocomplete_match_index_ = OmniboxPopupModel::kNoMatch;
   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() {
   const string16 suggestion = view_->GetInstantSuggestion();
   if (suggestion.empty())
     return false;
 
   // Assume that the gray text we are committing is a search suggestion.
   const string16 final_text = view_->GetText() + suggestion;
   view_->OnBeforePossibleChange();
@@ skipping 43 matching lines @@
         user_input_in_progress_, in_escape_handler_,
         view_->DeleteAtEndPressed() || just_deleted_text_,
         KeywordIsSelected());
   }
 
   if (!performed_instant) {
     // Hide any suggestions we might be showing.
     view_->SetInstantSuggestion(string16());
 
     // No need to wait any longer for Instant.
-    FinalizeInstantQuery(string16(), InstantSuggestion());
+    omnibox_controller_->FinalizeInstantQuery(string16(), InstantSuggestion());
   }
 
   switch (recommended_action) {
     case AutocompleteActionPredictor::ACTION_PRERENDER:
       // It's possible that there is no current page, for instance if the tab
       // has been closed or on return from a sleep state.
       // (http://crbug.com/105689)
       if (!delegate_->CurrentPageExists())
         break;
       // Ask for prerendering if the destination URL is different than the
       // current URL.
       if (current_match.destination_url != PermanentURL())
         delegate_->DoPrerender(current_match);
       break;
     case AutocompleteActionPredictor::ACTION_PRECONNECT:
       omnibox_controller_->DoPreconnect(current_match);
       break;
     case AutocompleteActionPredictor::ACTION_NONE:
       break;
   }
 
   controller_->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())) {
     *title = controller_->GetTitle();
     *favicon = controller_->GetFavicon();
   }
 }
 
 bool OmniboxEditModel::CurrentTextIsURL() const {
   if (view_->toolbar_model()->GetSearchTermsType() !=
       ToolbarModel::NO_SEARCH_TERMS)
     return false;
 
   // If current text is not composed of replaced search terms and
   // !user_input_in_progress_, then permanent text is showing and should be a
   // URL, so no further checking is needed.  By avoiding checking in this case,
   // we avoid calling into the autocomplete providers, and thus initializing the
   // history system, as long as possible, which speeds startup.
   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,
                                          bool is_all_selected,
                                          string16* text,
                                          GURL* url,
                                          bool* write_url) {
   *write_url = false;
 
   // Do not adjust if selection did not start at the beginning of the field, or
@@ skipping 137 matching lines @@
 
 bool OmniboxEditModel::IsPasteAndSearch(const string16& text) const {
   AutocompleteMatch match;
   ClassifyStringForPasteAndGo(text, &match, NULL);
   return AutocompleteMatch::IsSearchType(match.type);
 }
 
 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
   // that, then we use this. These matches are marked as generated by the
   // HistoryURLProvider so we only generate them if this provider is present.
   if (control_key_state_ == DOWN_WITHOUT_CHANGE && !KeywordIsSelected() &&
       autocomplete_controller()->history_url_provider()) {
     // Generate a new AutocompleteInput, copying the latest one but using "com"
     // as the desired TLD. Then use this autocomplete input to generate a
     // URL_WHAT_YOU_TYPED AutocompleteMatch. Note that using the most recent
@@ skipping 47 matching lines @@
     GoogleURLTracker::GoogleURLSearchCommitted(profile_);
 
   view_->OpenMatch(match, disposition, alternate_nav_url,
                    OmniboxPopupModel::kNoMatch);
 }
 
 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()) {
     const base::TimeTicks& now(base::TimeTicks::Now());
     base::TimeDelta elapsed_time_since_user_first_modified_omnibox(
         now - time_user_first_modified_omnibox_);
     base::TimeDelta elapsed_time_since_last_change_to_default_match(
         now - autocomplete_controller()->last_time_default_match_changed());
     // These elapsed times don't really make sense for ZeroSuggest matches
     // (because the user does not modify the omnibox for ZeroSuggest), so for
@@ skipping 49 matching lines @@
         content::Source<Profile>(profile_),
         content::Details<OmniboxLog>(&log));
     HISTOGRAM_ENUMERATION("Omnibox.EventCount", 1, 2);
   }
 
   TemplateURL* template_url = match.GetTemplateURL(profile_, false);
   if (template_url) {
     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,
                                              disposition)) {
         if (disposition != NEW_BACKGROUND_TAB)
           view_->RevertAll();
         return;
       }
 
       content::RecordAction(UserMetricsAction("AcceptedKeyword"));
@@ skipping 164 matching lines @@
   // TODO(samarth): determine if it is safe to move the call to
   // OmniboxFocusChanged() from OnWillKillFocus() to here, which would let us
   // just call SetFocusState() to handle the state change.
   focus_state_ = OMNIBOX_FOCUS_NONE;
   control_key_state_ = UP;
   paste_state_ = NONE;
 }
 
 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;
     }
   }
 
   // We do not clear the pending entry from the omnibox when a load is first
   // stopped.  If the user presses Escape while stopped, we clear it.
   if (delegate_->CurrentPageExists() && !delegate_->IsLoading()) {
     delegate_->GetNavigationController().DiscardNonCommittedEntries();
     view_->Update(NULL);
@@ skipping 253 matching lines @@
   // determine what keyword, if any, is applicable.
   //
   // If MaybeAcceptKeywordBySpace() accepts the keyword and returns true, that
   // will have updated our state already, so in that case we don't also return
   // true from this function.
   return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
            no_selection && (selection_start == user_text_.length()) &&
            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 {
   return controller_->GetInstant();
 }
 
 bool OmniboxEditModel::query_in_progress() const {
   return !autocomplete_controller()->done();
 }
 
@@ skipping 234 matching lines @@
     instant->OmniboxFocusChanged(state, reason, NULL);
 
   // Update state and notify view if the omnibox has focus and the caret
   // visibility changed.
   const bool was_caret_visible = is_caret_visible();
   focus_state_ = state;
   if (focus_state_ != OMNIBOX_FOCUS_NONE &&
       is_caret_visible() != was_caret_visible)
     view_->ApplyCaretVisibility();
 }