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..79961ca373c5cedd2a9dae26853cae9529a92f69 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,13 +939,26 @@ 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);
+    StartAutocomplete(false, false, false);

[Tom (Use chromium acct), 2016/05/18 23:58:36]

If I remove StartAutocomplete here, I get bad behavior when doing the following:

1. press ctrl-K to enter keyword mode
2. press backspace
3. observe that the autocomplete popup is still present

Maybe there's a check for the difference of user_text_ that's not taking the
keyword_ difference into account? (OnAfterPossibleChange?)

[Peter Kasting, 2016/05/21 00:01:01]

Maybe the right fix here is that the popup should not appear after step 1.

It didn't use to when typing a '?', but maybe that's because we ran autocomplete
but the input was parsed as INVALID?  Whereas now the input is effectively
"google.com ".

So maybe we should modify EnterKeywordModeForDefaultSearchProvider() so that it
runs autocomplete only if the popup is already open (to update it)?  Or is there
an entry method that can happen when the popup is currently closed that should
result in the popup opening?

[Tom (Use chromium acct), 2016/05/27 22:12:07]

Well, we would want the popup to remain open if the user already has text and
then types ctrl-k.  Also, the popup can be open anyway if the user enters text
and then deletes it.  They will be left with a "<Type a search term>" popup.

I changed this to UpdatePopup() instead.  LMK if there's a better [way to do
this]|[place to put this].  For your reference, the stack trace I get when
clearing the popup by deleting all text in the omnibox is:

#0 0x7f456cb5f83e base::debug::StackTrace::StackTrace()
#1 0x7f4570156be5 OmniboxPopupContentsView::UpdatePopupAppearance()
#2 0x7f45706907d6 OmniboxPopupModel::OnResultChanged()
#3 0x7f457067a39e OmniboxController::OnResultChanged()
#4 0x7f4570627286 AutocompleteController::NotifyChanged()
#5 0x7f4570625951 AutocompleteController::UpdateResult()
#6 0x7f4570624dfb AutocompleteController::Start()
#7 0x7f457067a233 OmniboxController::StartAutocomplete()
#8 0x7f457067d7f6 OmniboxEditModel::StartAutocomplete()
#9 0x7f457015fb22 OmniboxViewViews::UpdatePopup()
#10 0x7f4570680bd0 OmniboxEditModel::ClearKeyword()
#11 0x7f4570161e14 OmniboxViewViews::HandleKeyEvent()
#12 0x7f456209db08 views::Textfield::OnKeyPressed()

[Peter Kasting, 2016/05/27 22:24:04]

Right, that's why I said "runs autocomplete only if the popup is already open".
Doesn't that have the exact same effects as your previous code?  The only real
difference is that it sets the input in progress bit, which should always
already be set.

Let's back up here.  The issue you're having is that you hit ctrl-K, it opens a
"type to search" popup, you hit delete, and the popup contents don't change at
all?  If so then OnAfterPossibleChange() is not catching the keyword mode change
and should be.  It seems to catch other exit-from-keyword-mode changes, so I'd
want to understand why it doesn't catch this one.

[Tom (Use chromium acct), 2016/05/27 22:42:47]

I tested this out already.  Even if we do this, we can still type text, which
will open the popup.  If we then delete text, the popup will be open, but the
omnibox empty.  When we press backspace to clear the keyword, the popup will
still be open.
That's correct.  Ah, now I see it.  Line 1240.  The early return is skipping
UpdatePopup

   }
 }
 
@@ -1159,6 +1197,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
@@ -1198,7 +1237,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
@@ -1233,26 +1272,40 @@ 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()) {
+    // 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] == '?') {
+      view_->SetUserText(user_text_.substr(1));
+      EnterKeywordModeForDefaultSearchProvider(
+          KeywordModeEntryMethod::QUESTION_MARK);

[Tom (Use chromium acct), 2016/05/18 23:58:36]

If I type "abc" in the omnibox, move the cursor to the beginning and press '?',
we correctly enter keyword mode and the user_text_ is "abc", but the cursor is
at the end of the text.  Should it be at the beginning instead?

[Peter Kasting, 2016/05/21 00:01:01]

Yes.

[Tom (Use chromium acct), 2016/05/27 22:12:07]

Done.

+      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_));
+  }
+  return true;
 }
 
 // TODO(beaudoin): Merge OnPopupDataChanged with this method once the popup
@@ -1380,7 +1433,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(