Revise NativeWidgetWin focus hack; force focus on restore.

Revise NativeWidgetWin focus hack; force focus on restore.

Remove NativeWidgetWin::RestoreFocusOnActivate's built-in hack.
Always use kReasonFocusRestore in FocusManager::RestoreFocusedView().
Always run FocusManager::SetFocusedViewWithReason with kReasonFocusRestore.
(this clears and resets child HWND focus, needed for the hack)

Fixes initial omnibox focus on Win with --enable-views-textfield; plus:
interactive_ui_tests.exe --gtest_filter=BrowserFocusTest.ClickingMovesFocus --enable-views-textfield
views_unittests.exe --gtest_filter=BubbleDelegateTest.InitiallyFocusedView --enable-views-textfield
(test updated to have the proper expectation, removing a non-Aura Win hack)
Doesn't regress child HWND focus like the previous attempt: http://crrev.com/186235

Remove SearchTextfieldView Textfield subclass; make RequestFocus non-virtual.
TODO(followup): Polish find-bar focus behavior as needed in followups.

BUG=125976, 131660, 224591, 225963
TEST=View and web-contents focus restoration on minimize/restore; repro steps in bugs. Find bar works as expected (may need attention in followups). Tests pass with --enable-views-textfield.
R=ben@chromium.org

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

[msw, 2013-03-28 23:34:22]

Ben, please take a look at your earliest convenience; thanks!
Karen mentioned that a fix for would need to land soon for Beta merge
consideration. I can revert my earlier CL on the M27 branch if that makes more
sense (the ultimate effect of the two CLs is to resolve focus issues with and
without --enable-views-textfield, which isn't critical for M27).

[Ben Goodger (Google), 2013-03-29 15:17:06]

lgtm

[msw, 2013-03-29 16:22:20]

I'm going to revert http://crrev.com/186235 on ToT, then merge that to branch
1453 for M27 Beta.
Then I'll add a change from http://crrev.com/186235 to this CL that just removes
a unit test focus hack workaround:
http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/bubble/bubble_delega...

[msw, 2013-04-02 15:14:03]

Hey Ben, please take another look at this revised CL; thanks!

[Ben Goodger (Google), 2013-04-02 20:32:14]

https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
chrome/browser/ui/browser_focus_uitest.cc:260: // Chrome OS currently focuses
the tab content on window restoration.
why does cros differ?

[msw, 2013-04-02 20:46:55]

On 2013/04/02 20:32:14, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_focus_uitest.cc (right):
> 
>
https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_focus_uitest.cc:260: // Chrome OS currently focuses
> the tab content on window restoration.
> why does cros differ?

I'm not sure where the code resides, but I verified that this isn't impacted by
my change. I'll look for what triggers that focus change; presuming I find it
and there's not technical limitation, would you prefer the behavior to match the
other platforms and keep focus on the omnibox?

[msw, 2013-04-03 01:54:16]

It's tangential regression; please take another look.

https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
chrome/browser/ui/browser_focus_uitest.cc:260: // Chrome OS currently focuses
the tab content on window restoration.
On 2013/04/02 20:32:14, Ben Goodger (Google) wrote:
> why does cros differ?

I filed http://crbug.com/225963 and updated the comment.

[James Cook, 2013-04-03 02:19:35]

On 2013/04/03 01:54:16, msw wrote:
> It's tangential regression; please take another look.
> 
>
https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_focus_uitest.cc (right):
> 
>
https://codereview.chromium.org/13227002/diff/44001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_focus_uitest.cc:260: // Chrome OS currently focuses
> the tab content on window restoration.
> On 2013/04/02 20:32:14, Ben Goodger (Google) wrote:
> > why does cros differ?
> 
> I filed http://crbug.com/225963 and updated the comment.

The Chrome OS regression goes away with --disable-focus-controller; it's
probably related to that code being turned on by default recently (M27-ish).

[Ben Goodger (Google), 2013-04-03 17:54:32]

https://codereview.chromium.org/13227002/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/13227002/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_focus_uitest.cc:261:
ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER));
can you tackle this in a different CL then? i am confused by it here.

