[omnibox] Select-all after revert-all, in more cases

If you look at all the times we call RevertAll(), they either also call
SelectAll(true), or wouldn't mind if we did. We wish to standardize on
this policy. It happens to fix an issue also, where a stale cursor is
produced when tabbing into the Omnibox.

Bug: 735802, 750716
Change-Id: I5aace2b656f4241f2760acadc2a156dbd5fc31f4
Reviewed-on: https://chromium-review.googlesource.com/572206Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491874}

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

@@ -248,25 +248,14 @@ void OmniboxViewMac::ResetTabState(WebContents* web_contents) {
 
 void OmniboxViewMac::Update() {
   if (model()->UpdatePermanentText()) {
-    const bool was_select_all = IsSelectAll();
-    NSTextView* text_view =
-        base::mac::ObjCCastStrict<NSTextView>([field_ currentEditor]);
-    const bool was_reversed =
-        [text_view selectionAffinity] == NSSelectionAffinityUpstream;
-
     // Restore everything to the baseline look.
     RevertAll();
 
-    // Only select all when we have focus.  If we don't have focus, selecting
-    // all is unnecessary since the selection will change on regaining focus,
-    // and can in fact cause artifacts, e.g. if the user is on the NTP and
-    // clicks a link to navigate, causing |was_select_all| to be vacuously true
-    // for the empty omnibox, and we then select all here, leading to the
-    // trailing portion of a long URL being scrolled into view.  We could try
-    // and address cases like this, but it seems better to just not muck with
-    // things when the omnibox isn't focused to begin with.
-    if (was_select_all && model()->has_focus())
-      SelectAll(was_reversed);
+    // Only select all when we have focus. It's incorrect to have the
+    // Omnibox text selected while unfocused, and we'll re-select it
+    // when focus returns.
+    if (model()->has_focus())
+      SelectAll(true);
   } else {
     // TODO(shess): This corresponds to _win and _gtk, except those
     // guard it with a test for whether the security level changed.

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -1982,18 +1982,18 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DISABLED_SelectAllStaysAfterUpdate) {
   EXPECT_EQ(url_b, omnibox_view->GetText());
   EXPECT_TRUE(omnibox_view->IsSelectAll());
 
-  // Select nothing, then switch back. Shouldn't gain a selection.
+  // Select nothing, then update. Should gain SelectAll().
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_RIGHT, 0));
   test_toolbar_model->set_text(url_a);
   omnibox_view->Update();
   EXPECT_EQ(url_a, omnibox_view->GetText());
-  EXPECT_FALSE(omnibox_view->IsSelectAll());
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
 
   // Test behavior of the "reversed" attribute of OmniboxView::SelectAll().
   test_toolbar_model->set_text(ASCIIToUTF16("AB"));
   omnibox_view->Update();
-  // Should be at the end already. Shift+Left to select "reversed".
-  EXPECT_EQ(0u, GetSelectionSize(omnibox_view));
+  // Should be at beginning. Shift+left should do nothing.
+  EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, ui::EF_SHIFT_DOWN));
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, ui::EF_SHIFT_DOWN));
   EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
@@ -2020,12 +2020,12 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DISABLED_SelectAllStaysAfterUpdate) {
   omnibox_view->Update();
   EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
 
-  // Now Shift+Right should do nothing, and Shift+Left should reduce.
-  // At the end, so Shift+Right should do nothing.
-  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_RIGHT, ui::EF_SHIFT_DOWN));
+  // We reverse select all on Update() so shift-left won't do anything.
+  // On Cocoa, shift-left will just anchor it on the left.
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, ui::EF_SHIFT_DOWN));
   EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
 
-  // And Left should reduce by one character.
-  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, ui::EF_SHIFT_DOWN));
+  // And shift-right should reduce by one character.
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_RIGHT, ui::EF_SHIFT_DOWN));
   EXPECT_EQ(1u, GetSelectionSize(omnibox_view));
 }

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

@@ -210,15 +210,6 @@ void OmniboxViewViews::Update() {
   const security_state::SecurityLevel old_security_level = security_level_;
   UpdateSecurityLevel();
   if (model()->UpdatePermanentText()) {
-    // Select all the new text if the user had all the old text selected, or if
-    // there was no previous text (for new tab page URL replacement extensions).
-    // This makes one particular case better: the user clicks in the box to
-    // change it right before the permanent URL is changed.  Since the new URL
-    // is still fully selected, the user's typing will replace the edit contents
-    // as they'd intended.
-    const bool was_select_all = IsSelectAll();
-    const bool was_reversed = GetSelectedRange().is_reversed();
-
     RevertAll();
 
     // Only select all when we have focus.  If we don't have focus, selecting
@@ -229,8 +220,8 @@ void OmniboxViewViews::Update() {
     // trailing portion of a long URL being scrolled into view.  We could try
     // and address cases like this, but it seems better to just not muck with
     // things when the omnibox isn't focused to begin with.
-    if (was_select_all && model()->has_focus())
-      SelectAll(was_reversed);
+    if (model()->has_focus())
+      SelectAll(true);
   } else if (old_security_level != security_level_) {
     EmphasizeURLComponents();
   }

components/omnibox/browser/omnibox_edit_model.cc

@@ -659,9 +659,6 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
                               SEARCH_ENGINE_MAX);
   }
 
-  // Get the current text before we call RevertAll() which will clear it.
-  base::string16 current_text = view_->GetText();
-
   if (disposition != WindowOpenDisposition::NEW_BACKGROUND_TAB) {
     base::AutoReset<bool> tmp(&in_revert_, true);
     view_->RevertAll();  // Revert the box to its unedited state.