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()));
+    const base::string16 user_text(user_text_);

[Peter Kasting, 2016/05/09 23:15:53]

I think this change means we'll now take the first arm of the conditional when
someone switches tabs when in keyword mode with no visible text, when we didn't
before.  That doesn't seem right.

I think you probably want something like:

    const base::string16 display_text = view_->GetText();
    if (MaybePrependKeyword(display_text).empty()) {
      ...
    } else {
      InternalSetUserText(display_text);
    }

Though if we're in some sort of temporary text keyword case, I think maybe the
second arm is supposed to "accept" that keyword as a side effect.  You probably
want to test tabbing to keyword mode on a non-default dropdown match and then
changing tabs away and back and seeing if the temporary keyword was "accepted"
as the current state before and after your change.  If you do find problems
there, the change in AcceptTemporaryTextAsUserText() is probably worrisome as
well.

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Good catch.  Done.

     if (user_text.empty()) {
       base::AutoReset<bool> tmp(&in_revert_, true);
       view_->RevertAll();
       view_->SelectAll(true);
     } else {
       InternalSetUserText(user_text);
     }
   }
 
   UMA_HISTOGRAM_BOOLEAN("Omnibox.SaveStateForTabSwitch.UserInputInProgress",
@@ skipping 18 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.
     keyword_ = state->keyword;
     is_keyword_hint_ = state->is_keyword_hint;
-    view_->SetUserText(state->user_text,
-        DisplayTextFromUserText(state->user_text), false);
+    view_->SetUserText(state->user_text, state->user_text, false);

[Peter Kasting, 2016/05/09 23:15:53]

I think this was the only place that passed different values for the first two
args.

This means we should simplify SetUserText() to just take one arg.

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Done.

     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 216 matching lines @@
 
 void OmniboxEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete,
     bool entering_keyword_mode) {
   size_t cursor_position;
   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

[Peter Kasting, 2016/05/09 23:15:53]

This comment is no longer correct, as we no longer have undisplayed text in
|user_text_|.

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Done.

     // 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
+    // |is_keyword_hint_|, so MaybeStripKeyword() will believe we are
     // already in keyword mode, and will thus mis-adjust the cursor position.
-    if (!entering_keyword_mode) {
-      cursor_position +=
-          user_text_.length() - DisplayTextFromUserText(user_text_).length();
-    }
+    if (!entering_keyword_mode)
+      cursor_position += user_text_.length() - user_text_.length();

[Peter Kasting, 2016/05/09 23:15:53]

This statement now does nothing.  In general, you won't notice obvious bad
things if the cursor position here is set wrongly, so you'll need to debug/test
carefully to ensure you're making the right adjustments.

I would think you'd need something like:

      cursor_position +=
          MaybePrependKeyword(user_text_).length() - user_text_.length();

...And update the comment accordingly.

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Done.

   } 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_ ? MaybePrependKeyword(user_text_) : input_.text(),

[Peter Kasting, 2016/05/09 23:15:53]

We know is_keyword_selected() is false, so MaybePrependKeyword() here does
nothing.

However, I don't think using the |user_text_| is right.  I don't believe that
gets updated as the user tabs through the dropdown.  I would think
view_->GetText() (as we did before) would be correct instead.

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Done.

         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());
   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 241 matching lines @@
     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
     // in such cases, |user_input_in_progress| won't be true here.
     UMA_HISTOGRAM_ENUMERATION(kOmniboxUserTextClearedHistogram,
                               OMNIBOX_USER_TEXT_CLEARED_BY_EDITING,
                               OMNIBOX_USER_TEXT_CLEARED_NUM_OF_ITEMS);
   }
 
   const bool no_selection = selection_start == selection_end;
 
   // Update the popup for the change, in the process changing to keyword mode
   // if the user hit space in mid-string after a keyword.
   // |allow_exact_keyword_match_| will be used by StartAutocomplete() method,
   // which will be called by |view_->UpdatePopup()|; so after that returns we
   // can safely reset this flag.
   allow_exact_keyword_match_ = text_differs && allow_keyword_ui_change &&
       !just_deleted_text && no_selection &&
       CreatedKeywordSearchByInsertingSpaceInMiddle(old_text, user_text_,
                                                    selection_start);
+
+  if (allow_exact_keyword_match_) {
+    base::TrimWhitespace(user_text_.substr(0, selection_start - 1),
+                         base::TRIM_LEADING, &keyword_);
+    is_keyword_hint_ = false;
+    user_text_ = MaybeStripKeyword(user_text_);
+    view_->SetWindowTextAndCaretPos(user_text_, 0, false, false);
+  }

[Peter Kasting, 2016/05/09 23:15:53]

