Explicitly use Cmd+w for "Close Window" in apps. (Mac)

Explicitly use Cmd+w for "Close Window" in apps. (Mac)

Previously, the "Close Window" item is copied from Chrome's "File" menu.
However, the key equivalent of this item changes between Cmd+Shift+w and
Cmd+w depending on whether a tabbed window has focus. This happens in
-[AppControllerMac adjustCloseWindowMenuItemKeyEquivalent:].

This causes a bug when the item in the app's menu does not match the one
in Chrome's. OSX will remove the keyboard shortcut for any items that have
a matching title but different key equivalents. 

In this CL, the key equivalent is always set to Cmd+w in the app's menu. The "Close Window" item in Chrome will be changed to Cmd+w when an app
window gains focus. Duplicate items are allowed to share a key equivalent
if their titles match.

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

[tapted, 2014-01-09 04:35:05]

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

https://codereview.chromium.org/130673002/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:243:
[[[fileMenuItem_ submenu] itemWithTag:IDC_CLOSE_WINDOW] setKeyEquivalent:@"w"];
Do you need to hook this up in something like [DopplegangerMenuItem
enableForApp:] to ensure that Close Tab (or Close Window) is not currently using
Cmd+w?

also nit: wrapping

and maybe refer to -[AppControllerMac adjustCloseWindowMenuItemKeyEquivalent:]
which is the thing resposible for this

[jackhou1, 2014-01-09 06:18:56]

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

https://codereview.chromium.org/130673002/diff/1/chrome/browser/ui/cocoa/apps...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:243:
[[[fileMenuItem_ submenu] itemWithTag:IDC_CLOSE_WINDOW] setKeyEquivalent:@"w"];
On 2014/01/09 04:35:05, tapted wrote:
> Do you need to hook this up in something like [DopplegangerMenuItem
> enableForApp:] to ensure that Close Tab (or Close Window) is not currently
using
> Cmd+w?
> 
> also nit: wrapping
> 
> and maybe refer to -[AppControllerMac adjustCloseWindowMenuItemKeyEquivalent:]
> which is the thing resposible for this

I don't think so. "Close Window" will be Cmd+w when an app window is focused.
Added a comment.

[tapted, 2014-01-09 06:54:55]

https://codereview.chromium.org/130673002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/130673002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:240: // Set the
expected key equivalent explicitly here because Chrome's key
I would say "Set the expected key equivalent explicitly here because
-[AppControllerMac adjustCloseWindowMenuItemKeyEquivalent:] sets it to "W"
(Cmd+Shift+w) when a tabbed browser is active."

Then, for this case, won't we now have two menu items whose key equivalent is
Cmd+w? i.e. (our) "Close Window", and Chrome's "Close Tab"? How does it avoid
confusing Cocoa? Since, I thought that broke things, even when one of the menu
items is hidden.

[jackhou1, 2014-01-09 22:44:07]

https://codereview.chromium.org/130673002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/130673002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:240: // Set the
expected key equivalent explicitly here because Chrome's key
On 2014/01/09 06:54:55, tapted wrote:
> I would say "Set the expected key equivalent explicitly here because
> -[AppControllerMac adjustCloseWindowMenuItemKeyEquivalent:] sets it to "W"
> (Cmd+Shift+w) when a tabbed browser is active."
>
> Then, for this case, won't we now have two menu items whose key equivalent is
> Cmd+w? i.e. (our) "Close Window", and Chrome's "Close Tab"? How does it avoid
> confusing Cocoa? Since, I thought that broke things, even when one of the menu
> items is hidden.

Done.

AppControllerMac changes the key equivalent based on what kind of window has
focus. Since app windows are not tabbed, it'll set "Close Window" to Cmd+w,
which is consistent with the one here.
When a tabbed window has focus, the one here is removed from the main menu, so
"Close Tab" is the only item using Cmd+w.

[tapted, 2014-01-09 23:24:47]

lgtm . I think you can be more confident in the CL description too - with the
added tidbit that ~`Duplicate items are allowed to share a key equivalent so
long as their title matches` I'm satisfied that this is likely to fix the issues
I'm aware of.

[commit-bot: I haz the power, 2014-01-10 04:47:52]

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

[commit-bot: I haz the power, 2014-01-10 06:55:42]

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

[commit-bot: I haz the power, 2014-01-10 07:46:25]

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

[commit-bot: I haz the power, 2014-01-10 09:11:59]

Change committed as 244133