Replicate standard menus for apps.

Replicate standard menus for apps.

This maintains a duplicate set of menus for standard items. This enables
keyboard shortcuts to work as expected when the Chrome menus are hidden.

This also adds tags to various items in MainMenu.xib using existing and new
entries from chrome_command_ids.h. The tags are used to look up items in
the main menu.

BUG=168080, 276052
TEST=When an app window is focused, the main menu should change to a set of
menus relevant to apps. I.e. Chrome specific menus/items should be hidden.
The item in the menus should behave as expected for the app.

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

[tapted, 2013-08-22 05:16:15]

+chrome-apps-syd-reviews

This will need some tests too.

And.. it feels like there should be a better way to lookup items.. Looking at
chrome/app/nibs/MainMenu.xib, there is one root menu item that has an NSTag
specified (`View`, menu has tag=44000), which crops up in 

chrome/app/chrome_command_ids.h:#define IDC_VIEW_MENU                   44000   
 // OSX only

So, I think a nicer approach would be to add tags for the menus and menu items
we care about, and look them up using [NSMenu itemWithTag:] -- feels more
robust. Might be good to get one of the core Mac owners to weigh in though. Or
perhaps dig around to see where the tags in MainMenu.xib are used currently --
maybe there is a relevant use case.

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

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:23: NSMenuItem*
FindItemByTitle(const string16& title_utf16) {
nit: base::string16

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:37: NSMenuItem* item =
[[NSMenuItem alloc]
should assign to a scoped_nsobject when using alloc, and then do return
`item.autorelease()`

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:54:
base::scoped_nsobject<NSMenuItem> item(DuplicateItem(original));
NSMenuItem implements the NSCopying protocol, so it might be enough just to do
[original copy] in order to give it a new parent menu

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:115:
editMenuItem_.reset(DuplicateItem([[NSApp mainMenu]
NSMenu supports NSCopying as well.. and I'm thinking a first cut could be the
entire Edit menu. I thought `Search the web` didn't apply, but it opens a new
browser - maybe we want that.

[jackhou1, 2013-08-29 04:25:21]

> This will need some tests too.

Done.

> So, I think a nicer approach would be to add tags for the menus and menu items
> we care about, and look them up using [NSMenu itemWithTag:] -- feels more
> robust. Might be good to get one of the core Mac owners to weigh in though. Or
> perhaps dig around to see where the tags in MainMenu.xib are used currently --
> maybe there is a relevant use case.

Changed this to look up by tag and added tags for relevant items, most of them
were already defined, but not in MainMenu.xib.

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

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:23: NSMenuItem*
FindItemByTitle(const string16& title_utf16) {
On 2013/08/22 05:16:15, tapted wrote:
> nit: base::string16

Done.

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:37: NSMenuItem* item =
[[NSMenuItem alloc]
On 2013/08/22 05:16:15, tapted wrote:
> should assign to a scoped_nsobject when using alloc, and then do return
> `item.autorelease()`

Done.

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:54:
base::scoped_nsobject<NSMenuItem> item(DuplicateItem(original));
On 2013/08/22 05:16:15, tapted wrote:
> NSMenuItem implements the NSCopying protocol, so it might be enough just to do
> [original copy] in order to give it a new parent menu

Done.

https://codereview.chromium.org/23005021/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:115:
editMenuItem_.reset(DuplicateItem([[NSApp mainMenu]
On 2013/08/22 05:16:15, tapted wrote:
> NSMenu supports NSCopying as well.. and I'm thinking a first cut could be the
> entire Edit menu. I thought `Search the web` didn't apply, but it opens a new
> browser - maybe we want that.

I'd rather keep the items minimal until we decide they should be included.

[tapted, 2013-08-29 06:06:19]

looking pretty good!

A TEST= line would probably be an improvement too

https://codereview.chromium.org/23005021/diff/11001/chrome/app/chrome_command...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/app/chrome_command...
chrome/app/chrome_command_ids.h:73: #define IDC_MINIMIZE                   
34046
maybe IDC_MINIMIZE_WINDOW, to match IDC_CLOSE_WINDOW? Same below

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:24: // A menu item
for the currently focused packaged app.
nit: update this comment?

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:35:
keyEquivalent:[original keyEquivalent]]);
Can top-level items have key equivalents, or actions? Pretty sure they can't, so
maybe just initWithTitle here, and update the function comment.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:36: if ([original
hasSubmenu]) {
DCHECK? I think later code assumes that a submenu has been copied into the
duplicate item.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:46: NSMenuItem*
AddDuplicateItem(NSMenuItem* top_level_item,
return void?

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:90: NSMenuItem*
chromeMenuQuitItem = GetItemByTag(IDC_CHROME_MENU, IDC_EXIT);
nit: probably don't need this temporary any more

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:49:
NSUInteger extra_top_level_items = 4;
nit: const?

But, I can't decide between int or NSUInteger. Maybe int - I don't think it will
generate warnings, and `const NSAnything` looks a little funny.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:63:
EXPECT_GT([[[menu submenu] itemArray] count], 0u);
there's also [NSMenu numberOfItems] - maybe that's better

[jackhou1, 2013-08-29 06:30:02]

https://codereview.chromium.org/23005021/diff/11001/chrome/app/chrome_command...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/app/chrome_command...
chrome/app/chrome_command_ids.h:73: #define IDC_MINIMIZE                   
34046
On 2013/08/29 06:06:19, tapted wrote:
> maybe IDC_MINIMIZE_WINDOW, to match IDC_CLOSE_WINDOW? Same below

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:24: // A menu item
for the currently focused packaged app.
On 2013/08/29 06:06:19, tapted wrote:
> nit: update this comment?

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:35:
keyEquivalent:[original keyEquivalent]]);
On 2013/08/29 06:06:19, tapted wrote:
> Can top-level items have key equivalents, or actions? Pretty sure they can't,
so
> maybe just initWithTitle here, and update the function comment.

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:36: if ([original
hasSubmenu]) {
On 2013/08/29 06:06:19, tapted wrote:
> DCHECK? I think later code assumes that a submenu has been copied into the
> duplicate item.

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:46: NSMenuItem*
AddDuplicateItem(NSMenuItem* top_level_item,
On 2013/08/29 06:06:19, tapted wrote:
> return void?

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:90: NSMenuItem*
chromeMenuQuitItem = GetItemByTag(IDC_CHROME_MENU, IDC_EXIT);
On 2013/08/29 06:06:19, tapted wrote:
> nit: probably don't need this temporary any more

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:49:
NSUInteger extra_top_level_items = 4;
On 2013/08/29 06:06:19, tapted wrote:
> nit: const?
> 
> But, I can't decide between int or NSUInteger. Maybe int - I don't think it
will
> generate warnings, and `const NSAnything` looks a little funny.

Done.

https://codereview.chromium.org/23005021/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:63:
EXPECT_GT([[[menu submenu] itemArray] count], 0u);
On 2013/08/29 06:06:19, tapted wrote:
> there's also [NSMenu numberOfItems] - maybe that's better

Done.

[tapted, 2013-08-29 07:11:55]

lgtm % what UX say.. and provided all the rest lands

[jackhou1, 2013-08-29 07:15:49]

thakis, please review for OWNERS

[Nico, 2013-08-29 17:14:08]

When changing xib files, please describe what you did there in the CL
description (as xib diffs are hard to read). Examples:
http://svnsearch.org/svnsearch/repos/CHROMIUM/search?logMessage=xib%20changes

[Nico, 2013-08-29 17:19:37]

lgtm with cl description updated with xib changes and with addressing the below
comments in some way (either do them or say why you decide not to, they're all
optional)

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:30: NSMenuItem*
DuplicateTopLevelItem(NSInteger menu_tag) {
Files that return a retained object should have a name starting with "New",
"Alloc", "Copy" or similar (see cocoa naming conventions, we try to follow
those)

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:49:
[GetItemByTag(menu_tag, item_tag) copy]);
Do you want to DCHECK item here? If items with that tag go away, this will
silently fail otherwise I think.

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:49:
const int extra_top_level_items = 4;
kNameConstantsLikeThis

[jackhou1, 2013-08-30 01:30:40]

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:30: NSMenuItem*
DuplicateTopLevelItem(NSInteger menu_tag) {
On 2013/08/29 17:19:38, Nico wrote:
> Files that return a retained object should have a name starting with "New",
> "Alloc", "Copy" or similar (see cocoa naming conventions, we try to follow
> those)

Done.

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:49:
[GetItemByTag(menu_tag, item_tag) copy]);
On 2013/08/29 17:19:38, Nico wrote:
> Do you want to DCHECK item here? If items with that tag go away, this will
> silently fail otherwise I think.

Done.

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/23005021/diff/20001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_browsertest.mm:49:
const int extra_top_level_items = 4;
On 2013/08/29 17:19:38, Nico wrote:
> kNameConstantsLikeThis

Done.

[commit-bot: I haz the power, 2013-08-30 17:40:15]

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

[commit-bot: I haz the power, 2013-08-30 21:20:58]

Change committed as 220659