Refactor OmniboxEditModel::user_text_ to not include keyword

components/omnibox/browser/omnibox_edit_model.cc

Index: components/omnibox/browser/omnibox_edit_model.cc
diff --git a/components/omnibox/browser/omnibox_edit_model.cc b/components/omnibox/browser/omnibox_edit_model.cc
index 3935a9eca3109dbd3efef64b73c8c2c23d9fc4d8..0d4de246b2bd018ebc2cd8f74bb2d0fa343146b2 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -203,7 +203,7 @@ const OmniboxEditModel::State OmniboxEditModel::GetStateForTabSwitch() {
     // 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();
@@ -242,12 +242,9 @@ void OmniboxEditModel::RestoreState(const State* state) {
   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);
   }
 }
@@ -489,12 +486,10 @@ void OmniboxEditModel::StartAutocomplete(
     // 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
@@ -506,14 +501,15 @@ void OmniboxEditModel::StartAutocomplete(
     // 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),
@@ -576,8 +572,7 @@ void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
     // "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(),
@@ -789,6 +784,8 @@ bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
   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
@@ -820,11 +817,10 @@ bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
     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"));
@@ -835,7 +831,7 @@ bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
 }
 
 void OmniboxEditModel::AcceptTemporaryTextAsUserText() {
-  InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
+  InternalSetUserText(view_->GetText());
   has_temporary_text_ = false;
 
   if (user_input_in_progress_ || !in_revert_)
@@ -1097,7 +1093,7 @@ void OmniboxEditModel::OnPopupDataChanged(
       // 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;
   }
@@ -1125,11 +1121,9 @@ void OmniboxEditModel::OnPopupDataChanged(
     // 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;
   }
 
@@ -1199,7 +1193,7 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
   // 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
@@ -1227,6 +1221,15 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
       !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,
@@ -1294,13 +1297,13 @@ 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;
 }
@@ -1352,7 +1355,7 @@ void OmniboxEditModel::GetInfoForCurrentText(AutocompleteMatch* match,
       *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);
   }
 }