Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Pro…

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 87302727e29dab8cfd900d491954909f218c6b3c..d5655a9ac7c6d3add644148baac383f04518989a 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -71,7 +71,7 @@ enum UserTextClearedType {
 
 // Histogram name which counts the number of times the user enters
 // keyword hint mode and via what method.  The possible values are listed
-// in the EnteredKeywordModeMethod enum which is defined in the .h file.
+// in the KeywordModeEntryMethod enum which is defined in the .h file.
 const char kEnteredKeywordModeHistogram[] = "Omnibox.EnteredKeywordMode";
 
 // Histogram name which counts the number of milliseconds a user takes
@@ -149,6 +149,7 @@ OmniboxEditModel::State::State(bool user_input_in_progress,
                                const base::string16& gray_text,
                                const base::string16& keyword,
                                bool is_keyword_hint,
+                               KeywordModeEntryMethod keyword_mode_entry_method,
                                bool url_replacement_enabled,
                                OmniboxFocusState focus_state,
                                FocusSource focus_source,
@@ -158,6 +159,7 @@ OmniboxEditModel::State::State(bool user_input_in_progress,
       gray_text(gray_text),
       keyword(keyword),
       is_keyword_hint(is_keyword_hint),
+      keyword_mode_entry_method(keyword_mode_entry_method),
       url_replacement_enabled(url_replacement_enabled),
       focus_state(focus_state),
       focus_source(focus_source),
@@ -183,6 +185,7 @@ OmniboxEditModel::OmniboxEditModel(OmniboxView* view,
       user_input_in_progress_(false),
       user_input_since_focus_(true),
       just_deleted_text_(false),
+      clearing_keyword_(false),
       has_temporary_text_(false),
       paste_state_(NONE),
       control_key_state_(UP),
@@ -217,7 +220,7 @@ const OmniboxEditModel::State OmniboxEditModel::GetStateForTabSwitch() {
                         user_input_in_progress_);
   return State(
       user_input_in_progress_, user_text_, view_->GetGrayTextAutocompletion(),
-      keyword_, is_keyword_hint_,
+      keyword_, is_keyword_hint_, keyword_mode_entry_method_,
       controller_->GetToolbarModel()->url_replacement_enabled(),
       focus_state_, focus_source_, input_);
 }
@@ -246,6 +249,7 @@ void OmniboxEditModel::RestoreState(const State* state) {
     // DisplayTextFromUserText(), as its result depends upon this state.
     keyword_ = state->keyword;
     is_keyword_hint_ = state->is_keyword_hint;
+    keyword_mode_entry_method_ = state->keyword_mode_entry_method;
     view_->SetUserText(state->user_text,
         DisplayTextFromUserText(state->user_text), false);
     view_->SetGrayTextAutocompletion(state->gray_text);
@@ -328,6 +332,14 @@ bool OmniboxEditModel::CommitSuggestedText() {
 }
 
 void OmniboxEditModel::OnChanged() {
+  // If the user input a "?", put them into keyword mode.
+  if (!clearing_keyword_ && !just_deleted_text_ &&
+      user_text_ == base::ASCIIToUTF16("?")) {
+    user_text_ = base::string16();
+    EnterKeywordModeForDefaultSearchProvider(
+        METHOD_QUESTION_MARK);
+  }
+
   // Hide any suggestions we might be showing.
   view_->SetGrayTextAutocompletion(base::string16());
 
@@ -625,6 +637,25 @@ void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
                    popup_model()->selected_line());
 }
 
+void OmniboxEditModel::EnterKeywordModeForDefaultSearchProvider(
+    KeywordModeEntryMethod keyword_mode_entry_method) {
+  autocomplete_controller()->Stop(false);
+
+  base::string16 query = DisplayTextFromUserText(user_text_);
+  keyword_ = client_->GetTemplateURLService()->
+      GetDefaultSearchProvider()->keyword();
+  is_keyword_hint_ = false;
+  keyword_mode_entry_method_ = keyword_mode_entry_method;
+  InternalSetUserText(keyword_ + base::ASCIIToUTF16(" ") + query);
+
+  view_->OnTemporaryTextMaybeChanged(user_text_, false, true);

[Peter Kasting, 2016/04/15 20:32:47]

Calling InternalSetUserText() + OnTemporaryTextMaybeChanged() +
StartAutocomplete() isn't the right way to implement this.  This isn't a
temporary text change; that's for when you're arrowing around the dropdown and
you want to preserve the underlying original user edits in case the user hits
"escape" to revert to them.

You want to just call view_->SetUserText() or something like it; in other words,
you want to just blow away all previous user editing, preserved state, etc.

This is probably why you had to add the InternalSetUserText() call down below
that we're both suspicious of.

[Tom (Use chromium acct), 2016/04/28 17:40:30]

Done.

+  StartAutocomplete(false, false, true);
+
+  UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
+                            keyword_mode_entry_method,
+                            KEYWORD_MODE_ENTRY_METHOD_NUM_ITEMS);
+}
+
 void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
                                  WindowOpenDisposition disposition,
                                  const GURL& alternate_nav_url,
@@ -783,11 +814,13 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
     client_->OnBookmarkLaunched();
 }
 
-bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
+bool OmniboxEditModel::AcceptKeyword(
+      KeywordModeEntryMethod keyword_mode_entry_method) {
   DCHECK(is_keyword_hint_ && !keyword_.empty());
 
   autocomplete_controller()->Stop(false);
   is_keyword_hint_ = false;
+  keyword_mode_entry_method_ = keyword_mode_entry_method;
 
   if (popup_model() && popup_model()->IsOpen())
     popup_model()->SetSelectedLineState(OmniboxPopupModel::KEYWORD);
@@ -811,7 +844,7 @@ bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
   // here, may have generated a new match, which the user won't actually see and
   // which we don't want to switch back to when existing keyword mode; see
   // comments in ClearKeyword().
-  if (entered_method == ENTERED_KEYWORD_MODE_VIA_TAB) {
+  if (keyword_mode_entry_method == METHOD_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.
@@ -828,8 +861,9 @@ bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
   }
 
   base::RecordAction(base::UserMetricsAction("AcceptedKeywordHint"));
-  UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram, entered_method,
-                            ENTERED_KEYWORD_MODE_NUM_ITEMS);
+  UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
+                            keyword_mode_entry_method,
+                            KEYWORD_MODE_ENTRY_METHOD_NUM_ITEMS);
 
   return true;
 }
