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

Don't mess with omnibox selection on update when the omnibox doesn't have focus.
This fixes a regression in selection behavior when navigating from the NTP.

BUG=247933
TEST=Visit NTP, click a tile, URL should not be selected
R=msw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209839

[Peter Kasting, 2013-07-02 23:09:00]

This is the regression fix alone.

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

Please add 256789 to BUG= (that's the regression).

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:389: const bool
was_select_all = (range.length() == text().length());
nit: |was_select_all| should include the non-empty text check.
Ditto for omnibox_view_win.cc

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:394: // You might think we
could reach here if the user has deleted the previous
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

[msw, 2013-07-02 23:37:10]

On 2013/07/02 23:35:21, msw wrote:
> Please add 256789 to BUG= (that's the regression).
Doh, just saw that you changed the bug... nvm this.

[Peter Kasting, 2013-07-02 23:38:44]

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:389: const bool
was_select_all = (range.length() == text().length());
On 2013/07/02 23:35:21, msw wrote:
> nit: |was_select_all| should include the non-empty text check.
> Ditto for omnibox_view_win.cc

|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".

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:394: // You might think we
could reach here if the user has deleted the previous
On 2013/07/02 23:35:21, msw wrote:
> 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."

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]

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:389: const bool
was_select_all = (range.length() == text().length());
On 2013/07/02 23:38:45, Peter Kasting wrote:
> On 2013/07/02 23:35:21, msw wrote:
> > nit: |was_select_all| should include the non-empty text check.
> > Ditto for omnibox_view_win.cc
> 
> |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".

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.

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:394: // You might think we
could reach here if the user has deleted the previous
On 2013/07/02 23:38:45, Peter Kasting wrote:
> On 2013/07/02 23:35:21, msw wrote:
> > 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."
> 
> 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.

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?

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:401: // omnibox shouldn't
have focus anyway, and selecting all will at best do
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?

https://codereview.chromium.org/18543012/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:402: // nothing and at
worst cause problems like reversing the URL.
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.

[Peter Kasting, 2013-07-03 00:16:06]

Reworked the change to check focus, since on reflection I think you're right
that that's better than checking text emptiness.  That way, if I'm wrong and the
user _can_ have a focused omnibox with no text and no edit in progress, we'll go
ahead and select all (i.e. we'll continue to fix the case I changed the code for
to begin with).

[msw, 2013-07-03 00:30:19]

LGTM. Nice rework; please consider changing the CL title.

[Peter Kasting, 2013-07-03 01:12:32]

Committed patchset #2 manually as r209839 (presubmit successful).