Don't mess with omnibox selection on update when the omnibox doesn't have focus.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 368 matching lines @@
     // Not switching tabs, just updating the permanent text.  (In the case where
     // we _were_ switching tabs, the RevertAll() above already drew the new
     // permanent text.)
 
     // Tweak: if the user had all the text selected, select all the new text.
     // 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 ui::Range range(GetSelectedRange());
     const bool was_select_all = (range.length() == text().length());

[msw, 2013/07/02 23:35:21]

nit: |was_select_all| should include the non-empty text check.
Ditto for omnibox_view_win.cc

[Peter Kasting, 2013/07/02 23:38:45]

|was_select_all| is vacuously true when there's no text.  It's clearer to not
make it also mean "...or the text was empty".

[msw, 2013/07/03 00:04:07]

The current logic will 'make it also mean "...or the text was empty"', so your
argument seems in favor of including the emptiness check here, please explain.

 
     RevertAll();
 
-    if (was_select_all)
+    // When the old text was empty, we don't bother selecting the new text.
+    // You might think we could reach here if the user has deleted the previous

[msw, 2013/07/02 23:35:21]

nit: Just explain the reasoning plainly: "Do not select the new text when the
old text was empty. Navigating from the NTP should not trigger a URL selection."
If desired, you can add something like ", which may cause rendering jank."
Ditto for omnibox_view_win.cc

[Peter Kasting, 2013/07/02 23:38:45]

Rendering jank isn't my concern, and the rest of that comment is far too brief
to tell me anything.  I definitely would not understand the subtlety here from
reading that, which is the entire point of the comment.

[msw, 2013/07/03 00:04:07]

Fine, but there's little reason to speculate about unrelated reason why the
reader might think the empty check matters, can you explain the actual reasoning
without explaining why a hypothetical alternate reasoning is invalid?

+    // URL, but in that case the model will see that user input is in progress
+    // and return false from UpdatePermanentText() (so we won't reach here),
+    // unless we don't have focus, in which case we don't care anyway because
+    // re-focusing the omnibox will modify the selection regardless.  The only
+    // time we actually reach here with empty text is when the user was on the
+    // NTP and clicked something on the page to navigate, in which case the
+    // omnibox shouldn't have focus anyway, and selecting all will at best do

[msw, 2013/07/03 00:04:07]

The comment, "in which case the omnibox shouldn't have focus anyway" makes it
sound like the code should really check for omnibox focus, not text emptiness.
Should that be the actual condition checked here?

+    // nothing and at worst cause problems like reversing the URL.

[msw, 2013/07/03 00:04:07]

The URL would not be reversed; the problem we observed is that the
forward-selection of the URL causes the trailing URL text to be shown.

+    if (was_select_all && !range.is_empty())
       SelectAll(range.is_reversed());
   } else if (changed_security_level) {
     EmphasizeURLComponents();
   }
 }
 
 string16 OmniboxViewViews::GetText() const {
   // TODO(oshima): IME support
   return text();
 }
@@ skipping 503 matching lines @@
   const string16 text(GetClipboardText());
   if (!text.empty()) {
     // Record this paste, so we can do different behavior.
     model()->on_paste();
     // Force a Paste operation to trigger the text_changed code in
     // OnAfterPossibleChange(), even if identical contents are pasted.
     text_before_change_.clear();
     InsertOrReplaceText(text);
   }
 }