@@ -844,6 +878,7 @@ void OmniboxEditModel::AcceptTemporaryTextAsUserText() {
 
 void OmniboxEditModel::ClearKeyword() {
   autocomplete_controller()->Stop(false);
+  base::AutoReset<bool> reset_cleared_keyword(&clearing_keyword_, true);
 
   // While we're always in keyword mode upon reaching here, sometimes we've just
   // toggled in via space or tab, and sometimes we're on a non-toggled line
@@ -913,13 +948,27 @@ void OmniboxEditModel::ClearKeyword() {
     // user can easily delete it.  On the other hand, if there is no space and
     // the user wants it, it's more work to add because typing the space will
     // enter keyword mode, which then the user would have to leave again.
-    const base::string16 window_text =
-        keyword_ + base::ASCIIToUTF16(" ") + view_->GetText();
-    view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length() + 1,
-        false, false);
+
+    // If we entered keyword mode in a special way like using a keyboard
+    // shortcut or typing a question mark in a blank omnibox, don't restore the
+    // keyword.  Instead, restore the question mark iff the user originally
+    // typed one.
+    base::string16 prefix;
+    if (keyword_mode_entry_method_ == METHOD_KEYBOARD_SHORTCUT)

[Peter Kasting, 2016/04/15 00:58:55]

Nit: Simpler:

    if (keyword_mode_entry_method_ == METHOD_QUESTION_MARK)
      prefix = base::ASCIIToUTF16("?");
    else if (keyword_mode_entry_method_ != METHOD_KEYBOARD_SHORTCUT)
      prefix = keyword_ + base::ASCIIToUTF16(" ");

[Tom (Use chromium acct), 2016/04/15 05:20:52]

Done.

+      prefix = base::string16();
+    else if (keyword_mode_entry_method_ == METHOD_QUESTION_MARK)
+      prefix = base::ASCIIToUTF16("?");
+    else
+      prefix = keyword_ + base::ASCIIToUTF16(" ");
+
     keyword_.clear();
     is_keyword_hint_ = false;
+    InternalSetUserText(prefix + view_->GetText());

[Peter Kasting, 2016/04/15 00:58:55]

Why does this need to be added?  Shouldn't OnAfterPossibleChange() below already
deal with this?

[Tom (Use chromium acct), 2016/04/15 05:20:52]

Without it, if I press Ctrl-K and then backspace, "google.com " is left in the
search bar, because that's what I set the text to on line 649.  However, this
seems suspicious to me also, like it's masking some other behavior.

[Tom (Use chromium acct), 2016/04/28 17:40:30]

Removed call to InternalSetUserText

+    view_->SetWindowTextAndCaretPos(prefix + view_->GetText(), prefix.length(),
+        false, false);
+
     view_->OnAfterPossibleChange(false);
+    StartAutocomplete(false, false, false);
   }
 }
 
@@ -1155,6 +1204,7 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
                                              size_t selection_end,
                                              bool selection_differs,
                                              bool text_differs,
+                                             bool keyword_differs,
                                              bool just_deleted_text,
                                              bool allow_keyword_ui_change) {
   // Update the paste state as appropriate: if we're just finishing a paste
@@ -1194,7 +1244,7 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
     control_key_state_ = DOWN_WITH_CHANGE;
 
   if (!user_text_changed)
-    return false;
+    return keyword_differs;
 
   // 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
@@ -1230,8 +1280,8 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
   view_->UpdatePopup();
   if (allow_exact_keyword_match_) {
     UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
-                              ENTERED_KEYWORD_MODE_VIA_SPACE_IN_MIDDLE,
-                              ENTERED_KEYWORD_MODE_NUM_ITEMS);
+                              METHOD_SPACE_IN_MIDDLE,
+                              KEYWORD_MODE_ENTRY_METHOD_NUM_ITEMS);
     allow_exact_keyword_match_ = false;
   }
 
@@ -1376,7 +1426,7 @@ bool OmniboxEditModel::MaybeAcceptKeywordBySpace(
       (keyword_.length() == keyword_length) &&
       IsSpaceCharForAcceptingKeyword(new_text[keyword_length]) &&
       !new_text.compare(0, keyword_length, keyword_, 0, keyword_length) &&
-      AcceptKeyword(ENTERED_KEYWORD_MODE_VIA_SPACE_AT_END);
+      AcceptKeyword(METHOD_SPACE_AT_END);
 }
 
 bool OmniboxEditModel::CreatedKeywordSearchByInsertingSpaceInMiddle(