Fix multiple problems with omnibox text handling across focus changes.

Fix multiple problems with omnibox text handling across focus changes.
(1) RenderText was drawing unfocused selections with the non-selection
    background color, but the selected text color, leading to white-on-white
    text.  Fix by drawing unfocused selected text as unselected.
(2) OmniboxViewViews was preserving selections across focus changes using
    SaveStateToTab(), leading to problems when something about the omnibox state
    changed after the omnibox was unfocused -- a later state restoration would
    restore a selection model that no longer lined up with the rest of the
    omnibox state (e.g. the current text).  Fix by tracking selection across
    focus changes in the same way OmniboxViewWin does.
(3) On tab changes, OnTabChanged() could be followed by an OnBlur()/OnFocus()
    call if changing from a tab where the omnibox was focused to one where it
    wasn't (or vice versa).  This led to the selection state being stomped.
    Fixed by making Browser give BrowserWindow first crack at handling the tab
    change.  This makes tabbing out of the omnibox, changing tabs away and back,
    and tabbing back in correctly restore the selection even when changing
    between tabs that disagree about whether the omnibox is focused.

BUG=293258
TEST=Following steps in bug comment 0 does not result in invisible text
R=msw@chromium.org, shess@chromium.org, sky@chromium.org

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

[Peter Kasting, 2013-09-20 00:47:57]

Two notes:
(1) RenderText::ApplyCompositionAndSelectionStyles() can be called by
RenderTextWin::ItemizeLogicalText().  It's not immediately clear to me what the
impact of my change is during that call chain.  Please verify that this isn't
problematic.
(2) If you test dragging tabs around in Aura, there are some weirdnesses with
cursors suddenly appearing in the omnibox during or after dragging a tab.  I
think this is a pre-existing Aura bug (I reproduced similar problems on my Dev
release), not anything related to my changes here.

[msw, 2013-09-20 01:52:38]

