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_->CurrentPageExists() ? client_->GetURL() : GURL(), ClassifyPage(),

[Peter Kasting, 2016/10/25 02:46:33]

Seems like this isn't the only place that does this... wondering if GetURL()
itself should handle this case and simplify the callers.

[Tom (Use chromium acct), 2016/10/25 18:50:31]

Done.

       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();
+  switch (entry_method) {
+    case KeywordModeEntryMethod::KEYBOARD_SHORTCUT:

[Tom (Use chromium acct), 2016/10/25 02:11:08]

I don't know if it's better for EnterKeywordModeForDefaultSearchProvider to
calculate the display text and selection, or if it's better to pass these along
as arguments.  It might make the "?" trimming below more clear if they're
arguments, but I want to hear what you think

+      view_->SetUserText(
+          user_input_in_progress_ ? view_->GetText() : base::string16(), false);
+      view_->SelectAll(false);

[Peter Kasting, 2016/10/25 02:46:33]

Nit: Personally I'd write this as a call to the Model's SetUserText(), followed
by an explicit SetWindowTextAndCaretPos() call.  This is less redundant under
the hood, and it's more parallel with the code below.

[Tom (Use chromium acct), 2016/10/25 18:50:31]

The doc on SetUserText says only the view should call it (is this out of date?)
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...

Also SetWindowTextAndCaretPos has no mechanism for selecing all (unless you
meant SetUserText(); SetWindowTextAndCaretPos(); SelectAll(); ?)

+      break;
+    case KeywordModeEntryMethod::QUESTION_MARK:
+      DCHECK_EQ(base::ASCIIToUTF16("?"), view_->GetText().substr(0, 1));
+      view_->SetUserText(view_->GetText().substr(1), false);

[Peter Kasting, 2016/10/25 02:46:33]

No reason for "view_->" on this, that's just a wrapper around a call back to the
model + a call to SetWindowTextAndCaretPos() (that you'll be overriding).

[Tom (Use chromium acct), 2016/10/25 18:50:31]

Ok, done.  I've also made the changes above.  I used InternalSetUserText instead
of SetUserText because the latter clears the keyword state.

+      view_->SetWindowTextAndCaretPos(view_->GetText(), 0, false, false);

[Tom (Use chromium acct), 2016/10/25 02:11:08]

It would be useful if OmniboxView had a SetUserTextAndCaretPos, don't you think?

[Peter Kasting, 2016/10/25 02:46:33]

I don't think so, but we can revisit after responding to my other ideas here, to
see if you still think this makes sense.

+      break;
+    default:
+      NOTREACHED();
+  }
+
+  keyword_ =
+      client_->GetTemplateURLService()->GetDefaultSearchProvider()->keyword();
   is_keyword_hint_ = false;
   keyword_mode_entry_method_ = entry_method;

[Peter Kasting, 2016/10/25 02:46:33]

Nit: I would move this block above the switch.  See next comment for why.

 
-  StartAutocomplete(false, false);
+  size_t sel_start, sel_end;
+  view_->GetSelectionBounds(&sel_start, &sel_end);
+  StartAutocomplete(sel_start != sel_end, false);

[Peter Kasting, 2016/10/25 02:46:33]

The primary difference between this and just calling UdpatePopup() is that this
always allows inline autocompletion, even if the user entered keyword mode by
placing a question mark at the beginning of the text.

Is that what we want?  If I put my cursor at the start of my text and hit '?',
should I not only toggle into keyword mode but potentially get an inline
autocompletion at the end of my search query?  (Practically speaking, this would
be rare; it depends on the scores from the keyword suggestion service, I think?)

I suspect we probably want the UpdatePopup() behavior.

If so, and if you move the keyword-related block up and change to using
SetWindowTextAndCaretPos() consistently above (as my previous comments suggest),
then you can pass trus for the |update_popup| arguments of those
SetWindowTextAndCaretPos() calls and eliminate this entirely.

BTW, I don't know what should be passed for |notify_text_changed| -- I don't
recall offhand what that does and I didn't bother to look it up.  You should
look into that too.

[Tom (Use chromium acct), 2016/10/25 18:50:31]

I'd argue we want the UpdatePopup() behavior because it wouldn't disturb the
cursor position
Done.  I like this solution better.

 
   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,
@@ skipping 629 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);
 }