[msw, 2013-04-03 17:57:44]

https://codereview.chromium.org/13227002/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_focus_uitest.cc (right):

https://codereview.chromium.org/13227002/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_focus_uitest.cc:261:
ASSERT_TRUE(IsViewFocused(VIEW_ID_TAB_CONTAINER));
On 2013/04/03 17:54:32, Ben Goodger (Google) wrote:
> can you tackle this in a different CL then? i am confused by it here.

So, leave the test disabled on ChromeOS (or all platforms)?
Otherwise I'm not sure what you're asking for.

[msw, 2013-04-03 18:36:58]

I reverted changes to that file, leaving the test disabled.
Please take another look.

[msw, 2013-04-04 16:15:15]

Ping: Ben, please take another look; thanks!

[Ben Goodger (Google), 2013-04-04 16:54:22]

lgtm

[commit-bot: I haz the power, 2013-04-04 17:01:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-04 17:10:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-04 17:25:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-04 17:43:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[msw, 2013-04-04 17:50:15]

On 2013/04/04 17:43:25, I haz the power (commit-bot) wrote:
> Step "update" is always a major failure.
> Look at the try server FAQ for more details.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

How annoying, update is timing out a lot; I'll just keep trying.

[commit-bot: I haz the power, 2013-04-04 17:51:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-04 19:42:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-04 19:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-04 19:47:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-04 19:48:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-04 19:51:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-04 22:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/13227002/32002

[commit-bot: I haz the power, 2013-04-05 04:12:30]

Change committed as 192493

[Finnur, 2013-04-16 18:11:28]

https://chromiumcodereview.appspot.com/13227002/diff/32002/chrome/browser/ui/...
File chrome/browser/ui/views/find_bar_view.cc (left):

https://chromiumcodereview.appspot.com/13227002/diff/32002/chrome/browser/ui/...
chrome/browser/ui/views/find_bar_view.cc:511: SelectAll(true);
I believe this CL regresses a long standing Find in page behavior.

Repro:
1) Open Chrome (one tab open).
2) Search for 'a'.
3) Open another tab.
4) Search for 'b' in that tab.
5) Set focus to the Omnibox.
6) Switch to tab 'a'.
7) Make sure the Find box is focused *and* 'a' is highlighted (so you can
overwrite the Find query).

There used to be an interactive test covering this (FocusRestoreOnTabSwitch),
but it got disabled due to flakiness. Btw: it can be deflaked by adding a call
to:
  browser()->tab_strip_model()->ActivateTabAt(0, true);
... right before the first GetFindSelectedText() call.

[msw, 2013-04-17 07:12:52]

https://chromiumcodereview.appspot.com/13227002/diff/32002/chrome/browser/ui/...
File chrome/browser/ui/views/find_bar_view.cc (left):

https://chromiumcodereview.appspot.com/13227002/diff/32002/chrome/browser/ui/...
chrome/browser/ui/views/find_bar_view.cc:511: SelectAll(true);
On 2013/04/16 18:11:28, Finnur wrote:
> I believe this CL regresses a long standing Find in page behavior.
> 
> Repro:
> 1) Open Chrome (one tab open).
> 2) Search for 'a'.
> 3) Open another tab.
> 4) Search for 'b' in that tab.
> 5) Set focus to the Omnibox.
> 6) Switch to tab 'a'.
> 7) Make sure the Find box is focused *and* 'a' is highlighted (so you can
> overwrite the Find query).
> 
> There used to be an interactive test covering this (FocusRestoreOnTabSwitch),
> but it got disabled due to flakiness. Btw: it can be deflaked by adding a call
> to:
>   browser()->tab_strip_model()->ActivateTabAt(0, true);
> ... right before the first GetFindSelectedText() call.

Thanks, Finnur! I filed http://crbug.com/232290 and have a fix in progress.