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..3c896f78e2df17238604bef0549d6aefacd2e9b8 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_);
     if (user_text.empty()) {
       base::AutoReset<bool> tmp(&in_revert_, true);
       view_->RevertAll();
@@ -242,12 +242,10 @@ 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);
+        state->user_text, false);

[Peter Kasting, 2016/05/07 01:42:39]

Nit: Rewrap

[Tom (Use chromium acct), 2016/05/09 18:53:48]

Done.

     view_->SetGrayTextAutocompletion(state->gray_text);
   }
 }
@@ -337,9 +335,12 @@ void OmniboxEditModel::OnChanged() {
   const AutocompleteMatch& current_match = user_input_in_progress_ ?
       CurrentMatch(NULL) : AutocompleteMatch();
 
-  client_->OnTextChanged(current_match, user_input_in_progress_, user_text_,
-                         result(), popup_model() && popup_model()->IsOpen(),
-                         has_focus());
+  base::string16 temp(MaybePrependKeyword(user_text_));
+
+  client_->OnTextChanged(current_match, user_input_in_progress_,
+                         temp, result(),
+                         popup_model() && popup_model()->IsOpen(), has_focus());
+  user_text_ = MaybeStripKeyword(temp);

[Peter Kasting, 2016/05/07 01:42:39]

(1) Why is this line necessary?  If the call above sets |user_text_| in an
undesirable way, at least explain that here (if not fix the call)

(2) If this is necessary, why not just save off the |user_text_| instead of
possibly-stripping a possibly-added keyword?

[Tom (Use chromium acct), 2016/05/09 18:53:48]

I wasn't 100% sure if the behavior of OmniboxClient::OnTextChanged relied on the
user_text having an encoded keyword.  Reverted this; not seeing any bad behavior
so far.

   controller_->OnChanged();
 }
 
@@ -489,11 +490,11 @@ 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();
+          user_text_.length() - user_text_.length();

[Peter Kasting, 2016/05/07 01:42:39]

Nit: Rewrap

[Tom (Use chromium acct), 2016/05/09 18:53:48]

Done.

     }
   } else {
     // There are some cases where StartAutocomplete() may be called
@@ -506,14 +507,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 +578,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(),
         input_.cursor_position(), "com", GURL(),
         input_.current_page_classification(),
         input_.prevent_inline_autocomplete(), input_.prefer_keyword(),
@@ -789,6 +790,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 +823,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 +837,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_)
@@ -1072,6 +1074,7 @@ void OmniboxEditModel::OnPopupDataChanged(
   if (keyword_state_changed) {
     keyword_ = keyword;
     is_keyword_hint_ = is_keyword_hint;
+    user_text_ = MaybeStripKeyword(user_text_);

[Peter Kasting, 2016/05/07 01:42:39]

When will this be needed?  I'd think AcceptKeyword() would do this if we had
changed into keyword mode.

[Tom (Use chromium acct), 2016/05/09 18:53:48]

CreatedKeywordSearchByInsertingSpaceInMiddle.  I noticed OnAfterPossibleChange
doesn't update is_keyword_hint_ so I didn't want to put the MaybeStripKeyword
there.  I've moved this and added setting the keyword state to
OnAfterPossibleChange and all seems to work well.

 
     // |is_keyword_hint_| should always be false if |keyword_| is empty.
     DCHECK(!keyword_.empty() || !is_keyword_hint_);
@@ -1097,7 +1100,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 +1128,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 +1200,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
@@ -1294,13 +1295,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 +1353,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);
   }
 }