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 27 matching lines @@
 #include "components/search_engines/template_url_service.h"
 #include "components/toolbar/toolbar_model.h"
 #include "components/url_formatter/url_fixer.h"
 #include "ui/gfx/image/image.h"
 #include "url/url_util.h"
 
 using bookmarks::BookmarkModel;
 using metrics::OmniboxEventProto;
 
 
+

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Don't add this

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

 // Helpers --------------------------------------------------------------------
 
 namespace {
 
 // Histogram name which counts the number of times that the user text is
 // cleared.  IME users are sometimes in the situation that IME was
 // unintentionally turned on and failed to input latin alphabets (ASCII
 // characters) or the opposite case.  In that case, users may delete all
 // the text and the user text gets cleared.  We'd like to measure how often
 // this scenario happens.
@@ skipping 84 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_) {
+    if (user_text_ == base::ASCIIToUTF16("?")) {

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Combine these ifs

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+      user_text_ = base::string16();
+      SetKeywordWithDefaultSearchProvider(
+          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::SetKeywordWithDefaultSearchProvider(
+    EnteredKeywordModeMethod keyword_entered_method) {
+  autocomplete_controller()->Stop(false);
+
+  keyword_ = client_->GetTemplateURLService()->
+      GetDefaultSearchProvider()->keyword();
+

[Peter Kasting, 2016/04/08 00:39:56]

Nit: I'd remove this blank line

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+  is_keyword_hint_ = false;
+  keyword_entered_method_ = keyword_entered_method;
+
+  view_->OnTemporaryTextMaybeChanged(user_text_, 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);
+  view_->OnBeforePossibleChange();

[Peter Kasting, 2016/04/08 00:39:56]

This will result in calling this in the "state 4" case below, which is bad.

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.
I also moved view_->OnAfterPossibleChange(false) so it isn't called during state
4, can you confirm this is correct?

+  just_cleared_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();
 
+  // 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.
+  const char* clear_text = nullptr;

[Peter Kasting, 2016/04/08 00:39:56]

Use a std::string in preference to char*.

Or better yet, avoid the switch and the temp by restructuring like this:

  if (keyword_mode_entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT ||
      keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK) {
    if (keyword_mode_entry_method == KeywordModeEntryMethod::QUESTION_MARK)
      view_->SetWindowTextAndCaretPos('?' + view_->GetText(), 1, false, false);
    else
      view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false);
    ...
  }

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+  switch (keyword_entered_method_) {
+    case ENTERED_KEYWORD_MODE_VIA_KEYBOARD_SHORTCUT:
+      clear_text = "";
+      break;
+    case ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK:
+      clear_text = "?";
+      break;
+    default:
+      break;
+  }
+  if (clear_text) {
+    view_->SetWindowTextAndCaretPos(
+        base::ASCIIToUTF16(clear_text) + view_->GetText(), strlen(clear_text),
+        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 27 matching lines @@
   // then hitting backspace could wind up with the default match as the "x y"
   // search, which feels bizarre.
   if (was_toggled_into_keyword_mode && has_temporary_text_) {
     // State 4 above.
     is_keyword_hint_ = true;
     const base::string16 window_text = keyword_ + view_->GetText();
     view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(),
         false, true);
   } else {
     // States 1-3 above.
     view_->OnBeforePossibleChange();

[Peter Kasting, 2016/04/08 00:39:56]

This will result in calling this twice, which is bad.

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

Done.

     // Add a space after the keyword to allow the user to continue typing
     // 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);
     keyword_.clear();
     is_keyword_hint_ = false;
-    view_->OnAfterPossibleChange(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);
 }