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

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/omnibox/browser/omnibox_edit_model.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 53 matching lines @@
 const char kOmniboxUserTextClearedHistogram[] = "Omnibox.UserTextCleared";
 
 enum UserTextClearedType {
   OMNIBOX_USER_TEXT_CLEARED_BY_EDITING = 0,
   OMNIBOX_USER_TEXT_CLEARED_WITH_ESCAPE = 1,
   OMNIBOX_USER_TEXT_CLEARED_NUM_OF_ITEMS,
 };
 
 // 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
 // between focusing and editing the omnibox.
 const char kFocusToEditTimeHistogram[] = "Omnibox.FocusToEditTime";
 
 // Histogram name which counts the number of milliseconds a user takes
 // between focusing and opening an omnibox match.
 const char kFocusToOpenTimeHistogram[] = "Omnibox.FocusToOpenTimeAnyPopupState";
 
@@ skipping 57 matching lines @@
 }  // namespace
 
 
 // OmniboxEditModel::State ----------------------------------------------------
 
 OmniboxEditModel::State::State(bool user_input_in_progress,
                                const base::string16& user_text,
                                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,
                                const AutocompleteInput& autocomplete_input)
     : user_input_in_progress(user_input_in_progress),
       user_text(user_text),
       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),
       autocomplete_input(autocomplete_input) {
 }
 
 OmniboxEditModel::State::State(const State& other) = default;
 
 OmniboxEditModel::State::~State() {
 }
 
 
 // OmniboxEditModel -----------------------------------------------------------
 
 OmniboxEditModel::OmniboxEditModel(OmniboxView* view,
                                    OmniboxEditController* controller,
                                    scoped_ptr<OmniboxClient> client)
     : client_(std::move(client)),
       view_(view),
       controller_(controller),
       focus_state_(OMNIBOX_FOCUS_NONE),
       focus_source_(INVALID),
       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),
       is_keyword_hint_(false),
       in_revert_(false),
       allow_exact_keyword_match_(false) {
   omnibox_controller_.reset(new OmniboxController(this, client_.get()));
 }
 
 OmniboxEditModel::~OmniboxEditModel() {
@@ skipping 14 matching lines @@
       view_->SelectAll(true);
     } else {
       InternalSetUserText(user_text);
     }
   }
 
   UMA_HISTOGRAM_BOOLEAN("Omnibox.SaveStateForTabSwitch.UserInputInProgress",
                         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_);
 }
 
 void OmniboxEditModel::RestoreState(const State* state) {
   // We need to update the permanent text correctly and revert the view
   // regardless of whether there is saved state.
   bool url_replacement_enabled = !state || state->url_replacement_enabled;
   controller_->GetToolbarModel()->set_url_replacement_enabled(
       url_replacement_enabled);
   permanent_text_ = controller_->GetToolbarModel()->GetText();
   // Don't muck with the search term replacement state, as we've just set it
   // correctly.
   view_->RevertWithoutResettingSearchTermReplacement();
   // Restore the autocomplete controller's input, or clear it if this is a new
   // tab.
   input_ = state ? state->autocomplete_input : AutocompleteInput();
   if (!state)
     return;
 
   SetFocusState(state->focus_state, OMNIBOX_FOCUS_CHANGE_TAB_SWITCH);
   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;
+    keyword_mode_entry_method_ = state->keyword_mode_entry_method;
     view_->SetUserText(state->user_text,
         DisplayTextFromUserText(state->user_text), false);
     view_->SetGrayTextAutocompletion(state->gray_text);
   }
 }
 
 AutocompleteMatch OmniboxEditModel::CurrentMatch(
     GURL* alternate_nav_url) const {
   // If we have a valid match use it. Otherwise get one for the current text.
   AutocompleteMatch match = omnibox_controller_->current_match();
@@ skipping 62 matching lines @@
 
   const base::string16 final_text = view_->GetText() + suggestion;
   view_->OnBeforePossibleChange();
   view_->SetWindowTextAndCaretPos(final_text, final_text.length(), false,
       false);
   view_->OnAfterPossibleChange(true);
   return true;
 }
 
 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());
 
   // Don't call CurrentMatch() when there's no editing, as in this case we'll
   // never actually use it.  This avoids running the autocomplete providers (and
   // any systems they then spin up) during startup.
   const AutocompleteMatch& current_match = user_input_in_progress_ ?
       CurrentMatch(NULL) : AutocompleteMatch();
 
   client_->OnTextChanged(current_match, user_input_in_progress_, user_text_,
@@ skipping 277 matching lines @@
     match.transition = ui::PAGE_TRANSITION_LINK;
   }
 
   client_->OnInputAccepted(match);
 
   DCHECK(popup_model());
   view_->OpenMatch(match, disposition, alternate_nav_url, base::string16(),
                    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,
                                  const base::string16& pasted_text,
                                  size_t index) {
   const base::TimeTicks& now(base::TimeTicks::Now());
   base::TimeDelta elapsed_time_since_user_first_modified_omnibox(
       now - time_user_first_modified_omnibox_);
   autocomplete_controller()->UpdateMatchDestinationURLWithQueryFormulationTime(
       elapsed_time_since_user_first_modified_omnibox, &match);
@@ skipping 138 matching lines @@
             match.transition | ui::PAGE_TRANSITION_FROM_ADDRESS_BAR));
     if (observer && observer->HasSeenPendingLoad())
       ignore_result(observer.release());  // The observer will delete itself.
   }
 
   BookmarkModel* bookmark_model = client_->GetBookmarkModel();
   if (bookmark_model && bookmark_model->IsBookmarked(match.destination_url))
     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);
   else
     StartAutocomplete(false, true, true);
 
   // When entering keyword mode via tab, the new text to show is whatever the
   // newly-selected match in the dropdown is.  When entering via space, however,
   // we should make sure to use the actual |user_text_| as the basis for the new
   // text.  This ensures that if the user types "<keyword><space>" and the
   // default match would have inline autocompleted a further string (e.g.
   // because there's a past multi-word search beginning with this keyword), the
   // inline autocompletion doesn't get filled in as the keyword search query
   // text.
   //
   // We also treat tabbing into keyword mode like tabbing through the popup in
   // that we set |has_temporary_text_|, whereas pressing space is treated like
   // a new keystroke that changes the current match instead of overlaying it
   // with a temporary one.  This is important because rerunning autocomplete
   // after the user pressed space, which will have happened just before reaching
   // 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.
     bool save_original_selection = !has_temporary_text_;
     has_temporary_text_ = true;
     const AutocompleteMatch& match = CurrentMatch(NULL);
     original_url_ = match.destination_url;
     view_->OnTemporaryTextMaybeChanged(
         DisplayTextFromUserText(match.fill_into_edit),
         save_original_selection, true);
   } else {
     view_->OnTemporaryTextMaybeChanged(DisplayTextFromUserText(user_text_),
                                        false, true);
   }
 
   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;
 }
 
 void OmniboxEditModel::AcceptTemporaryTextAsUserText() {
   InternalSetUserText(UserTextFromDisplayText(view_->GetText()));
   has_temporary_text_ = false;
 
   if (user_input_in_progress_ || !in_revert_)
     client_->OnInputStateChanged();
 }
 
 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
   // (usually because the user has typed a search string).  Keep track of the
   // difference, as we'll need it below.
   bool was_toggled_into_keyword_mode =
     popup_model()->selected_line_state() == OmniboxPopupModel::KEYWORD;
 
   omnibox_controller_->ClearPopupKeywordMode();
 
