Revert "[omnibox] Select all when we revert all"

This reverts commit 3dc51db9.

Reason for revert: For some reason, this change causes a 25% increase in GL rendering for Windows. See crbug.com/739303

Original change's description:
> [omnibox] Select all when we revert all
> 
> If you look at all the times we call RevertAll(), they either also call
> SelectAll(true), or wouldn't mind if we did. To simplify both
> RevertAll() and the calling code, we now make this call unconditionally.
> It happens to fix an issue also, where a stale cursor is produced when
> tabbing into the Omnibox.
> 
> Bug: 735802
> Change-Id: Ib70f3ffd88555ef6aa600e17f111f22442e94eb0
> Reviewed-on: https://chromium-review.googlesource.com/548985
> Commit-Queue: Kevin Bailey <krb@chromium.org>
> Reviewed-by: Trent Apted <tapted@chromium.org>
> Reviewed-by: Peter Kasting <pkasting@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#483362}

TBR=pkasting@chromium.org,tapted@chromium.org,krb@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 735802
Change-Id: I4d075e5c5a20707f74bd2d330d466f67a3241ce8
Reviewed-on: https://chromium-review.googlesource.com/563482Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484992}

chrome/browser/autocomplete/autocomplete_browsertest.cc

@@ -111,14 +111,14 @@ IN_PROC_BROWSER_TEST_F(AutocompleteBrowserTest, Basic) {
 
   EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
   EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL), omnibox_view->GetText());
-  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   omnibox_view->SetUserText(base::ASCIIToUTF16("chrome"));
   location_bar->Revert();
 
   EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
   EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL), omnibox_view->GetText());
-  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
 }
 
 // Autocomplete test is flaky on ChromeOS.
@@ -163,7 +163,7 @@ IN_PROC_BROWSER_TEST_F(AutocompleteBrowserTest, MAYBE_Autocomplete) {
     location_bar->Revert();
     EXPECT_FALSE(location_bar->GetDestinationURL().is_valid());
     EXPECT_EQ(base::UTF8ToUTF16(url::kAboutBlankURL), omnibox_view->GetText());
-    EXPECT_TRUE(omnibox_view->IsSelectAll());
+    EXPECT_FALSE(omnibox_view->IsSelectAll());
     const AutocompleteResult& result = autocomplete_controller->result();
     EXPECT_TRUE(result.empty()) << AutocompleteResultAsString(result);
   }

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

@@ -248,8 +248,25 @@ 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);
   } 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

@@ -866,18 +866,11 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, BasicTextOperations) {
 
   // Check if RevertAll() resets the text and preserves the cursor position.
   omnibox_view->RevertAll();
-  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
   EXPECT_EQ(old_text, omnibox_view->GetText());
   omnibox_view->GetSelectionBounds(&start, &end);
-#if defined(OS_MACOSX)
-  // Cocoa doesn't choose a direction until the user interacts with it,
-  // so it will have a default direction here.
-  std::swap(start, end);
-#endif
-  // When |Revert()| calls |SelectAll()|, it requests a reverse selection
-  // so that the start of a long URL is in view.
-  EXPECT_EQ(11U, start);
-  EXPECT_EQ(0U, end);
+  EXPECT_EQ(3U, start);
+  EXPECT_EQ(3U, end);
 
   // Check that reverting clamps the cursor to the bounds of the new text.
   // Move the cursor to the end.
@@ -893,11 +886,8 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, BasicTextOperations) {
   omnibox_view->RevertAll();
   // Cursor should be no further than original text.
   omnibox_view->GetSelectionBounds(&start, &end);
-#if defined(OS_MACOSX)
-  std::swap(start, end);
-#endif
   EXPECT_EQ(11U, start);
-  EXPECT_EQ(0U, end);
+  EXPECT_EQ(11U, end);
 }
 
 // Make sure the cursor position doesn't get set past the last character of
@@ -1966,13 +1956,13 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, SelectAllStaysAfterUpdate) {
   test_toolbar_model->set_text(url_a);
   omnibox_view->Update();
   EXPECT_EQ(url_a, omnibox_view->GetText());
-  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(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(2u, GetSelectionSize(omnibox_view));
+  EXPECT_EQ(0u, 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));
@@ -1999,8 +1989,6 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, SelectAllStaysAfterUpdate) {
   omnibox_view->Update();
   EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
 
-  // Force a forwards select-all because Cocoa will anyways.
-  omnibox_view->SelectAll(false);
   // 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));

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

@@ -203,10 +203,31 @@ void OmniboxViewViews::ResetTabState(content::WebContents* web_contents) {
 void OmniboxViewViews::Update() {
   const security_state::SecurityLevel old_security_level = security_level_;
   UpdateSecurityLevel();
-  if (model()->UpdatePermanentText())
+  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();
-  else if (old_security_level != security_level_)
+
+    // 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);
+  } else if (old_security_level != security_level_) {
     EmphasizeURLComponents();
+  }
 }
 
 base::string16 OmniboxViewViews::GetText() const {

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

@@ -286,7 +286,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewViewsTest, SelectAllOnTabToFocus) {
   // RevertAll after navigation to invalidate the selection range saved on blur.
   omnibox_view->RevertAll();
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
-  EXPECT_TRUE(omnibox_view->IsSelectAll());
+  EXPECT_FALSE(omnibox_view->IsSelectAll());
 
   // Pressing tab to focus the omnibox should select all text.
   while (!ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)) {

components/omnibox/browser/omnibox_edit_model.cc

@@ -149,6 +149,7 @@ const OmniboxEditModel::State OmniboxEditModel::GetStateForTabSwitch() {
     if (MaybePrependKeyword(display_text).empty()) {
       base::AutoReset<bool> tmp(&in_revert_, true);
       view_->RevertAll();
+      view_->SelectAll(true);
     } else {
       InternalSetUserText(display_text);
     }
@@ -369,8 +370,13 @@ void OmniboxEditModel::Revert() {
   keyword_.clear();
   is_keyword_hint_ = false;
   has_temporary_text_ = false;
+  size_t start, end;
+  view_->GetSelectionBounds(&start, &end);
+  // First home the cursor, so view of text is scrolled to left, then correct
+  // it. |SetCaretPos()| doesn't scroll the text, so doing that first wouldn't
+  // accomplish anything.
   view_->SetWindowTextAndCaretPos(permanent_text_, 0, false, true);
-  view_->SelectAll(true);
+  view_->SetCaretPos(std::min(permanent_text_.length(), start));
   client_->OnRevert();
 }
 
@@ -653,6 +659,9 @@ 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.
@@ -928,6 +937,7 @@ bool OmniboxEditModel::OnEscapeKeyPressed() {
   // for ease of replacement, and matches other browsers.
   bool user_input_was_in_progress = user_input_in_progress_;
   view_->RevertAll();
+  view_->SelectAll(true);
 
   // If the user was in the midst of editing, don't cancel any underlying page
   // load.  This doesn't match IE or Firefox, but seems more correct.  Note that