Revert FindBarView::SearchTextfieldView deletion.

Revert FindBarView::SearchTextfieldView deletion.

My http://crrev.com/192493 caused a regression:
FindBarView doesn't select-all on tab-switch focus restoration.

Restore the FindBarView::SearchTextfieldView class with RestoreFocus override.
( this restores the previous select-all behavior to fix the regression )

BUG=232290
TEST=Switching back to a tab that had the find bar focused (from a tab that didn't have the find bar focused) restores find bar focus and selects the entire text content.
R=finnur@chromium.org
TBR=ben@chromium.org

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

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

Hey Finnur and Ben, please take a look; thanks!

I'd re-enable FindInPageTest.FocusRestoreOnTabSwitch, but I wasn't able to repro
the flaky failure in 100 local runs after simply enabling the test, so I can't
verify the suggested fix for the test, and think it's best left to address that
in a separate CL.

Also worth considering for this CL or a follow-up: Would we want to select-all
when restoring focus to the find box on Tab1 *when switching from a Tab2 that
also had its find box focused*? Apparently, if you skip step 5 (focusing the
Tab2 omnibox before switching back to Tab1) and leave focus on Tab2's find box,
we don't select-all in Tab1's find box on restore. We could do so by eliminating
the HasFocus() early return from FindBarView::SearchTextfieldView::RequestFocus.

[Finnur, 2013-04-17 09:54:26]

LGTM.

On 2013/04/17 07:52:23, msw wrote:
> Hey Finnur and Ben, please take a look; thanks!
> 
> I'd re-enable FindInPageTest.FocusRestoreOnTabSwitch, but I wasn't able to
repro
> the flaky failure in 100 local runs after simply enabling the test, so I can't
> verify the suggested fix for the test, and think it's best left to address
that
> in a separate CL.

There are two sources of flakiness:

1) When the test starts a FindInPage operation, the Find bar text field isn't
getting updated before the test checks its value. The first check to see if 'a'
is in the text field therefore fails.

2) The test web server often fails to start. This is http://crbug.com/96594,
which was gathering steam but seems to have tapered off a bit lately.

I can repro 1) and was working on a fix when I discovered the issue you are
fixing right now. 2) is an environment issue that affects a lot of tests and
other people have been looking into.

I'll keep working on 1) today.

> 
> Also worth considering for this CL or a follow-up: Would we want to select-all
> when restoring focus to the find box on Tab1 *when switching from a Tab2 that
> also had its find box focused*? Apparently, if you skip step 5 (focusing the
> Tab2 omnibox before switching back to Tab1) and leave focus on Tab2's find
box,
> we don't select-all in Tab1's find box on restore. We could do so by
eliminating
> the HasFocus() early return from
FindBarView::SearchTextfieldView::RequestFocus.

So... It has been so many years since I wrote this that I don't remember all the
nuances of the UX anymore. On the face of it, it sounds reasonable, but I'm
worried that the check is there to prevent some annoyance and given that people
have been blindly disabling tests due to issue 2) above we might not have the
coverage that we need to detect problems that might arise. After all, that's
what got us into this situation in the first place. :)

[msw, 2013-04-17 17:51:28]

TBR'ing Ben and landing; thanks!

[msw, 2013-04-17 18:00:58]

Committed patchset #1 manually as r194630 (presubmit successful).

[Ben Goodger (Google), 2013-04-18 17:51:13]

https://codereview.chromium.org/13976011/diff/1/chrome/browser/ui/views/find_...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/13976011/diff/1/chrome/browser/ui/views/find_...
chrome/browser/ui/views/find_bar_view.cc:505: void
FindBarView::SearchTextfieldView::RequestFocus() {
Can you do this in an OnFocus() handler? It'd be nice if RequestFocus didn't
have to be virtual.

[msw, 2013-04-18 19:10:24]

https://codereview.chromium.org/13976011/diff/1/chrome/browser/ui/views/find_...
File chrome/browser/ui/views/find_bar_view.cc (right):

https://codereview.chromium.org/13976011/diff/1/chrome/browser/ui/views/find_...
chrome/browser/ui/views/find_bar_view.cc:505: void
FindBarView::SearchTextfieldView::RequestFocus() {
On 2013/04/18 17:51:13, Ben Goodger (Google) wrote:
> Can you do this in an OnFocus() handler? It'd be nice if RequestFocus didn't
> have to be virtual.

Done in https://codereview.chromium.org/13852009/