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 131 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,
+                               EnteredKeywordModeMethod keyword_entered_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_entered_method(keyword_entered_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),
+      just_cleared_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_entered_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_entered_method_ = state->keyword_entered_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 (!just_cleared_keyword_ && !just_deleted_text_ &&
+      user_text_ == base::ASCIIToUTF16("?")) {

[Peter Kasting, 2016/04/13 02:52:16]

It doesn't seem like this handles the case of inserting a '?' before an existing
string, which it probably should?

[Tom (Use chromium acct), 2016/04/13 23:37:40]

It does not.  However, it would be a bit complex to add that logic, as we would
have to keep the user_text_ before OnChanged was called in order to determine if
a question mark was typed.  Is it worth it to add this?

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

I think so -- if you prefix some existing text with a keyword and a space, the
space will flip into keyword mode, so we should be parallel here.

I am OK with that being a separate change, as long as you're up for doing it
after this one.  Or if you think it would be reasonably easy/not add lots of
complications and bugs, you can do it in this CL.

+    user_text_ = base::string16();
+    EnterKeywordModeForDefaultSearchProvider(
+        ENTERED_KEYWORD_MODE_VIA_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(
+    EnteredKeywordModeMethod keyword_entered_method) {
+  autocomplete_controller()->Stop(false);
+
+  base::string16 query = DisplayTextFromUserText(user_text_);
+  keyword_ = client_->GetTemplateURLService()->
+      GetDefaultSearchProvider()->keyword();
+  is_keyword_hint_ = false;
+  keyword_entered_method_ = keyword_entered_method;
+  InternalSetUserText(keyword_ + base::ASCIIToUTF16(" ") + query);
+
+  view_->OnTemporaryTextMaybeChanged(user_text_, false, true);
+  StartAutocomplete(false, false, true);
+
+  UMA_HISTOGRAM_ENUMERATION(kEnteredKeywordModeHistogram,
+                            keyword_entered_method,
+                            ENTERED_KEYWORD_MODE_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 143 matching lines @@
   BookmarkModel* bookmark_model = client_->GetBookmarkModel();
   if (bookmark_model && bookmark_model->IsBookmarked(match.destination_url))
     client_->OnBookmarkLaunched();
 }
 
 bool OmniboxEditModel::AcceptKeyword(EnteredKeywordModeMethod entered_method) {
   DCHECK(is_keyword_hint_ && !keyword_.empty());
 
   autocomplete_controller()->Stop(false);
   is_keyword_hint_ = false;
+  keyword_entered_method_ = entered_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
@@ skipping 36 matching lines @@
 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);
+  just_cleared_keyword_ = true;

[Peter Kasting, 2016/04/13 02:52:16]

Nit: Use base::AutoReset here to ensure you set this false on all exit paths,
instead of doing it manually.

Although... is clearing this on exit to this function safe?  Maybe this should
persist until the next time something actually changes?  What I'm worried about
is a case where you backspace out of keyword mode and then hit something that
doesn't actually change the text -- like the home or end keys or up arrow or
something.  This shouldn't suddenly jump back into keyword mode.  Make sure that
isn't broken...

[Tom (Use chromium acct), 2016/04/13 23:37:41]

Not broken.  Added base::AutoReset for now.
On a side note, if I were to add the complex logic for OnChanged that I
described in another reply, we would not need just_cleared_keyword_, so that
could be a bonus.

 
   // 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();
 
+  // 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.

[Peter Kasting, 2016/04/13 02:52:16]

Nit: "... Instead, restore the question mark iff the user originally typed one."

[Tom (Use chromium acct), 2016/04/13 23:37:41]

Done.

+  if (keyword_entered_method_ == ENTERED_KEYWORD_MODE_VIA_KEYBOARD_SHORTCUT ||
+      keyword_entered_method_ == ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK) {
+    view_->OnBeforePossibleChange();

[Peter Kasting, 2016/04/13 02:52:16]

Nit: This function body is basically identical to the "states 1-3" block below,
and if we changed one place we'd probably want to change the other.

I suspect given the comments below that these two entry methods can't result in
us winding up in the state 4 block anyway.  Therefore maybe instead of adding
all this, we should just modify the states 1-3 block to set the right window
text.  At that point, we'd basically want to use conditionals to set a "prefix"
variable to one of (keyword + space, question mark, nothing), then call
SetWindowTextAndCaretPos(prefix + view_->GetText(), prefix.length(), false,
false);.

[Tom (Use chromium acct), 2016/04/13 23:37:40]

Done.

+    if (keyword_entered_method_ == ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK)

[Peter Kasting, 2016/04/13 02:52:16]

Nit: Needs {}

[Tom (Use chromium acct), 2016/04/13 23:37:41]

Done.

+      view_->SetWindowTextAndCaretPos(
+          base::ASCIIToUTF16("?") + view_->GetText(), 1, false, false);
+    else
+      view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false);
+    keyword_.clear();
+    is_keyword_hint_ = false;
+    view_->OnAfterPossibleChange(false);
+    just_cleared_keyword_ = false;
+    return;
+  }
+
   // There are several possible states we could have been in before the user hit
   // backspace or shift-tab to enter this function:
   // (1) was_toggled_into_keyword_mode == false, has_temporary_text_ == false
   //     The user typed a further key after being in keyword mode already, e.g.
   //     "google.com f".
   // (2) was_toggled_into_keyword_mode == false, has_temporary_text_ == true
   //     The user tabbed away from a dropdown entry in keyword mode, then tabbed
   //     back to it, e.g. "google.com f<tab><shift-tab>".
   // (3) was_toggled_into_keyword_mode == true, has_temporary_text_ == false
   //     The user had just typed space to enter keyword mode, e.g.
@@ skipping 47 matching lines @@
     // 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);
     keyword_.clear();
     is_keyword_hint_ = false;
     view_->OnAfterPossibleChange(false);
   }
+  just_cleared_keyword_ = 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
   // OmniboxView::SetFocus(), e.g. by tabbing in.
@@ skipping 214 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 255 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);
 }