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 14f7ae75945a0bec933f8fdf0b8d8d584a52d729..7c5f8394bb481abe5a40204dafe8d9b732f837ac 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),
@@ -217,7 +219,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_);
 }
@@ -242,11 +244,12 @@ 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 AFTER invoking
+    // NOTE: Be sure to 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;
+    keyword_mode_entry_method_ = state->keyword_mode_entry_method;
     view_->SetGrayTextAutocompletion(state->gray_text);
   }
 }
@@ -625,6 +628,23 @@ void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
                    popup_model()->selected_line());
 }
 
+void OmniboxEditModel::EnterKeywordModeForDefaultSearchProvider(
+    KeywordModeEntryMethod entry_method) {
+  autocomplete_controller()->Stop(false);
+
+  user_input_in_progress_ = true;
+  keyword_ = client_->GetTemplateURLService()->
+      GetDefaultSearchProvider()->keyword();
+  is_keyword_hint_ = false;
+  keyword_mode_entry_method_ = entry_method;
+
+  StartAutocomplete(false, false, true);
+
+  UMA_HISTOGRAM_ENUMERATION(
+      kEnteredKeywordModeHistogram, static_cast<int>(entry_method),
+      static_cast<int>(KeywordModeEntryMethod::NUM_ITEMS));
+}
+
 void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
                                  WindowOpenDisposition disposition,
                                  const GURL& alternate_nav_url,
@@ -783,11 +803,15 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
     client_->OnBookmarkLaunched();
 }
 
-bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
+bool OmniboxEditModel::AcceptKeyword(
+      KeywordModeEntryMethod entry_method) {
   DCHECK(is_keyword_hint_ && !keyword_.empty());
 
   autocomplete_controller()->Stop(false);
+
   is_keyword_hint_ = false;
+  keyword_mode_entry_method_ = entry_method;
+  user_text_ = MaybeStripKeyword(user_text_);
 
   user_text_ = MaybeStripKeyword(user_text_);
 
@@ -813,7 +837,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 (entry_method == KeywordModeEntryMethod::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.
@@ -829,8 +853,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, static_cast<int>(entry_method),
+      static_cast<int>(KeywordModeEntryMethod::NUM_ITEMS));
 
   return true;
 }
@@ -914,12 +939,24 @@ 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_ == KeywordModeEntryMethod::QUESTION_MARK)
+      prefix = base::ASCIIToUTF16("?");
+    else if (keyword_mode_entry_method_ !=
+             KeywordModeEntryMethod::KEYBOARD_SHORTCUT)
+      prefix = keyword_ + base::ASCIIToUTF16(" ");
+
     keyword_.clear();
     is_keyword_hint_ = false;
+
+    view_->SetWindowTextAndCaretPos(prefix + view_->GetText(), prefix.length(),
+                                    false, false);
+
     view_->OnAfterPossibleChange(false);
   }
 }
@@ -1159,6 +1196,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
@@ -1197,8 +1235,18 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
       (control_key_state_ == DOWN_WITHOUT_CHANGE))
     control_key_state_ = DOWN_WITH_CHANGE;
 
-  if (!user_text_changed)
-    return false;
+  if (!user_text_changed) {
+    if (keyword_differs) {
+      // We won't need the below logic for creating a keyword by a space at the
+      // end or in the middle, or by typing a '?', but we do need to update the
+      // popup view because the keyword can change without the text changing,
+      // for example when the keyword is "youtube.com" and the user presses
+      // ctrl-k to change it to "google.com", or if the user text is empty and
+      // the user presses backspace.
+      view_->UpdatePopup();
+    }
+    return keyword_differs;

[Peter Kasting, 2016/06/01 01:07:27]

What are the consequences of returning true versus false here?  I assume what
you're doing here is correct, but the comment in the header says something like
"when a significant change occurs" which is pretty vague about what callers
would expect in a case like this.

This also informs how I react to where you return false below in the new code to
handle pressing '?'; it would be nice to be able to say precisely which state
callers think they need to update if this returns true, so that we can say that
EnterKeywordModeForDefaultSearchProvider() has updated all that state and thus
returning false is correct.

[Tom (Use chromium acct), 2016/06/02 19:24:40]

My memory is foggy since I changed the "return false" to "return
keyword_differs" like 2 months ago, but if I remember correctly, I think if we
return false, the view does not get rerendered.  So if we just returned false
here like before, if the keyword changes without the text changing, the view
will not change, and we will have to type an extra character for the view to
update and see the keyword change.

So, I guess it might be more correct to say "if a view-visible change occurs"
instead of "when a significant change occurs"?

[Peter Kasting, 2016/06/04 02:17:18]

"if the view needs to be repainted"?

+  }
 
   // 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
