Cocoa: Support keyboard navigation in avatar menu

Cocoa: Support keyboard navigation in avatar menu

Added support for up arrow, down arrow, and enter key.

BUG=75782
TEST=


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

[Robert Sesek, 2011-10-24 19:07:04]

Overall, awesome. Just some type nits and a question.

Thanks for doing this!

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h:75:
@property(assign, nonatomic) BOOL isHighlighted;
Could you instead make the view key and test for that, rather than maintaing a
separate BOOL? Would it make sense to do that?

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:257: -
(void)highlightNextItemByDelta:(int)delta {
NSInteger

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:258: int count
= [items_ count];
NSUInteger

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:262: int
old_index = -1;
NSInteger

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:263: for (int i
= 0; i < count; i++) {
NSUInteger

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:284:
AvatarMenuItemController* oldItem = nil;
No need to initialize.

[sail, 2011-10-24 19:16:38]

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h:75:
@property(assign, nonatomic) BOOL isHighlighted;
On 2011/10/24 19:07:04, rsesek wrote:
> Could you instead make the view key and test for that, rather than maintaing a
> separate BOOL? Would it make sense to do that?

Currently we highlight on mouse over too. I think changing the key view on mouse
over might be a bit weird.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:257: -
(void)highlightNextItemByDelta:(int)delta {
On 2011/10/24 19:07:04, rsesek wrote:
> NSInteger

Done.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:258: int count
= [items_ count];
On 2011/10/24 19:07:04, rsesek wrote:
> NSUInteger

Done.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:262: int
old_index = -1;
On 2011/10/24 19:07:04, rsesek wrote:
> NSInteger

Done.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:263: for (int i
= 0; i < count; i++) {
On 2011/10/24 19:07:04, rsesek wrote:
> NSUInteger

Done.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:284:
AvatarMenuItemController* oldItem = nil;
On 2011/10/24 19:07:04, rsesek wrote:
> No need to initialize.

Are stack variables automatically initialized to nil? If no item is highlighted
then the below for loop might not change the value of this variable.

[Robert Sesek, 2011-10-24 19:25:53]

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h:75:
@property(assign, nonatomic) BOOL isHighlighted;
On 2011/10/24 19:16:38, sail wrote:
> On 2011/10/24 19:07:04, rsesek wrote:
> > Could you instead make the view key and test for that, rather than maintaing
a
> > separate BOOL? Would it make sense to do that?
> 
> Currently we highlight on mouse over too. I think changing the key view on
mouse
> over might be a bit weird.

OK.

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

http://codereview.chromium.org/8332008/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:284:
AvatarMenuItemController* oldItem = nil;
On 2011/10/24 19:16:38, sail wrote:
> On 2011/10/24 19:07:04, rsesek wrote:
> > No need to initialize.
> 
> Are stack variables automatically initialized to nil? If no item is
highlighted
> then the below for loop might not change the value of this variable.

Oops, no. Only global scope variables and instance variables.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h (right):

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h:61: // Tracks
wheter this item is currently highlighted.
nit: whether

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:30: -
(void)keyDown:(NSEvent *)theEvent;
nit: no space before *

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:34: -
(void)highlightNextItemByDelta:(int)delta;
nit: NSInteger

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:236: -
(void)keyDown:(NSEvent *)theEvent {
nit: same space

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:257: -
(void)highlightNextItemByDelta:(int)delta {
nit: NSUInteger

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:263: for
(NSUInteger i = 0; i < count; i++) {
nit: postfix

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:276: new_index
= old_index + delta;
nit: blank line after

[sail, 2011-10-24 19:32:46]

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h (right):

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.h:61: // Tracks
wheter this item is currently highlighted.
On 2011/10/24 19:25:53, rsesek wrote:
> nit: whether

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:30: -
(void)keyDown:(NSEvent *)theEvent;
On 2011/10/24 19:25:53, rsesek wrote:
> nit: no space before *

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:34: -
(void)highlightNextItemByDelta:(int)delta;
On 2011/10/24 19:25:53, rsesek wrote:
> nit: NSInteger

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:236: -
(void)keyDown:(NSEvent *)theEvent {
On 2011/10/24 19:25:53, rsesek wrote:
> nit: same space

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:257: -
(void)highlightNextItemByDelta:(int)delta {
On 2011/10/24 19:25:53, rsesek wrote:
> nit: NSUInteger

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:263: for
(NSUInteger i = 0; i < count; i++) {
On 2011/10/24 19:25:53, rsesek wrote:
> nit: postfix

Done.

http://codereview.chromium.org/8332008/diff/12/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:276: new_index
= old_index + delta;
On 2011/10/24 19:25:53, rsesek wrote:
> nit: blank line after

Done.

[Robert Sesek, 2011-10-24 20:21:49]

lgtm

[commit-bot: I haz the power, 2011-10-24 20:29:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8332008/2002

[commit-bot: I haz the power, 2011-10-24 22:24:52]

Change committed as 106991