Refactor OmniboxEditModel::user_text_ to not include keyword

components/omnibox/browser/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 "components/omnibox/browser/omnibox_edit_model.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 185 matching lines @@
 }
 
 const OmniboxEditModel::State OmniboxEditModel::GetStateForTabSwitch() {
   // Like typing, switching tabs "accepts" the temporary text as the user
   // text, because it makes little sense to have temporary text when the
   // popup is closed.
   if (user_input_in_progress_) {
     // Weird edge case to match other browsers: if the edit is empty, revert to
     // the permanent text (so the user can get it back easily) but select it (so
     // on switching back, typing will "just work").
-    const base::string16 user_text(UserTextFromDisplayText(view_->GetText()));
-    if (user_text.empty()) {
+    const base::string16 display_text = view_->GetText();
+    if (MaybePrependKeyword(display_text).empty()) {
       base::AutoReset<bool> tmp(&in_revert_, true);
       view_->RevertAll();
       view_->SelectAll(true);
     } else {
-      InternalSetUserText(user_text);
+      InternalSetUserText(display_text);
     }
   }
 
   UMA_HISTOGRAM_BOOLEAN("Omnibox.SaveStateForTabSwitch.UserInputInProgress",
                         user_input_in_progress_);
   return State(
       user_input_in_progress_, user_text_, view_->GetGrayTextAutocompletion(),
       keyword_, is_keyword_hint_,
       controller_->GetToolbarModel()->url_replacement_enabled(),
       focus_state_, focus_source_, input_);
@@ skipping 12 matching lines @@
   // Restore the autocomplete controller's input, or clear it if this is a new
   // tab.
   input_ = state ? state->autocomplete_input : AutocompleteInput();
   if (!state)
     return;
 
   SetFocusState(state->focus_state, OMNIBOX_FOCUS_CHANGE_TAB_SWITCH);
   focus_source_ = state->focus_source;
   // Restore any user editing.
   if (state->user_input_in_progress) {
-    // NOTE: Be sure and set keyword-related state BEFORE invoking
-    // DisplayTextFromUserText(), as its result depends upon this state.
+    // NOTE: Be sure and set keyword-related state AFTER invoking
+    // SetUserText(), as SetUserText() clears the keyword state.
+    view_->SetUserText(state->user_text, false);
     keyword_ = state->keyword;
     is_keyword_hint_ = state->is_keyword_hint;
-    view_->SetUserText(state->user_text,
-        DisplayTextFromUserText(state->user_text), false);
     view_->SetGrayTextAutocompletion(state->gray_text);
   }
 }
 
 AutocompleteMatch OmniboxEditModel::CurrentMatch(
     GURL* alternate_nav_url) const {
   // If we have a valid match use it. Otherwise get one for the current text.
   AutocompleteMatch match = omnibox_controller_->current_match();
 
   if (!match.destination_url.is_valid()) {
@@ skipping 40 matching lines @@
   return visibly_changed_permanent_text;
 }
 
 GURL OmniboxEditModel::PermanentURL() {
   return url_formatter::FixupURL(base::UTF16ToUTF8(permanent_text_),
                                  std::string());
 }
 
 void OmniboxEditModel::SetUserText(const base::string16& text) {
   SetInputInProgress(true);
+  keyword_.clear();
+  is_keyword_hint_ = false;

[Peter Kasting, 2016/05/18 03:32:22]

What caller(s) of this function require that we clear this state, versus which
don't?  I just want to make sure this is the right place in the call chain to
put this code.

[Tom (Use chromium acct), 2016/05/18 19:15:35]

There were some browser tests that called SetUserText and then had a check that
the keyword was empty afterwards.  I think the only one that doesn't is on line
247, but there's a comment to clarify.  

   InternalSetUserText(text);
   omnibox_controller_->InvalidateCurrentMatch();
   paste_state_ = NONE;
   has_temporary_text_ = false;
 }
 
 bool OmniboxEditModel::CommitSuggestedText() {
   const base::string16 suggestion = view_->GetGrayTextAutocompletion();
   if (suggestion.empty())
     return false;
@@ skipping 151 matching lines @@
   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus() ? permanent_text_.length() : 0,
                                   false, true);
   client_->OnRevert();
 }
 
 void OmniboxEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete,
     bool entering_keyword_mode) {
   size_t cursor_position;

[Peter Kasting, 2016/05/18 03:32:22]

Nit: Perhaps if up here you added a temp:

  const base::string16 input_text = MaybePrependKeyword(user_text_);

...then you could refer to this in the three separate places below that do this,
saving both time and verbosity.

[Tom (Use chromium acct), 2016/05/18 19:15:36]

Done.

   if (inline_autocomplete_text_.empty()) {
     // Cursor position is equivalent to the current selection's end.
     size_t start;
     view_->GetSelectionBounds(&start, &cursor_position);
-    // If we're in keyword mode, we're not displaying the full |user_text_|, so
-    // the cursor position we got from the view has to be adjusted later by the
-    // length of the undisplayed text.  If we're just entering keyword mode,
-    // though, we have to avoid making this adjustment, because we haven't
-    // actually hidden any text yet, but the caller has already cleared
-    // |is_keyword_hint_|, so DisplayTextFromUserText() will believe we are
-    // already in keyword mode, and will thus mis-adjust the cursor position.
+    // The text that AutocompleteInput expects is of the form

[Peter Kasting, 2016/05/18 03:32:22]

Nit: Maybe start this whole comment with "For keyword searches," or similar,
since this is only about what happens when we're doing a keyword search?

[Tom (Use chromium acct), 2016/05/18 19:15:35]

Done.

+    // "<keyword><SpaceChar><query>", where our query is |user_text_|.  So if

[Peter Kasting, 2016/05/18 03:32:22]

Nit: I'd just use an actual space in place of "<SpaceChar>" in this comment, it
seems clearer.

[Tom (Use chromium acct), 2016/05/18 19:15:35]

Done.

+    // we're in keyword mode, we need to adjust the cursor position forward by
+    // the length of "<keyword><SpaceChar>".  If we're just entering keyword
+    // mode, though, we have to avoid making this adjustment, because we haven't
+    // actually updated |user_text_| yet, but the caller has already cleared
+    // |is_keyword_hint_|, so MaybeStripKeyword() will believe we are already in

[Peter Kasting, 2016/05/18 03:32:22]

Nit: Did you mean to say MaybePrependKeyword() here?

[Tom (Use chromium acct), 2016/05/18 19:15:35]

Yes & done

+    // keyword mode, and will thus mis-adjust the cursor position.
     if (!entering_keyword_mode) {
       cursor_position +=
-          user_text_.length() - DisplayTextFromUserText(user_text_).length();
+          MaybePrependKeyword(user_text_).length() - user_text_.length();
     }
   } else {
     // There are some cases where StartAutocomplete() may be called
     // with non-empty |inline_autocomplete_text_|.  In such cases, we cannot
     // use the current selection, because it could result with the cursor
     // position past the last character from the user text.  Instead,
     // we assume that the cursor is simply at the end of input.
     // One example is when user presses Ctrl key while having a highlighted
     // inline autocomplete text.
     // TODO: Rethink how we are going to handle this case to avoid
     // inconsistent behavior when user presses Ctrl key.
     // See http://crbug.com/165961 and http://crbug.com/165968 for more details.
-    cursor_position = user_text_.length();
+    cursor_position = MaybePrependKeyword(user_text_).length();
   }
 
   GURL current_url;
   if (client_->CurrentPageExists())
     current_url = client_->GetURL();
   input_ = AutocompleteInput(
-      user_text_, cursor_position, std::string(), current_url, ClassifyPage(),
+      MaybePrependKeyword(user_text_), cursor_position, std::string(),
+      current_url, ClassifyPage(),
       prevent_inline_autocomplete || just_deleted_text_ ||
           (has_selected_text && inline_autocomplete_text_.empty()) ||
           (paste_state_ != NONE),
       is_keyword_selected(),
       is_keyword_selected() || allow_exact_keyword_match_, true, false,
       client_->GetSchemeClassifier());
 
   omnibox_controller_->StartAutocomplete(input_);
 }
 
