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

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

Index: chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm b/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
index 7d296dd7b43eac1eeaa73540e8d966c7c8375408..efa97fe3e940f8fc6adb1e5faa78024854b9b69a 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())

[Peter Kasting, 2015/05/19 21:32:55]

This line isn't in views -- are you sure it's correct?  I'm wondering when it
will actually trigger.

[tapted, 2015/05/20 00:52:22]

Yeah - this was to keep consistent with the behavior of the SetSelectedRange
call that was here previously. There's a few comments about focus stealing in
here (e.g.  OmniboxViewMac::OnTabChanged), so I thought it best to keep it. (If
it were false, text_view below would be `nil` and the last line would just no-op
in Obj-C's annoying way, but that's a bit too subtle).

I think the difference on Mac is that (for things that aren't WebContents, which
don't use Cocoa's FieldEditor), it's not possible to have text selected without
focus. i.e. selection can only be shown in a FieldEditor, and there is only one
field editor per window (which moves around with focus). Whereas on other
platforms (and inside WebContents), selection is done by RenderText.

I checked all the callers of SelectAll -- they're preceeded by a SetFocus call,
verify that |user_input_in_progress_|, or are
`OmniboxEditModel::OnEscapeKeyPressed()`, so I think focus is generally ensured
here, but on Mac any window control has an opportunity to "refuse" a focus
change away from it, so I'm not sure it can be guaranteed.

+    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() {