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 3935a9eca3109dbd3efef64b73c8c2c23d9fc4d8..9227cc5ad989487267f79ea4b6262ca7c7f19af8 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(
+        KeywordModeEntryMethod::QUESTION_MARK);
+  }
+
   // Hide any suggestions we might be showing.
   view_->SetGrayTextAutocompletion(base::string16());
 
@@ -625,6 +637,26 @@ void OmniboxEditModel::AcceptInput(WindowOpenDisposition disposition,
                    popup_model()->selected_line());
 }
 
+void OmniboxEditModel::EnterKeywordModeForDefaultSearchProvider(
+    KeywordModeEntryMethod keyword_mode_entry_method) {
+  autocomplete_controller()->Stop(false);
+
+  view_->OnBeforePossibleChange();
+
+  base::string16 query = DisplayTextFromUserText(user_text_);

[Peter Kasting, 2016/04/28 21:24:41]

Nit: Can be const

[Tom (Use chromium acct), 2016/04/29 01:10:21]

Done.

+  keyword_ = client_->GetTemplateURLService()->
+      GetDefaultSearchProvider()->keyword();
+  is_keyword_hint_ = false;
+  keyword_mode_entry_method_ = keyword_mode_entry_method;
+  view_->SetUserText(keyword_ + base::ASCIIToUTF16(" ") + query, query, true);
+
+  view_->OnAfterPossibleChange(false);
+
+  UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
+                            (int)keyword_mode_entry_method,

[Peter Kasting, 2016/04/28 21:24:41]

Nit: No C-style casts (several places)

[Tom (Use chromium acct), 2016/04/29 01:10:21]

Done.

+                            (int)KeywordModeEntryMethod::NUM_ITEMS);
+}
+
 void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
                                  WindowOpenDisposition disposition,
                                  const GURL& alternate_nav_url,
@@ -783,11 +815,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 +845,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 == 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.
@@ -828,8 +862,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,
+                            (int)keyword_mode_entry_method,
+                            (int)KeywordModeEntryMethod::NUM_ITEMS);
 
   return true;
 }
@@ -844,6 +879,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
@@ -899,6 +935,12 @@ void OmniboxEditModel::ClearKeyword() {
     const base::string16 window_text = keyword_ + view_->GetText();
     view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(),
         false, true);
+
+    // If the user has typed "goo<tab>", then backspace, they will be left with
+    // "google.com" in the omnibox with no text selected.  The user text is
+    // still "goo", so odd behavior can result if we don't sync the user text
+    // to the new display text.
+    AcceptTemporaryTextAsUserText();

[Peter Kasting, 2016/04/28 21:24:41]

I don't think this is right.

If I have an entry in mid-dropdown for amazon.com, and it has an associated
keyword, then I think if I hit tab enough times to first get to that line and
then enter keyword mode, I reach here.  If we call
AcceptTemporaryTextAsUserText() when I hit shift-tab, then I exit temporary text
mode (i.e. tabbing-through-the-dropdown mode) entirely and am no longer tabbing
through the dropdown.

What "odd behavior" were you trying to fix here?  In my testing things seem to
work OK.

[Tom (Use chromium acct), 2016/04/29 01:10:21]

To repro the odd behavior (without the call to AcceptTemporaryTextAsUserText()):
type "goo<tab>" to go into keyword mode for google.com.  Type backspace to clear
keyword mode.  Press ctrl-K.  The text used to be google.com, but is now "goo".

I was able to tab into and out of keyword mode with the patch applied.  How
exactly can I reproduce your issue?

[Peter Kasting, 2016/04/29 01:36:47]

Ensure the keyword mode you're tabbing into is not associated with the default
match, but rather with a match several rows down.  When you tab back out we
shouldn't reset the rest of the dropdown and escape should do exactly what it
would do if you had tabbed down to that row but not gone into keyword mode at
all.

   } else {
     // States 1-3 above.
     view_->OnBeforePossibleChange();
@@ -913,13 +955,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_->SetUserText(prefix + view_->GetText());
+    view_->SetCaretPos(prefix.length());

[Peter Kasting, 2016/04/28 21:24:41]

Why call these and not SetWindowTextAndCaretPos()?

[Tom (Use chromium acct), 2016/04/29 01:10:21]

If I use SetWidnowTextAndCaretPos, I get this behavior:
Press ctrl-k to go into keyword mode.  Press backspace to leave keyword mode.  I
should be left with an empty omnibox, but I'm left with "google.com".  I think
that because I use SetUserText in EnterKeywordModeforDefaultSearchProvider, I
also need to use it here to update the model

[Peter Kasting, 2016/04/29 01:36:47]

That doesn't sound like the right cause; this code doesn't know what method you
called when entering keyword mode.  I'd like to get a clearer call stack trace
through all the code in here to see precisely what's resulting in the text you
describe.

It's very possible that the solution to all this stuff is that we need to ditch
the somewhat-insane under-the-hood representation of keyword mode using a
modified user text string that gets its prefix snipped off for display to the
user, and keep those two pieces of info entirely separate.

+
     view_->OnAfterPossibleChange(false);
+    StartAutocomplete(false, true, false);

[Peter Kasting, 2016/04/28 21:24:41]

Why is this needed?  SetUserText() above should have already updated the popup.

[Tom (Use chromium acct), 2016/04/29 01:10:21]

Done.

   }
 }
 
@@ -1155,6 +1210,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 +1250,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
@@ -1229,9 +1285,10 @@ bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
                                                    selection_start);
   view_->UpdatePopup();
   if (allow_exact_keyword_match_) {
+    keyword_mode_entry_method_ = KeywordModeEntryMethod::SPACE_IN_MIDDLE;
     UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
-                              ENTERED_KEYWORD_MODE_VIA_SPACE_IN_MIDDLE,
-                              ENTERED_KEYWORD_MODE_NUM_ITEMS);
+                              (int)KeywordModeEntryMethod::SPACE_IN_MIDDLE,
+                              (int)KeywordModeEntryMethod::NUM_ITEMS);
     allow_exact_keyword_match_ = false;
   }
 
@@ -1376,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(