@@ skipping 42 matching lines @@
       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
     // input instead of the currently visible text means we'll ignore any
     // visible inline autocompletion: if a user types "foo" and is autocompleted
     // to "foodnetwork.com", ctrl-enter will navigate to "foo.com", not
     // "foodnetwork.com".  At the time of writing, this behavior matches
     // Internet Explorer, but not Firefox.
     input_ = AutocompleteInput(
-        has_temporary_text_ ? UserTextFromDisplayText(view_->GetText())
-                            : input_.text(),
+        has_temporary_text_ ? view_->GetText() : input_.text(),
         input_.cursor_position(), "com", GURL(),
         input_.current_page_classification(),
         input_.prevent_inline_autocomplete(), input_.prefer_keyword(),
         input_.allow_exact_keyword_match(), input_.want_asynchronous_matches(),
         input_.from_omnibox_focus(), client_->GetSchemeClassifier());
     AutocompleteMatch url_match(
         autocomplete_controller()->history_url_provider()->SuggestExactInput(
             input_, input_.canonicalized_url(), false));
 
     if (url_match.destination_url.is_valid()) {
@@ skipping 191 matching lines @@
   if (bookmark_model && bookmark_model->IsBookmarked(match.destination_url))
     client_->OnBookmarkLaunched();
 }
 
 bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
   DCHECK(is_keyword_hint_ && !keyword_.empty());
 
   autocomplete_controller()->Stop(false);
   is_keyword_hint_ = false;
 