@@ skipping 49 matching lines @@
     // without re-enabling keyword mode.  The common case is state 3, where
     // the user entered keyword mode unintentionally, so backspacing
     // immediately out of keyword mode should keep the space.  In states 1 and
     // 2, having the space is "safer" behavior.  For instance, if the user types
     // "google.com f" or "google.com<tab>f" in the omnibox, moves the cursor to
     // the left, and presses backspace to leave keyword mode (state 1), it's
     // better to have the space because it may be what the user wanted.  The
     // 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);
   }
 }
 
 void OmniboxEditModel::OnSetFocus(bool control_down) {
   last_omnibox_focus_ = base::TimeTicks::Now();
   user_input_since_focus_ = false;
 
   // If the omnibox lost focus while the caret was hidden and then regained
   // focus, OnSetFocus() is called and should restore visibility. Note that
   // focus can be regained without an accompanying call to
@@ skipping 215 matching lines @@
   if (call_controller_onchanged)
     OnChanged();
 }
 
 bool OmniboxEditModel::OnAfterPossibleChange(const base::string16& old_text,
                                              const base::string16& new_text,
                                              size_t selection_start,
                                              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
   // that replaced all the text, preserve that information; otherwise, if we've
   // made some other edit, clear paste tracking.
   if (paste_state_ == PASTING)
     paste_state_ = PASTED;
   else if (text_differs)
     paste_state_ = NONE;
 
@@ skipping 19 matching lines @@
   const bool user_text_changed =
       text_differs || (selection_differs && !inline_autocomplete_text_.empty());
 
   // If something has changed while the control key is down, prevent
   // "ctrl-enter" until the control key is released.
   if ((text_differs || selection_differs) &&
       (control_key_state_ == DOWN_WITHOUT_CHANGE))
     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
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
   InternalSetUserText(UserTextFromDisplayText(new_text));
   has_temporary_text_ = false;
 
   // Track when the user has deleted text so we won't allow inline
   // autocomplete.
   just_deleted_text_ = just_deleted_text;
@@ skipping 15 matching lines @@
   // |allow_exact_keyword_match_| will be used by StartAutocomplete() method,
   // which will be called by |view_->UpdatePopup()|; so after that returns we
   // can safely reset this flag.
   allow_exact_keyword_match_ = text_differs && allow_keyword_ui_change &&
       !just_deleted_text && no_selection &&
       CreatedKeywordSearchByInsertingSpaceInMiddle(old_text, user_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);
+                              METHOD_SPACE_IN_MIDDLE,
+                              KEYWORD_MODE_ENTRY_METHOD_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.
@@ skipping 124 matching lines @@
   view_->OnRevertTemporaryText();
 }
 
 bool OmniboxEditModel::MaybeAcceptKeywordBySpace(
     const base::string16& new_text) {
   size_t keyword_length = new_text.length() - 1;
   return (paste_state_ == NONE) && is_keyword_hint_ &&
       (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(
     const base::string16& old_text,
     const base::string16& new_text,
     size_t caret_position) const {
   DCHECK_GE(new_text.length(), caret_position);
 
   // Check simple conditions first.
   if ((paste_state_ != NONE) || (caret_position < 2) ||
@@ skipping 73 matching lines @@
   // Update state and notify view if the omnibox has focus and the caret
   // visibility changed.
   const bool was_caret_visible = is_caret_visible();
   focus_state_ = state;
   if (focus_state_ != OMNIBOX_FOCUS_NONE &&
       is_caret_visible() != was_caret_visible)
     view_->ApplyCaretVisibility();
 
   client_->OnFocusChanged(focus_state_, reason);
 }