@@ -1233,26 +1281,42 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
                                                    selection_start);
   view_->UpdatePopup();
   if (allow_exact_keyword_match_) {
-    UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
-                              ENTERED_KEYWORD_MODE_VIA_SPACE_IN_MIDDLE,
-                              ENTERED_KEYWORD_MODE_NUM_ITEMS);
+    keyword_mode_entry_method_ = KeywordModeEntryMethod::SPACE_IN_MIDDLE;
+    UMA_HISTOGRAM_ENUMERATION(
+        kEnteredKeywordModeHistogram,
+        static_cast<int>(KeywordModeEntryMethod::SPACE_IN_MIDDLE),
+        static_cast<int>(KeywordModeEntryMethod::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
-  // hint when we entered this function (e.g. if the user has used space to
-  // replace some selected text that was adjoined to this keyword), there will
-  // be one now because of the call to UpdatePopup() above; so it's safe for
-  // MaybeAcceptKeywordBySpace() to look at |keyword_| and |is_keyword_hint_| to
-  // determine what keyword, if any, is applicable.
-  //
-  // If MaybeAcceptKeywordBySpace() accepts the keyword and returns true, that
-  // will have updated our state already, so in that case we don't also return
-  // true from this function.
-  return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
-           no_selection && (selection_start == user_text_.length()) &&
-           MaybeAcceptKeywordBySpace(user_text_));
+  if (text_differs && allow_keyword_ui_change && !just_deleted_text &&
+      no_selection && !is_keyword_selected()) {

[Peter Kasting, 2016/06/01 01:07:27]

Nit: Reverse this conditional and early-return true

[Tom (Use chromium acct), 2016/06/02 19:24:40]

Done.

+    // If the user input a "?" at the beginning of the text, put them into
+    // keyword mode for their default search provider.
+    if (selection_start == 1 && user_text_[0] == '?') {

[Peter Kasting, 2016/06/01 01:07:27]

MaybeAcceptKeywordBySpace() checks that the paste state is NONE, i.e. if we
paste in a space instead of typing it it won't trigger keyword mode.

I think we should do the same here.  Pasting a "?" in seems like it should
result in a question mark showing, not being treated as the search accelerator.

Nit: IIRC file style is to put parens around each boolean subexpr.

[Tom (Use chromium acct), 2016/06/02 19:24:40]

Done, also removed the paste state check from MaybeAcceptKeywordBySpace.  LMK if
you think that should be added back.

+      view_->SetUserText(user_text_.substr(1));
+      EnterKeywordModeForDefaultSearchProvider(
+          KeywordModeEntryMethod::QUESTION_MARK);
+      // Set the caret position to 0 without changing the user text.
+      view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false);
+      return 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
+    // hint when we entered this function (e.g. if the user has used space to
+    // replace some selected text that was adjoined to this keyword), there will
+    // be one now because of the call to UpdatePopup() above; so it's safe for
+    // MaybeAcceptKeywordBySpace() to look at |keyword_| and |is_keyword_hint_|
+    // to determine what keyword, if any, is applicable.
+    //
+    // If MaybeAcceptKeywordBySpace() accepts the keyword and returns true, that
+    // will have updated our state already, so in that case we don't also return
+    // true from this function.
+    return !(selection_start == user_text_.length() &&
+             MaybeAcceptKeywordBySpace(user_text_));

[Peter Kasting, 2016/06/01 01:07:27]

Nit: Distribute the '!' through this expression

[Tom (Use chromium acct), 2016/06/02 19:24:40]

Done.

+  }
+  return true;
 }
 
 // TODO(beaudoin): Merge OnPopupDataChanged with this method once the popup
@@ -1380,7 +1444,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(KeywordModeEntryMethod::SPACE_AT_END);
 }
 
 bool OmniboxEditModel::CreatedKeywordSearchByInsertingSpaceInMiddle(