[omnibox] Home cursor, then restore, on revert

components/omnibox/browser/omnibox_edit_model.cc

Index: components/omnibox/browser/omnibox_edit_model.cc
diff --git a/components/omnibox/browser/omnibox_edit_model.cc b/components/omnibox/browser/omnibox_edit_model.cc
index 4c3a2782776335c03ee4aca6c61f16876b685e55..9b5d6abf41b298317759b5a4719fa4663524e640 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -363,6 +363,7 @@ void OmniboxEditModel::SetInputInProgress(bool in_progress) {
 }
 
 void OmniboxEditModel::Revert() {
+  bool user_input_was_in_progress = user_input_in_progress_;
   SetInputInProgress(false);
   input_.Clear();
   paste_state_ = NONE;
@@ -370,9 +371,15 @@ void OmniboxEditModel::Revert() {
   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();
 }