[omnibox] Break out SetCaretPos() method and enhance browser test

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 e9fd507c1c66e3999e1de6bffcfd1fd5b7fe8ed6..f43f8924e7d331edfdf90eeaa4a4038c3cce9099 100644
--- a/components/omnibox/browser/omnibox_edit_model.cc
+++ b/components/omnibox/browser/omnibox_edit_model.cc
@@ -372,9 +372,8 @@ void OmniboxEditModel::Revert() {
   has_temporary_text_ = false;
   size_t start, end;
   view_->GetSelectionBounds(&start, &end);
-  view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, false);
-  view_->SetWindowTextAndCaretPos(
-      permanent_text_, std::min(permanent_text_.length(), start), false, true);
+  view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, true);

[Peter Kasting, 2017/05/18 18:38:34]

Nit: I didn't mention this before, but this deserves a comment about why we're
setting the caret pos twice.

[Kevin Bailey, 2017/05/18 19:48:17]

Done.

+  view_->SetCaretPos(std::min(permanent_text_.length(), start));

[Peter Kasting, 2017/05/18 18:38:34]

Should we use SetCaretPos() first, then SetWindowTextAndCaretPos()?  Or will
that break the scrolling functionality we're trying to achieve?

I ask because if someone listening for change notifications reads the caret pos,
that would ensure they see the correct value.

[Kevin Bailey, 2017/05/18 19:48:17]

Ya, RenderText apparently won't update with just a cursor change. So when we
update the text, the cursor must be homed.
Recall, the notification was my concern too, but I've yet to find anything that
complains. iirc Textfield will bounce important changes back up to the
controller?

   client_->OnRevert();
 }