Omnibox: Preserve display text and select all on a focus search

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 392 matching lines @@
   view_->SetWindowTextAndCaretPos(permanent_text_,
                                   has_focus() ? permanent_text_.length() : 0,
                                   false, true);
   client_->OnRevert();
 }
 
 void OmniboxEditModel::StartAutocomplete(bool has_selected_text,
                                          bool prevent_inline_autocomplete) {
   const base::string16 input_text = MaybePrependKeyword(user_text_);
 
-  // Compute the cursor position.  There are three cases:
-  //   1. The user is in the midst of typing; there is no inline autocompletion.
-  //   2. The user is in the midst of typing; there is inline autocompletion.
-  //   3. The user is not in the midst of typing, but is triggering this some
-  //      other way, e.g. hitting ctrl-K while the view is showing the permanent
-  //      or temporary text.
-  size_t cursor_position;
-  if (!has_temporary_text_ && (user_text_ == view_->GetText())) {
-    // Case 1 above.  In this case there's a meaningful current cursor position,
-    // so we read it from the view.  (Note that if there is a selected range,
-    // the "cursor position" is considered to be the selection's end.)
-    size_t start;
-    view_->GetSelectionBounds(&start, &cursor_position);
+  size_t start, cursor_position;
+  view_->GetSelectionBounds(&start, &cursor_position);
 
-    // For keyword searches, the text that AutocompleteInput expects is of the
-    // form "<keyword> <query>", where our query is |user_text_|.  So we need to
-    // adjust the cursor position forward by the length of any keyword added by
-    // MaybePrependKeyword() above.
+  // For keyword searches, the text that AutocompleteInput expects is
+  // of the form "<keyword> <query>", where our query is |user_text_|.
+  // So we need to adjust the cursor position forward by the length of
+  // any keyword added by MaybePrependKeyword() above.
+  if (is_keyword_selected())
     cursor_position += input_text.length() - user_text_.length();
-  } else {
-    // Case 2 or 3 above.  In case 2, the existing inline autocompletion will be
-    // ignored for this next autocomplete run.  The current cursor position is
-    // always effectively "the end of the input text".  (If the user changes
-    // this cursor position by arrowing, it will accept the inline
-    // autocompletion, which would put us in case 1.)  In case 3, there is no
-    // meaningful current cursor position; the correct default behavior is to
-    // simply claim the cursor is at the end of the input.
-    cursor_position = input_text.length();
-  }
 
-  GURL current_url;
-  if (client_->CurrentPageExists())
-    current_url = client_->GetURL();
   input_ = AutocompleteInput(
-      input_text, cursor_position, std::string(), current_url, ClassifyPage(),
+      input_text, cursor_position, std::string(), client_->GetURL(),
+      ClassifyPage(),
       prevent_inline_autocomplete || just_deleted_text_ ||
           (has_selected_text && inline_autocomplete_text_.empty()) ||
           (paste_state_ != NONE),
       is_keyword_selected(),
       is_keyword_selected() || allow_exact_keyword_match_, true, false,
       client_->GetSchemeClassifier());
 
   omnibox_controller_->StartAutocomplete(input_);
 }
 