This seems really questionable.  Before your patch, what actually cleared
is_keyword_hint_, updated user_text_, etc. when we created a keyword search by
inserting a space in the middle?  Why is that code not taking effect without
this?

[Tom (Use chromium acct), 2016/05/10 04:54:50]

Here's the stack trace I get from setting a watchpoint on is_keyword_hint_.
Moved MaybeStripKeyword back where it was before.

#0  0x0000555558b0d6a8 in OmniboxEditModel::OnPopupDataChanged (
    this=0x20b8584c78e0, text="", destination_for_temporary_text_change=0x0, 
    keyword="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000", 
    is_keyword_hint=false)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1074
#1  0x0000555558b0e665 in OmniboxEditModel::OnCurrentMatchChanged (
    this=0x20b8584c78e0)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1275
#2  0x0000555558b065ad in OmniboxController::OnResultChanged (
    this=0x20b8584c7ba0, default_match_changed=true)
    at ../../components/omnibox/browser/omnibox_controller.cc:50
#3  0x0000555558ab3406 in AutocompleteController::NotifyChanged (
    this=0x20b857ee2020, notify_default_match=true)
    at ../../components/omnibox/browser/autocomplete_controller.cc:611
#4  0x0000555558ab1ad1 in AutocompleteController::UpdateResult (
    this=0x20b857ee2020, regenerate_result=false, 
    force_notify_default_match_changed=true)
    at ../../components/omnibox/browser/autocomplete_controller.cc:473
#5  0x0000555558ab0f7b in AutocompleteController::Start (this=0x20b857ee2020, 
    input=...)
    at ../../components/omnibox/browser/autocomplete_controller.cc:299
#6  0x0000555558b06463 in OmniboxController::StartAutocomplete (
    this=0x20b8584c7ba0, input=...)
---Type <return> to continue, or q <return> to quit---
    at ../../components/omnibox/browser/omnibox_controller.cc:39
#7  0x0000555558b099f4 in OmniboxEditModel::StartAutocomplete (
    this=0x20b8584c78e0, has_selected_text=false, 
    prevent_inline_autocomplete=true, entering_keyword_mode=false)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:524
#8  0x000055555856c3a2 in OmniboxViewViews::UpdatePopup (this=0x20b858791c20)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:475
#9  0x0000555558b0dee4 in OmniboxEditModel::OnAfterPossibleChange (
    this=0x20b8584c78e0, 
   
old_text="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000t\000e\000s\000t\000",

    new_text="g\000o\000o\000g\000l\000e\000.\000c\000o\000m\000
\000t\000e\000s\000t\000", selection_start=11, selection_end=11,
selection_differs=false, 
    text_differs=true, just_deleted_text=false, allow_keyword_ui_change=true)
    at ../../components/omnibox/browser/omnibox_edit_model.cc:1230
#10 0x000055555856c90e in OmniboxViewViews::OnAfterPossibleChange (
    this=0x20b858791c20, allow_keyword_ui_change=true)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:551
#11 0x000055555856e6ff in OmniboxViewViews::OnAfterUserAction (
    this=0x20b858791c20, sender=0x20b858791c38)
    at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:975
#12 0x00007fffed160785 in views::Textfield::OnAfterUserAction (
    this=0x20b858791c38) at ../../ui/views/controls/textfield/textfield.cc:1795
---Type <return> to continue, or q <return> to quit---
#13 0x00007fffed1671bb in views::Textfield::DoInsertChar (this=0x20b858791c38, 
    ch=32) at ../../ui/views/controls/textfield/textfield.cc:1650
#14 0x000055555856e3c6 in OmniboxViewViews::DoInsertChar (this=0x20b858791c20, 
    ch=32) at ../../chrome/browser/ui/views/omnibox/omnibox_view_views.cc:917
#15 0x00007fffed165daa in views::Textfield::InsertChar (this=0x20b858791c38, 
    event=...) at ../../ui/views/controls/textfield/textfield.cc:1476
#16 0x00007fffed38c723 in ui::InputMethodAuraLinux::ProcessKeyEventDone (
    this=0x20b858e173e0, event=0x7fffffffc128, filtered=true, is_handled=false)
    at ../../ui/base/ime/input_method_auralinux.cc:175

+
   view_->UpdatePopup();
   if (allow_exact_keyword_match_) {
     UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
                               ENTERED_KEYWORD_MODE_VIA_SPACE_IN_MIDDLE,
                               ENTERED_KEYWORD_MODE_NUM_ITEMS);
     allow_exact_keyword_match_ = false;
   }
 
   // Change to keyword mode if the user is now pressing space after a keyword
   // name.  Note that if this is the case, then even if there was no keyword
@@ skipping 47 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(user_text_), 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);
 }