[omnibox] Home cursor, then restore, on revert

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 345 matching lines @@
     autocomplete_controller()->ResetSession();
   }
 
   controller_->OnInputInProgress(in_progress);
 
   if (user_input_in_progress_ || !in_revert_)
     client_->OnInputStateChanged();
 }
 
 void OmniboxEditModel::Revert() {
+  bool user_input_was_in_progress = user_input_in_progress_;
   SetInputInProgress(false);
   input_.Clear();
   paste_state_ = NONE;
   InternalSetUserText(base::string16());
   keyword_.clear();
   is_keyword_hint_ = false;
   has_temporary_text_ = false;
-  view_->SetWindowTextAndCaretPos(permanent_text_,
-                                  has_focus() ? permanent_text_.length() : 0,
-                                  false, true);
+  if (has_focus() && user_input_was_in_progress) {

[Peter Kasting, 2017/05/15 23:51:23]

Do we even need this distinction anymore?  I'm wondering if we can use the code
in the } else { in all cases.

[Kevin Bailey, 2017/05/16 01:01:01]

I don't know all the cases that Revert() is called so I'm not confident saying,
'yes'. e.g. If permanent_text_ is shorter than where the user placed the cursor,
what happens? Perhaps we can still make this unconditional, but min() the two
numbers?

Other than this concern, I tried a lot of things and couldn't break it, so
conceivably it's a good strategy.

[Peter Kasting, 2017/05/16 05:53:53]

There are several, but not so many that you couldn't look through them all, I
think.  Might be good practice :)
I'm pretty sure we need to do that even in the case where the user was editing
-- consider typing a whole bunch, then hitting escape.

[Kevin Bailey, 2017/05/17 01:53:54]

I said 'cases', not 'places'. :) I looked through all the RevertAll() calls;
they look reasonable.
Done.

+    view_->SetWindowTextAndCaretPos(permanent_text_, permanent_text_.length(),
+                                    false, true);
+  } else {
+    size_t start, end;
+    view_->GetSelectionBounds(&start, &end);
+    view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, true);
+    view_->SetWindowTextAndCaretPos(permanent_text_, start, false, true);

[Peter Kasting, 2017/05/16 05:53:53]

Even if you don't plumb full facilities for updating the scrolling separately
from the caret, it might be worth plumbing a better way to muck with the caret. 
The first call needs to update the text too, but the second doesn't, nor does it
need to do all the associated notifications.

[Kevin Bailey, 2017/05/17 01:53:54]

Ya, exposing SetCursorPosition() wouldn't be too hard. Separate CL ok?
I removed the first notification.

[Peter Kasting, 2017/05/17 02:44:14]

Sure.
What I meant was, when making two SetWindowTextAndCaretPos() calls, the first
call may update the text and thus needs to make underlying notifications, but
the second call won't.

[Kevin Bailey, 2017/05/17 03:57:52]

I was thinking we wanted to notify when the cursor was correct, and not before.
I don't see where it compares the old and new text; it appears to
unconditionally notify (as long as the parameter is true.) So this is good, ya?

[Peter Kasting, 2017/05/17 03:59:21]

Correctnesswise it's fine.  My concern is that it does excessive work, and the
fix would presumably be the move-cursor-only API.

+  }
   client_->OnRevert();
 }
 
 void OmniboxEditModel::StartAutocomplete(bool has_selected_text,
                                          bool prevent_inline_autocomplete) {
   const base::string16 input_text = MaybePrependKeyword(user_text_);
 
   size_t start, cursor_position;
   view_->GetSelectionBounds(&start, &cursor_position);
 
@@ skipping 1000 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);
 }