Mac: Omnibox: Retain the "SelectAll" state after a navigation.

When a navigation occurs and the omnibox is focused and fully selected,
the replacement URL should also be fully selected. OmnboxViewMac
currently does not do this.

Use the same logic from void OmniboxViewViews::Update().

This makes Mac consistent with other platforms. Adds a cross-platform
test to verify.

BUG=471635

Review URL: https://codereview.chromium.org/1144853003

Cr-Commit-Position: refs/heads/master@{#330683}

2 files changed, 111 insertions, 8 deletions

diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
index 7d296dd..efa97fe 100644
--- a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
+++ b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
@@ -219,11 +219,25 @@ void OmniboxViewMac::Update() {
     controller()->GetToolbarModel()->set_url_replacement_enabled(true);
     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();
 
-    // TODO(shess): Figure out how this case is used, to make sure
-    // we're getting the selection and popup right.
+    // 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.
@@ -343,13 +357,17 @@ void OmniboxViewMac::GetSelectionBounds(base::string16::size_type* start,
 }
 
 void OmniboxViewMac::SelectAll(bool reversed) {
-  // TODO(shess): Figure out what |reversed| implies.  The gtk version
-  // has it imply inverting the selection front to back, but I don't
-  // even know if that makes sense for Mac.
+  DCHECK(!in_coalesced_update_block_);
+  if (!model()->has_focus())
+    return;
+
+  NSTextView* text_view =
+      base::mac::ObjCCastStrict<NSTextView>([field_ currentEditor]);
+  NSSelectionAffinity affinity =
+      reversed ? NSSelectionAffinityUpstream : NSSelectionAffinityDownstream;
+  NSRange range = NSMakeRange(0, GetTextLength());
 
-  // TODO(shess): Verify that we should be stealing focus at this
-  // point.
-  SetSelectedRange(NSMakeRange(0, GetTextLength()));
+  [text_view setSelectedRange:range affinity:affinity stillSelecting:NO];
 }
 
 void OmniboxViewMac::RevertAll() {
diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
index 7744770..2083426 100644
--- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
+++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
@@ -1866,3 +1866,88 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
   omnibox_view->Update();
   EXPECT_EQ(url_c, omnibox_view->GetText());
 }
+
+namespace {
+
+// Returns the number of characters currently selected in |omnibox_view|.
+size_t GetSelectionSize(OmniboxView* omnibox_view) {
+  size_t start, end;
+  omnibox_view->GetSelectionBounds(&start, &end);
+  if (end >= start)
+    return end - start;
+  return start - end;
+}
+
+}  // namespace
+
+// Test that if the Omnibox has focus, and had everything selected before a
+// non-user-initiated update, then it retains the selection after the update.
+IN_PROC_BROWSER_TEST_F(OmniboxViewTest, SelectAllStaysAfterUpdate) {
+  OmniboxView* omnibox_view = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
+  TestToolbarModel* test_toolbar_model = new TestToolbarModel;
+  scoped_ptr<ToolbarModel> toolbar_model(test_toolbar_model);
+  browser()->swap_toolbar_models(&toolbar_model);
+
+  base::string16 url_a(ASCIIToUTF16("http://www.a.com/"));
+  base::string16 url_b(ASCIIToUTF16("http://www.b.com/"));
+  chrome::FocusLocationBar(browser());
+
+  test_toolbar_model->set_text(url_a);
+  omnibox_view->Update();
+  EXPECT_EQ(url_a, omnibox_view->GetText());
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  // Updating while selected should retain SelectAll().
+  test_toolbar_model->set_text(url_b);
+  omnibox_view->Update();
+  EXPECT_EQ(url_b, omnibox_view->GetText());
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  // Select nothing, then switch back. Shouldn't gain a selection.
+  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());
+
+  // 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));
+  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));
+  EXPECT_TRUE(omnibox_view->IsSelectAll());
+
+  test_toolbar_model->set_text(ASCIIToUTF16("CD"));
+  omnibox_view->Update();
+  EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
+
+  // At the start, so Shift+Left should do nothing.
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, ui::EF_SHIFT_DOWN));
+  EXPECT_EQ(2u, GetSelectionSize(omnibox_view));
+
+  // 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));
+
+  // No go to start and select all to the right (not reversed).
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_RIGHT, ui::EF_SHIFT_DOWN));
+  ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_RIGHT, ui::EF_SHIFT_DOWN));
+  test_toolbar_model->set_text(ASCIIToUTF16("AB"));
+  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));
+  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));
+  EXPECT_EQ(1u, GetSelectionSize(omnibox_view));
+}