@@ skipping 95 matching lines @@
 
   DCHECK(popup_model());
   view_->OpenMatch(match, disposition, alternate_nav_url, base::string16(),
                    popup_model()->selected_line());
 }
 
 void OmniboxEditModel::EnterKeywordModeForDefaultSearchProvider(
     KeywordModeEntryMethod entry_method) {
   autocomplete_controller()->Stop(false);
 
-  user_input_in_progress_ = true;
-  keyword_ = client_->GetTemplateURLService()->
-      GetDefaultSearchProvider()->keyword();
+  keyword_ =
+      client_->GetTemplateURLService()->GetDefaultSearchProvider()->keyword();
   is_keyword_hint_ = false;
   keyword_mode_entry_method_ = entry_method;
 
-  StartAutocomplete(false, false);
+  base::string16 display_text;
+  switch (entry_method) {
+    case KeywordModeEntryMethod::KEYBOARD_SHORTCUT:
+      display_text =
+          user_input_in_progress_ ? view_->GetText() : base::string16();
+      InternalSetUserText(display_text);

[Peter Kasting, 2016/10/25 20:21:27]

Nit: Could move these next lines (that the two cases have in common) down below
the switch, and then guard the SelectAll() call with a conditional.

Also, since in the QUESTION_MARK case |user_input_in_progress_| is set, we can
sort-of share even more code.  Here's a possible replacement for the whole
switch:

  base::string16 display_text =
      user_input_in_progress_ ? view_->GetText() : base::string16();
  if (entry_method == KeywordModeEntryMethod::QUESTION_MARK)
    display_text.erase(0, 1);

  InternalSetUserText(display_text);
  view_->SetWindowTextAndCaretPos(display_text, 0, true, false);
  if (entry_method == KeywordModeEntryMethod::KEYBOARD_SHORTCUT)
    view_->SelectAll(false);

[Tom (Use chromium acct), 2016/10/25 21:55:09]

Done.

+      view_->SetWindowTextAndCaretPos(display_text, 0, true, false);

[Tom (Use chromium acct), 2016/10/25 21:55:09]

I wanted to clear up one thing I noticed before I land.

Now that UpdatePopup() is called before SelectAll(), we're using 0 as the cursor
position to AutocompleteInput (before adjusting for keyword).

Is this right, or should it be len(display_text)?  Or does it not matter?

[Peter Kasting, 2016/10/25 22:06:11]

Good question.  In the case of QUESTION_MARK it should definitely be zero
(that's where the cursor really is), in the KEYBOARD_SHORTCUT case it should
probably be display_text.length().  I doubt it will make a huge difference, but
that seems more correct. :/

+      view_->SelectAll(false);
+      break;
+    case KeywordModeEntryMethod::QUESTION_MARK:
+      DCHECK_EQ(base::ASCIIToUTF16("?"), view_->GetText().substr(0, 1));
+      display_text = view_->GetText().substr(1);
+      InternalSetUserText(display_text);
+      view_->SetWindowTextAndCaretPos(display_text, 0, true, false);
+      break;
+    default:
+      NOTREACHED();
+  }
 
   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,
                                  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);
 
   base::string16 input_text(pasted_text);
   if (input_text.empty())
       input_text = user_input_in_progress_ ? user_text_ : permanent_text_;
   // Create a dummy AutocompleteInput for use in calling SuggestExactInput()
   // to create an alternate navigational match.
   AutocompleteInput alternate_input(
       input_text, base::string16::npos, std::string(),
       // Somehow we can occasionally get here with no active tab.  It's not
       // clear why this happens.
-      client_->CurrentPageExists() ? client_->GetURL() : GURL(), ClassifyPage(),
-      false, false, true, true, false, client_->GetSchemeClassifier());
+      client_->GetURL(), ClassifyPage(), false, false, true, true, false,
+      client_->GetSchemeClassifier());
   std::unique_ptr<OmniboxNavigationObserver> observer(
       client_->CreateOmniboxNavigationObserver(
           input_text, match,
           autocomplete_controller()->history_url_provider()->SuggestExactInput(
               alternate_input, alternate_nav_url, false)));
 
   base::TimeDelta elapsed_time_since_last_change_to_default_match(
       now - autocomplete_controller()->last_time_default_match_changed());
   DCHECK(match.provider);
   // These elapsed times don't really make sense for matches that come from
@@ skipping 601 matching lines @@
   }
 
   if (!state_changes.text_differs || !allow_keyword_ui_change ||
       (state_changes.just_deleted_text && no_selection) ||
       is_keyword_selected() || (paste_state_ != NONE))
     return true;
 
   // If the user input a "?" at the beginning of the text, put them into
   // keyword mode for their default search provider.
   if ((state_changes.new_sel_start == 1) && (user_text_[0] == '?')) {
-    view_->SetUserText(user_text_.substr(1));
     EnterKeywordModeForDefaultSearchProvider(
         KeywordModeEntryMethod::QUESTION_MARK);
-    // Set the caret position to 0 without changing the user text.
-    view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false);
     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.
@@ skipping 197 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);
 }