Add quit item to app menu.

Add quit item to app menu.

This also changes the action associated with Quit Chrome when an app
is focused. This is to allow Quit <app> to have the same key binding.
The action closes the app instead of attempting to quit Chrome.

BUG=168080, 276052
TEST=Focus an app's window.
The main menu bar should contain a menu for the app which contains a Quit item.
Selecting the Quit item, or using Cmd+Q quits the app.
TEST=Focus an app's window.
Right click Chrome's dock icon and select Quit.
Chrome and all apps should quit.

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

[tapted, 2013-08-22 04:40:06]

+chrome-apps-syd-reviews -- you should install Raymes' extension ;)

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:23: NSMenuItem*
quitChromeItem_;
I think objective C policy is to use a scoped_nsobject unless you need a weak
pointer to break a cycle

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:49: quitChromeItem_ =
[[[[[NSApp mainMenu] itemAtIndex:0] submenu] itemArray]
maybe chromeMenuQuitItem_ - keeps it consistent with appQuitItem below.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:50: lastObject];
This might be a bit fragile. I think we need to look up similar to how it's done
in [ConfirmQuitPanelController quitAccelerator]. i.e. iterate through the first
submenu and look for ([item action] == @selector(terminate:))

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:57: NSMenuItem*
appQuitItem = [appMenu addItemWithTitle:@""
nit: maybe appMenuQuitItem?

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:61: [appQuitItem
setEnabled:YES];
is this line needed?

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:126: [quitChromeItem_
setAction:@selector(quitCurrentPlatformApp)];
With the current code, can they both stay as terminate:? Since, we handle that
correctly now (right?).

This feels a little weird, because it doesn't also update the target of
Chrome->Quit, so if the action is ever applied to the item's target, Chrome will
crash.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:128: NSString*
quit_localized_string =
maybe localizedQuitApp ? (should be camel case, since we're in an ObjC member)

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:133: [appQuitItem
setKeyEquivalent:[quitChromeItem_ keyEquivalent]];
should you also setKeyEquivalentModifierMask: ?

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:156: // If the currently
focused window belongs to a platform app, quit the app.
move this comment to the declaration? (if we keep this function at all ;)

[tapted, 2013-08-22 04:42:02]

On 2013/08/22 04:40:06, tapted wrote:
>
https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
> chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:126: [quitChromeItem_
> setAction:@selector(quitCurrentPlatformApp)];
> With the current code, can they both stay as terminate:? Since, we handle that
> correctly now (right?).
> 
> This feels a little weird, because it doesn't also update the target of
> Chrome->Quit, so if the action is ever applied to the item's target, Chrome
will
> crash.

Ohh, also it will break the logic in [ConfirmQuitPanelController
quitAccelerator], since it is looking for an item with @selector(terminate:) ;)

[jackhou1, 2013-08-22 06:41:01]

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:23: NSMenuItem*
quitChromeItem_;
On 2013/08/22 04:40:06, tapted wrote:
> I think objective C policy is to use a scoped_nsobject unless you need a weak
> pointer to break a cycle

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:49: quitChromeItem_ =
[[[[[NSApp mainMenu] itemAtIndex:0] submenu] itemArray]
On 2013/08/22 04:40:06, tapted wrote:
> maybe chromeMenuQuitItem_ - keeps it consistent with appQuitItem below.

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:50: lastObject];
On 2013/08/22 04:40:06, tapted wrote:
> This might be a bit fragile. I think we need to look up similar to how it's
done
> in [ConfirmQuitPanelController quitAccelerator]. i.e. iterate through the
first
> submenu and look for ([item action] == @selector(terminate:))

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:57: NSMenuItem*
appQuitItem = [appMenu addItemWithTitle:@""
On 2013/08/22 04:40:06, tapted wrote:
> nit: maybe appMenuQuitItem?

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:61: [appQuitItem
setEnabled:YES];
On 2013/08/22 04:40:06, tapted wrote:
> is this line needed?

Apparently not.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:126: [quitChromeItem_
setAction:@selector(quitCurrentPlatformApp)];
On 2013/08/22 04:40:06, tapted wrote:
> With the current code, can they both stay as terminate:? Since, we handle that
> correctly now (right?).
> 
> This feels a little weird, because it doesn't also update the target of
> Chrome->Quit, so if the action is ever applied to the item's target, Chrome
will
> crash.

Yeah, I don't really like it either. I was hoping to move the logic in
app_controller_mac into here. If they're both set to terminate:, clicking the
item will terminate Chrome.

I've changed this to unset the key equivalent instead of setting the action.
This simply disables the keyboard shortcut, which should be fine since it
shouldn't be activated while an app window is focused anyway.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:128: NSString*
quit_localized_string =
On 2013/08/22 04:40:06, tapted wrote:
> maybe localizedQuitApp ? (should be camel case, since we're in an ObjC member)

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:133: [appQuitItem
setKeyEquivalent:[quitChromeItem_ keyEquivalent]];
On 2013/08/22 04:40:06, tapted wrote:
> should you also setKeyEquivalentModifierMask: ?

Done.

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:156: // If the currently
focused window belongs to a platform app, quit the app.
On 2013/08/22 04:40:06, tapted wrote:
> move this comment to the declaration? (if we keep this function at all ;)

Done.

[tapted, 2013-08-22 07:20:15]

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/23301018/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:126: [quitChromeItem_
setAction:@selector(quitCurrentPlatformApp)];
On 2013/08/22 06:41:01, jackhou1 wrote:
> On 2013/08/22 04:40:06, tapted wrote:
> > With the current code, can they both stay as terminate:? Since, we handle
that
> > correctly now (right?).
> > 
> > This feels a little weird, because it doesn't also update the target of
> > Chrome->Quit, so if the action is ever applied to the item's target, Chrome
> will
> > crash.
> 
> Yeah, I don't really like it either. I was hoping to move the logic in
> app_controller_mac into here. If they're both set to terminate:, clicking the
> item will terminate Chrome.
> 
> I've changed this to unset the key equivalent instead of setting the action.
> This simply disables the keyboard shortcut, which should be fine since it
> shouldn't be activated while an app window is focused anyway.

Ahhhh - clicking. What if you just remove the condition

  if ([[app currentEvent] type] == NSKeyDown) {

from -
(NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication*)app in
app_controller_mac.mm? Would that let the App menu item work with terminate:
when clicked?

But if that doesn't work (and quitCurrentPlatformApp is used instead), then that
block in applicationShouldTerminate: that checks for a shellwindow should be
removed in this CL, since it should never be hit.

But, I'm actually not sure which alternative is better - this might be the time
to consult one of the Chrome mac gurus.

https://codereview.chromium.org/23301018/diff/6001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/23301018/diff/6001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:58: 
Hm.. what to do if it's not found. Perhaps DCHECK -- this should always exist.

[jackhou1, 2013-08-23 05:31:56]

> What if you just remove the condition
> 
>   if ([[app currentEvent] type] == NSKeyDown) {
> 
> from -
> (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication*)app in
> app_controller_mac.mm? Would that let the App menu item work with terminate:
> when clicked?

Yes, but it would also mean right click -> Quit on the Chrome dock icon only
closes the app.

> But if that doesn't work (and quitCurrentPlatformApp is used instead), then
that
> block in applicationShouldTerminate: that checks for a shellwindow should be
> removed in this CL, since it should never be hit.

Done.

https://codereview.chromium.org/23301018/diff/6001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/23301018/diff/6001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:58: 
On 2013/08/22 07:20:15, tapted wrote:
> Hm.. what to do if it's not found. Perhaps DCHECK -- this should always exist.

Done.

[jackhou1, 2013-08-23 05:44:30]

thakis, please review as a Mac guru

Note this CL is based off http://codereview.chromium.org/22867009/

Background:
If two menu items have the same keyEquivalent:, they must also have the same
action:, otherwise the keyboard shortcut does not work. In order to add a "Quit
<app>" item that has the same keyboard shortcut as "Quit Chrome" (e.g. Cmd+Q),
this CL sets the Quit Chrome key equivalent to @"" and restores it when the
menus change. Only one of them is shown at a time; the main menu is updated
based on whether a Chrome or app window is focused.

What do you think of this solution?

[Nico, 2013-08-23 13:46:57]

On 2013/08/23 05:44:30, jackhou1 wrote:
> thakis, please review as a Mac guru
> 
> Note this CL is based off http://codereview.chromium.org/22867009/
> 
> Background:
> If two menu items have the same keyEquivalent:, they must also have the same
> action:, otherwise the keyboard shortcut does not work.

Hm, it's unfortunate that this is true even if one of the two menu items is
hidden. This sounds like an appkit bug.

> In order to add a "Quit
> <app>" item that has the same keyboard shortcut as "Quit Chrome" (e.g. Cmd+Q),
> this CL sets the Quit Chrome key equivalent to @"" and restores it when the
> menus change. Only one of them is shown at a time; the main menu is updated
> based on whether a Chrome or app window is focused.
> 
> What do you think of this solution?

lgtm

One thing to check: It's possible to change menu shortcuts by System Preferences
-> Keyboard -> Keyboard Shortcuts -> Application Shor… (on the left) -> + ->
Chrome -> Quit. Try giving "quit" a different shortcut, and check that that
still works. Dunno if it's important that changes to "Chrome" will apply to all
chrome apps too, it's probably fine.

[jackhou1, 2013-08-26 06:12:06]

> One thing to check: It's possible to change menu shortcuts by System
Preferences
> -> Keyboard -> Keyboard Shortcuts -> Application Shor… (on the left) -> + ->
> Chrome -> Quit. Try giving "quit" a different shortcut, and check that that
> still works. Dunno if it's important that changes to "Chrome" will apply to
all
> chrome apps too, it's probably fine.

Those overrides seem to just replace items with a matching title. If another
item (e.g. "Quit <app>") has the same keyEquivalent:, that item is displayed
with no keyboard shortcut. I've changed it so that apps always use Cmd+Q. They
can't yet be changed anyway, since setting an override on the app shim does not
affect the menu item in Chrome.

[commit-bot: I haz the power, 2013-08-30 11:09:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/23301018/27004

[commit-bot: I haz the power, 2013-08-30 14:44:35]

Change committed as 220580