+  user_text_ = MaybeStripKeyword(user_text_);
+
   if (popup_model() && popup_model()->IsOpen())
     popup_model()->SetSelectedLineState(OmniboxPopupModel::KEYWORD);
   else
     StartAutocomplete(false, true, true);
 
   // When entering keyword mode via tab, the new text to show is whatever the
   // newly-selected match in the dropdown is.  When entering via space, however,
   // we should make sure to use the actual |user_text_| as the basis for the new
   // text.  This ensures that if the user types "<keyword><space>" and the
   // default match would have inline autocompleted a further string (e.g.
@@ skipping 11 matching lines @@
   // comments in ClearKeyword().
   if (entered_method == ENTERED_KEYWORD_MODE_VIA_TAB) {
     // Ensure the current selection is saved before showing keyword mode
     // so that moving to another line and then reverting the text will restore
     // the current state properly.
     bool save_original_selection = !has_temporary_text_;
     has_temporary_text_ = true;
     const AutocompleteMatch& match = CurrentMatch(NULL);
     original_url_ = match.destination_url;
     view_->OnTemporaryTextMaybeChanged(
-        DisplayTextFromUserText(match.fill_into_edit),
-        save_original_selection, true);
+        MaybeStripKeyword(match.fill_into_edit), save_original_selection,
+        true);
   } else {
-    view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(user_text_),
-                                       false, true);
+    view_->OnTemporaryTextMaybeChanged(user_text_, false, true);
   }
 
   base::RecordAction(base::UserMetricsAction("AcceptedKeywordHint"));
   UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram, entered_method,
                             ENTERED_KEYWORD_MODE_NUM_ITEMS);
 
   return true;
 }
 
 void OmniboxEditModel::AcceptTemporaryTextAsUserText() {
-  InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
+  InternalSetUserText(view_->GetText());

[Peter Kasting, 2016/05/18 03:32:22]

Don't we need to call MaybePrependKeyword() here?

Otherwise, if I tab through the dropdown into keyword mode on a non-default row,
then hit ctrl-tab + ctrl-shift-tab to change tabs away and back, I won't be in
keyword mode anymore.

[Tom (Use chromium acct), 2016/05/18 19:15:36]

InternalSetUserText will not change the keyword (that's in SetUserText).  In a
quick test, ctrl-tab + ctrl-shift-tab from keyword mode remains in keyword mode,
even on a non-default row.

   has_temporary_text_ = false;
 
   if (user_input_in_progress_ || !in_revert_)
     client_->OnInputStateChanged();
 }
 
 void OmniboxEditModel::ClearKeyword() {
   autocomplete_controller()->Stop(false);
 
   // While we're always in keyword mode upon reaching here, sometimes we've just
@@ skipping 214 matching lines @@
     GURL* destination_for_temporary_text_change,
     const base::string16& keyword,
     bool is_keyword_hint) {
   // The popup changed its data, the match in the controller is no longer valid.
   omnibox_controller_->InvalidateCurrentMatch();
 
   // Update keyword/hint-related local state.
   bool keyword_state_changed = (keyword_ != keyword) ||
       ((is_keyword_hint_ != is_keyword_hint) && !keyword.empty());
   if (keyword_state_changed) {
+    bool keyword_was_selected = is_keyword_selected();
     keyword_ = keyword;
     is_keyword_hint_ = is_keyword_hint;
+    if(!keyword_was_selected && is_keyword_selected()) {

[Peter Kasting, 2016/05/18 03:32:22]

Nit: Space after if

[Tom (Use chromium acct), 2016/05/18 19:15:35]

Done.

+      // We just entered keyword mode, so remove the keyword from the query.

[Peter Kasting, 2016/05/18 03:32:22]

Nit: query -> input

[Tom (Use chromium acct), 2016/05/18 19:15:35]

Done.

+      user_text_ = MaybeStripKeyword(user_text_);
+    }
 
     // |is_keyword_hint_| should always be false if |keyword_| is empty.
     DCHECK(!keyword_.empty() || !is_keyword_hint_);
   }
 
   // Handle changes to temporary text.
   if (destination_for_temporary_text_change != NULL) {
     const bool save_original_selection = !has_temporary_text_;
     if (save_original_selection) {
       // Save the original selection and URL so it can be reverted later.
       has_temporary_text_ = true;
       original_url_ = *destination_for_temporary_text_change;
       inline_autocomplete_text_.clear();
       view_->OnInlineAutocompleteTextCleared();
     }
     if (control_key_state_ == DOWN_WITHOUT_CHANGE) {
       // Arrowing around the popup cancels control-enter.
       control_key_state_ = DOWN_WITH_CHANGE;
       // Now things are a bit screwy: the desired_tld has changed, but if we
       // update the popup, the new order of entries won't match the old, so the
       // user's selection gets screwy; and if we don't update the popup, and the
       // user reverts, then the selected item will be as if control is still
       // pressed, even though maybe it isn't any more.  There is no obvious
       // right answer here :(
     }
-    view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(text),
+    view_->OnTemporaryTextMaybeChanged(MaybeStripKeyword(text),
                                        save_original_selection, true);
     return;
   }
 
   bool call_controller_onchanged = true;
   inline_autocomplete_text_ = text;
   if (inline_autocomplete_text_.empty())
     view_->OnInlineAutocompleteTextCleared();
 
   const base::string16& user_text =
       user_input_in_progress_ ? user_text_ : permanent_text_;
   if (keyword_state_changed && is_keyword_selected()) {
     // If we reach here, the user most likely entered keyword mode by inserting
     // a space between a keyword name and a search string (as pressing space or
     // tab after the keyword name alone would have been be handled in
     // MaybeAcceptKeywordBySpace() by calling AcceptKeyword(), which won't reach
     // here).  In this case, we don't want to call
     // OnInlineAutocompleteTextMaybeChanged() as normal, because that will
     // correctly change the text (to the search string alone) but move the caret
     // to the end of the string; instead we want the caret at the start of the
     // search string since that's where it was in the original input.  So we set
     // the text and caret position directly.
     //
     // It may also be possible to reach here if we're reverting from having
     // temporary text back to a default match that's a keyword search, but in
     // that case the RevertTemporaryText() call below will reset the caret or
     // selection correctly so the caret positioning we do here won't matter.
-    view_->SetWindowTextAndCaretPos(DisplayTextFromUserText(user_text), 0,
-                                                            false, false);
+    view_->SetWindowTextAndCaretPos(user_text, 0, false, false);
   } else if (view_->OnInlineAutocompleteTextMaybeChanged(
-             DisplayTextFromUserText(user_text + inline_autocomplete_text_),
-             DisplayTextFromUserText(user_text).length())) {
+                 user_text + inline_autocomplete_text_, user_text.length())) {
     call_controller_onchanged = false;
   }
 
   // If |has_temporary_text_| is true, then we previously had a manual selection
   // but now don't (or |destination_for_temporary_text_change| would have been
   // non-NULL). This can happen when deleting the selected item in the popup.
   // In this case, we've already reverted the popup to the default match, so we
   // need to revert ourselves as well.
   if (has_temporary_text_) {
     RevertTemporaryText(false);
@@ skipping 49 matching lines @@
   if ((text_differs || selection_differs) &&
       (control_key_state_ == DOWN_WITHOUT_CHANGE))
     control_key_state_ = DOWN_WITH_CHANGE;
 
   if (!user_text_changed)
     return false;
 
   // If the user text has not changed, we do not want to change the model's
   // state associated with the text.  Otherwise, we can get surprising behavior
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
-  InternalSetUserText(UserTextFromDisplayText(new_text));
+  InternalSetUserText(new_text);
   has_temporary_text_ = false;
 
   // Track when the user has deleted text so we won't allow inline
   // autocomplete.
   just_deleted_text_ = just_deleted_text;
 
   if (user_input_in_progress_ && user_text_.empty()) {
     // Log cases where the user started editing and then subsequently cleared
     // all the text.  Note that this explicitly doesn't catch cases like
     // "hit ctrl-l to select whole edit contents, then hit backspace", because
@@ skipping 74 matching lines @@
   user_text_ = text;
   just_deleted_text_ = false;
   inline_autocomplete_text_.clear();
   view_->OnInlineAutocompleteTextCleared();
 }
 
 void OmniboxEditModel::ClearPopupKeywordMode() const {
   omnibox_controller_->ClearPopupKeywordMode();
 }
 
-base::string16 OmniboxEditModel::DisplayTextFromUserText(
+base::string16 OmniboxEditModel::MaybeStripKeyword(
     const base::string16& text) const {
   return is_keyword_selected() ?
       KeywordProvider::SplitReplacementStringFromInput(text, false) : text;
 }
 
-base::string16 OmniboxEditModel::UserTextFromDisplayText(
+base::string16 OmniboxEditModel::MaybePrependKeyword(
     const base::string16& text) const {
   return is_keyword_selected() ? (keyword_ + base::char16(' ') + text) : text;
 }
 
 void OmniboxEditModel::GetInfoForCurrentText(AutocompleteMatch* match,
                                              GURL* alternate_nav_url) const {
   DCHECK(match != NULL);
 
   if (controller_->GetToolbarModel()->WouldPerformSearchTermReplacement(
       false)) {
@@ skipping 31 matching lines @@
           result().match_at(popup_model()->selected_line());
       *match =
           (popup_model()->selected_line_state() == OmniboxPopupModel::KEYWORD) ?
               *selected_match.associated_keyword : selected_match;
     }
     if (alternate_nav_url &&
         (!popup_model() || popup_model()->manually_selected_match().empty()))
       *alternate_nav_url = result().alternate_nav_url();
   } else {
     client_->GetAutocompleteClassifier()->Classify(
-        UserTextFromDisplayText(view_->GetText()), is_keyword_selected(), true,
+        MaybePrependKeyword(view_->GetText()), is_keyword_selected(), true,
         ClassifyPage(), match, alternate_nav_url);
   }
 }
 
 void OmniboxEditModel::RevertTemporaryText(bool revert_popup) {
   // The user typed something, then selected a different item.  Restore the
   // text they typed and change back to the default item.
   // NOTE: This purposefully does not reset paste_state_.
   just_deleted_text_ = false;
   has_temporary_text_ = false;
@@ skipping 97 matching lines @@
   // 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();
 
   client_->OnFocusChanged(focus_state_, reason);
 }