Fix toolbar keyboard accessibility if bookmarks bar is shown.

Fix toolbar keyboard accessibility if bookmarks bar is shown.

If the bookmarks bar was shown on startup, toolbar focus searching
was broken because the code that tried to override the focus search
order of the location bar container got undone when the bookmarks
bar became a sibling of the location bar container. To fix this,
overriding the focus search order is now done dynamically when the
toolbar gets focus.

Adds a new test for this case.

BUG=151747
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158559

[sky, 2012-09-24 14:05:31]

http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view_browsertest.cc (right):

http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view_browsertest.cc:95:
IN_PROC_BROWSER_TEST_F(ToolbarViewTest, PRE_ToolbarCycleFocusWithBookmarkBar) {
Why does this need to be a separate test?

[dmazzoni, 2012-09-24 16:58:23]

http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view_browsertest.cc (right):

http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view_browsertest.cc:95:
IN_PROC_BROWSER_TEST_F(ToolbarViewTest, PRE_ToolbarCycleFocusWithBookmarkBar) {
On 2012/09/24 14:05:32, sky wrote:
> Why does this need to be a separate test?

See "span a restart" in in_process_browser_test.h if you're not familiar with
the magic "PRE_" prefix for a test.

http://code.google.com/p/chromium/source/search?q=%22span+a+restart%22&origq=...

This particular bug only reproduces if the bookmarks bar is visible when the
window opens, not if you open it later. It'd be possible to open the bookmarks
bar and then open a new browser window within the test, of course, but it'd be a
lot more boilerplate code.

[sky, 2012-09-24 20:52:53]

On Mon, Sep 24, 2012 at 9:58 AM,  <dmazzoni@chromium.org> wrote:
>
>
http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
> File chrome/browser/ui/views/toolbar_view_browsertest.cc (right):
>
>
http://codereview.chromium.org/10962048/diff/5002/chrome/browser/ui/views/too...
> chrome/browser/ui/views/toolbar_view_browsertest.cc:95:
> IN_PROC_BROWSER_TEST_F(ToolbarViewTest,
> PRE_ToolbarCycleFocusWithBookmarkBar) {
> On 2012/09/24 14:05:32, sky wrote:
>>
>> Why does this need to be a separate test?
>
>
> See "span a restart" in in_process_browser_test.h if you're not familiar
> with the magic "PRE_" prefix for a test.
>
>
http://code.google.com/p/chromium/source/search?q=%22span+a+restart%22&origq=...
>
> This particular bug only reproduces if the bookmarks bar is visible when
> the window opens, not if you open it later.
> It'd be possible to open the
> bookmarks bar and then open a new browser window within the test, of
> course, but it'd be a lot more boilerplate code.

browser_tests are extremely time consuming to wrong. Since your test
doesn't need two runs I would rather see a single test doing this.

  -Scott

[dmazzoni, 2012-09-24 22:29:34]

On 2012/09/24 20:52:53, sky wrote:
> browser_tests are extremely time consuming to wrong. Since your test
> doesn't need two runs I would rather see a single test doing this.

No problem, done.

[sky, 2012-09-24 23:07:26]

LGTM

[commit-bot: I haz the power, 2012-09-25 06:09:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/10962048/15001

[commit-bot: I haz the power, 2012-09-25 13:10:21]

Change committed as 158559