Add shortcut to show avatar menu

Add shortcut to show avatar menu

Shortcut on Views and GTK is Control-Shift-M. Shortcut on Mac is Command-Shift-M.

BUG=75782
TEST=


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

[sail, 2011-10-20 01:43:19]

rsesek: cocoa
erg: gtk
pkasting: everything else

[Robert Sesek, 2011-10-20 16:32:33]

What happens if there's no browser window open on Mac? It looks like the command
gets disabled in that case. Isn't that ignoring a potentially common use case?

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/global_keyboa...
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/global_keyboa...
chrome/browser/global_keyboard_shortcuts_mac.mm:54: {true,  true, false, false, 
0,             'm', IDC_SHOW_AVATAR_MENU},
nit: columnar alignment, as above

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:315: -
(AvatarButtonController*)avatarButtonController;
This should be towards the top with the rest of the accessors.

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1902: -
(AvatarButtonController*)avatarButtonController {
Move to be in the same order as in the .h after you move the declaration.

[sail, 2011-10-20 20:42:49]

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/global_keyboa...
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/global_keyboa...
chrome/browser/global_keyboard_shortcuts_mac.mm:54: {true,  true, false, false, 
0,             'm', IDC_SHOW_AVATAR_MENU},
On 2011/10/20 16:32:33, rsesek wrote:
> nit: columnar alignment, as above

Done.

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.h:315: -
(AvatarButtonController*)avatarButtonController;
On 2011/10/20 16:32:33, rsesek wrote:
> This should be towards the top with the rest of the accessors.

Done.

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/8356028/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:1902: -
(AvatarButtonController*)avatarButtonController {
On 2011/10/20 16:32:33, rsesek wrote:
> Move to be in the same order as in the .h after you move the declaration.

Done.

[sail, 2011-10-20 20:44:11]

On 2011/10/20 16:32:33, rsesek wrote:
> What happens if there's no browser window open on Mac? It looks like the
command
> gets disabled in that case. Isn't that ignoring a potentially common use case?

Yea, currently it gets disabled if there are no windows.

For the no windows case, what I'd like to do is show the Users menu. I don't
think there's a public API to show a menu from the menu bar but I've used
private APIs to do this before. See:
http://www.cocoadev.com/index.pl?ProgramaticallyShowMenuInMenuBar

If you don't mind I'd like to tackle this in a separate CL.

[Elliot Glaysher, 2011-10-20 20:45:13]

The changes in the gtk files are small and lgtm

[Robert Sesek, 2011-10-20 20:49:06]

c/b/u/cocoa LGTM with nit

On 2011/10/20 20:44:11, sail wrote:
> On 2011/10/20 16:32:33, rsesek wrote:
> > What happens if there's no browser window open on Mac? It looks like the
> command
> > gets disabled in that case. Isn't that ignoring a potentially common use
case?
> 
> Yea, currently it gets disabled if there are no windows.
> 
> For the no windows case, what I'd like to do is show the Users menu. I don't
> think there's a public API to show a menu from the menu bar but I've used
> private APIs to do this before. See:
> http://www.cocoadev.com/index.pl?ProgramaticallyShowMenuInMenuBar
> 
> If you don't mind I'd like to tackle this in a separate CL.

That's fine. Just wanted to confirm the behavior.

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

Y U NO SEND MSG RIETVELD?!?!?!

http://codereview.chromium.org/8356028/diff/10001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

http://codereview.chromium.org/8356028/diff/10001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:154: -
(void)showAvatarBubble {
This shouldn't be in the private section. Move this to below |-setImage:|.

[sail, 2011-10-20 21:08:23]

http://codereview.chromium.org/8356028/diff/10001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

http://codereview.chromium.org/8356028/diff/10001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:154: -
(void)showAvatarBubble {
On 2011/10/20 20:49:21, rsesek wrote:
> This shouldn't be in the private section. Move this to below |-setImage:|.

Done.

[sail, 2011-10-20 21:08:39]

binji: FYI

[sail, 2011-10-24 19:53:59]

pkasting: PING!

[Peter Kasting, 2011-10-24 21:53:22]

Sorry, somehow I omitted this from my list of code reviews to get to...

The only question I have is whether some of the plumbing could be eliminated if
someone other than the nonclient frame view owned the menu button; or possibly
if instead of the button gaining a method to show the bubble, instead some other
code knew how to show a bubble, and we merely needed to provide it the
coordinates of the button to point at.  (This is sort of how we show the star
bubble, where we just have code that can get the relevant star icon coordinates
and return them to code elsewhere that shows the bubble.)

If neither of these would result in anything simpler, LGTM.

[sail, 2011-10-24 22:01:42]

I think changing the parent of the button is a good idea.

I'd rather not have someone else show it. The reason is that the button does
some tracking so that if you click it a 2nd time the bubble is dismissed.

Now the only question is who should own the button. Would BrowserView be ok?

Thanks!

[Peter Kasting, 2011-10-24 23:05:50]

On 2011/10/24 22:01:42, sail wrote:
> Now the only question is who should own the button. Would BrowserView be ok?

I don't know.  It's mostly going to come down to what combination of (plumbing
to access it + logic to ensure it's only shown on the correct kinds of windows)
you have to write and how visibly logical the placement is (e.g. it would be
weird for the tabstrip to own the wrench menu regardless of if that somehow made
the code simple).  I would imagine that the right design would be obviously
simple in both regards.

[sail, 2011-10-25 17:35:13]

On 2011/10/24 23:05:50, Peter Kasting wrote:
> On 2011/10/24 22:01:42, sail wrote:
> > Now the only question is who should own the button. Would BrowserView be ok?
> 
> I don't know.  It's mostly going to come down to what combination of (plumbing
> to access it + logic to ensure it's only shown on the correct kinds of
windows)
> you have to write and how visibly logical the placement is (e.g. it would be
> weird for the tabstrip to own the wrench menu regardless of if that somehow
made
> the code simple).  I would imagine that the right design would be obviously
> simple in both regards.

Hey Peter. I moved the button to BrowserFrame but the code doesn't seem any
simpler. There's less duplication between GlassBrowserFrameView and
OpaqueBrowserFrameView but they both end up calling into the BrowserFrame for
hit test and layout now.

I'd like to get this CL merged to M16 so I'd like to avoid making it anymore
complicated. Is it ok if I do a proper refactoring in a separate change?

Thanks.

[Peter Kasting, 2011-10-25 18:11:47]

On 2011/10/25 17:35:13, sail wrote:
> Hey Peter. I moved the button to BrowserFrame but the code doesn't seem any
> simpler. There's less duplication between GlassBrowserFrameView and
> OpaqueBrowserFrameView but they both end up calling into the BrowserFrame for
> hit test and layout now.

Oof.  Sounds like maybe not the right tradeoff.

> I'd like to get this CL merged to M16 so I'd like to avoid making it anymore
> complicated. Is it ok if I do a proper refactoring in a separate change?

Sure.  It sounds like there may not end up being any real good refactoring ideas
anyway.

[commit-bot: I haz the power, 2011-10-25 18:27:18]

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

[commit-bot: I haz the power, 2011-10-25 20:13:21]

Try job failure for 8356028-13001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-10-25 21:12:12]

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

[commit-bot: I haz the power, 2011-10-25 22:58:50]

Change committed as 107226