Fixes focus traversal bug

Fixes focus traversal bug

When on the last focusable view in a Container FocusManager would
start searching from Widget. Problem is child Widgets share the same
FocusManager, meaning focus in a child widget wraps to the parent,
which is unexpected in most situations. In theory there could be
situations where we should wrap to the parent, but I don't think we
have any.

BUG=276213
TEST=covered by test
R=dmazzoni@chromium.org

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

[dmazzoni, 2013-09-03 16:33:57]

lgtm

https://codereview.chromium.org/23475012/diff/3001/ui/views/focus/focus_manag...
File ui/views/focus/focus_manager_unittest.cc (right):

https://codereview.chromium.org/23475012/diff/3001/ui/views/focus/focus_manag...
ui/views/focus/focus_manager_unittest.cc:857:
GetFocusManager()->AdvanceFocus(false);
Also cycle in the reverse direction for better test coverage.

[sky, 2013-09-03 16:48:16]

https://codereview.chromium.org/23475012/diff/3001/ui/views/focus/focus_manag...
File ui/views/focus/focus_manager_unittest.cc (right):

https://codereview.chromium.org/23475012/diff/3001/ui/views/focus/focus_manag...
ui/views/focus/focus_manager_unittest.cc:857:
GetFocusManager()->AdvanceFocus(false);
On 2013/09/03 16:33:57, Dominic Mazzoni wrote:
> Also cycle in the reverse direction for better test coverage.
> 

Done.

[commit-bot: I haz the power, 2013-09-03 16:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23475012/8001

[commit-bot: I haz the power, 2013-09-03 18:56:19]

Retried try job too often on win7_aura for step(s) views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[sky, 2013-09-04 20:09:55]

Turns out we have some widgets that don't want the new behavior (say the find
bar). I added a WidgetDelegate method to control new behavior. The find bar now
works correctly with this patch since the find bar doesn't supply a
WidgetDelegate, which means it gets the DefaultWidgetDelegate implementation
that does the right thing.

[sky, 2013-09-04 20:10:01]

Take another look?

[dmazzoni, 2013-09-04 20:40:54]

If the Find bar is the only special case, would it make more sense to override
FocusSearch like we did in AccessiblePaneView?

(Actually it looks like the FocusSearch override is now unused since
LocationBarContainer was deleted. I'll get rid of it.)

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.cc:312: widget = widget_;
Isn't it possible that |widget|'s parent is not |widget_|?  It doesn't seem
quite accurate to call this API ShouldAdvanceFocusToParent when that's not
exactly what it does. How about ShouldCycleFocusOutsideOfWidget or something
more along those lines?

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.h:313: // |starting_view| and |starting_widget| are
NULL, traversal starts at
This comment should mention the widget delegate method.

[sky, 2013-09-04 20:50:28]

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.cc:312: widget = widget_;
On 2013/09/04 20:40:54, Dominic Mazzoni wrote:
> Isn't it possible that |widget|'s parent is not |widget_|?

Certainly. But I don't think it comes up much (at all?).

> It doesn't seem
> quite accurate to call this API ShouldAdvanceFocusToParent when that's not
> exactly what it does. How about ShouldCycleFocusOutsideOfWidget or something
> more along those lines?

Renamed to ShouldAdvanceFocusToTopLevelWidget()

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.h:313: // |starting_view| and |starting_widget| are
NULL, traversal starts at
On 2013/09/04 20:40:54, Dominic Mazzoni wrote:
> This comment should mention the widget delegate method.

Done.

[dmazzoni, 2013-09-04 21:07:16]

lgtm

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.h:313: // |starting_view| and |starting_widget| are
NULL, traversal starts at
On 2013/09/04 20:50:28, sky wrote:
> On 2013/09/04 20:40:54, Dominic Mazzoni wrote:
> > This comment should mention the widget delegate method.
> 
> Done.

nit: I don't see this in the uploaded changelist

https://codereview.chromium.org/23475012/diff/28008/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/23475012/diff/28008/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:94: // is used as a container, so that we want focus
to advance to the parent. A
nit: parent -> top-level widget

[sky, 2013-09-04 21:10:03]

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/23475012/diff/27001/ui/views/focus/focus_mana...
ui/views/focus/focus_manager.h:313: // |starting_view| and |starting_widget| are
NULL, traversal starts at
On 2013/09/04 21:07:17, Dominic Mazzoni wrote:
> On 2013/09/04 20:50:28, sky wrote:
> > On 2013/09/04 20:40:54, Dominic Mazzoni wrote:
> > > This comment should mention the widget delegate method.
> > 
> > Done.
> 
> nit: I don't see this in the uploaded changelist

Woops. Forgot to save the file. Done.

[commit-bot: I haz the power, 2013-09-04 21:12:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23475012/38001

[commit-bot: I haz the power, 2013-09-04 22:05:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-04 22:10:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23475012/55001

[commit-bot: I haz the power, 2013-09-04 23:14:16]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 15:32:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23475012/55001

[commit-bot: I haz the power, 2013-09-05 15:40:36]

Change committed as 221437