LGTM with nits; thanks for the fixes.

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

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:76: const gfx::Range
selection;
Can you explain here why two selection ranges are needed, and perhaps rename
this one to something more descriptive? I suppose this affords us new
functionality where the current selection is disambiguated from the selection
saved from an OnBlur, so you can now do the following: (1) Make an omnibox
selection. (2) Focus away (3) Switch tabs (4) Switch back to the previous tab
(5) Restore omnibox focus with tab-to-focus (6) The prior selection from #1 will
be restored (where #6 wasn't previously possible).

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:196: // with this edit.
nit: What is "this edit"? Do you mean the gesture event handling?
Try to explain what's happening rather than eluding to bad things.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:222: // with this edit.
nit: ditto to comment clarity and value.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:346: // Restore saved
selection if available.
nit: consider "Restore the saved selection from OnBlur if it is valid."

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:479:
OmniboxView::SetUserText(ASCIIToUTF16("?"));
Yuck, an overload! (no action item...)

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

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:178: // selection if this
range IsValid() .
nit: remove space before period.

[Peter Kasting, 2013-09-21 00:36:46]

The comments about "tab out, change tabs and back, tab back in" inspired me to
test that case more.  I found it was sometimes broken, if the two tabs disagreed
about whether the omnibox had focus.  This in turn was because the focus change
was being processed after the omnibox state restoration, overwriting the
selection state.

I fixed this after a chat with sky.  Scott, could you review everything in
c/b/ui that is NOT views/omnibox?

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

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:76: const gfx::Range
selection;
On 2013/09/20 01:52:39, msw wrote:
> Can you explain here why two selection ranges are needed, and perhaps rename
> this one to something more descriptive? I suppose this affords us new
> functionality where the current selection is disambiguated from the selection
> saved from an OnBlur, so you can now do the following: (1) Make an omnibox
> selection. (2) Focus away (3) Switch tabs (4) Switch back to the previous tab
> (5) Restore omnibox focus with tab-to-focus (6) The prior selection from #1
will
> be restored (where #6 wasn't previously possible).

Yes.  Added a description of that case.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:196: // with this edit.
On 2013/09/20 01:52:39, msw wrote:
> nit: What is "this edit"? Do you mean the gesture event handling?
> Try to explain what's happening rather than eluding to bad things.

Done.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:222: // with this edit.
On 2013/09/20 01:52:39, msw wrote:
> nit: ditto to comment clarity and value.

Done.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:346: // Restore saved
selection if available.
On 2013/09/20 01:52:39, msw wrote:
> nit: consider "Restore the saved selection from OnBlur if it is valid."

Done.

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:479:
OmniboxView::SetUserText(ASCIIToUTF16("?"));
On 2013/09/20 01:52:39, msw wrote:
> Yuck, an overload! (no action item...)

Yeah.  I find this intensely annoying :(

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

https://codereview.chromium.org/23536075/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:178: // selection if this
range IsValid() .
On 2013/09/20 01:52:39, msw wrote:
> nit: remove space before period.

Done.

[msw, 2013-09-21 00:43:11]

It seems like those latest changes are tangential, can you land them separately?

https://codereview.chromium.org/23536075/diff/25001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/23536075/diff/25001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:201: // restoring it fight
with the "select all" action.
nit: "fights" I guess.

https://codereview.chromium.org/23536075/diff/25001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:227: // selection lest
restoring it fight with the "select all" action.  It's
nit: "fights" I guess.

[Peter Kasting, 2013-09-21 00:46:41]

On 2013/09/21 00:43:11, msw wrote:
> It seems like those latest changes are tangential, can you land them
separately?

Yes, but since they were closely involved with the OmniboxViewViews code I was
writing and its comments, it seemed OK to have them be part of a generalized
"fix focus change handling" CL.  (Technically, the RenderText change is also
isolated, so this could land as three pieces.)

I don't feel violently.  Not splitting these to a series of three
separately-landed changes is mostly laziness :/

>
https://codereview.chromium.org/23536075/diff/25001/chrome/browser/ui/views/o...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
https://codereview.chromium.org/23536075/diff/25001/chrome/browser/ui/views/o...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:201: // restoring it
fight
> with the "select all" action.
> nit: "fights" I guess.

Yeah, it was supposed to be.  Fixed locally.

[sky, 2013-09-23 15:05:46]

LGTM

[Peter Kasting, 2013-09-24 00:40:48]

New snap up.  This calls BrowserWindow from Browser instead of the other way
around, to fix some tests where we have a nontrivial Browser but a stub
BrowserWindow.  sky, do you want to glance at this again?

shess, could you look at the cocoa/ files?  I haven't made any functional
changes, but there's a TODO question I added which it would be nice to
understand before I submit.

[Scott Hess - ex-Googler, 2013-09-24 01:04:38]

On 2013/09/24 00:40:48, Peter Kasting wrote:
> shess, could you look at the cocoa/ files?  I haven't made any functional
> changes, but there's a TODO question I added which it would be nice to
> understand before I submit.

LGTM in terms of compiling and stuff, but I have no idea why things are so
convoluted there.  I think there is sometimes an unnecessary degree of
indirection in some of the Cocoa code, where it confuses things notably more
than it helps.  If you want to understand it earlier rather than later, maybe
loop in Avi or Nico.

[Peter Kasting, 2013-09-24 01:35:35]

On 2013/09/24 01:04:38, shess wrote:
> On 2013/09/24 00:40:48, Peter Kasting wrote:
> > shess, could you look at the cocoa/ files?  I haven't made any functional
> > changes, but there's a TODO question I added which it would be nice to
> > understand before I submit.
> 
> LGTM in terms of compiling and stuff, but I have no idea why things are so
> convoluted there.  I think there is sometimes an unnecessary degree of
> indirection in some of the Cocoa code, where it confuses things notably more
> than it helps.  If you want to understand it earlier rather than later, maybe
> loop in Avi or Nico.

I'm not particularly worried -- I don't even know if the core problem I'm
addressing applies to Mac, and if it does, the solution may look different
anyway.

It seems like perhaps the whole TabStripController should just go away, and
BrowserWindowCocoa should take on its jobs.  I don't know whether that makes any
real sense, so I'm not planning to file it.

[Peter Kasting, 2013-09-24 02:31:40]

Ugh.  Had to reorder stuff some more in Browser::ActiveTabChanged() to make a
find bar test pass.  I filed http://crbug.com/297385 against myself to fix this
the right way.

[sky, 2013-09-24 14:28:41]

SLGTM

[Peter Kasting, 2013-09-24 21:52:58]

Committed patchset #4 manually as r225074 (presubmit successful).