Add remaining items to Chrome App menus. (Mac)

Add remaining items to Chrome App menus. (Mac)

These are disabled as they are not yet implemented. They are being added
now primarily for aesthetic reasons, on advice from UX.

Menu items added:
<app> -> About <app>
File -> New
File -> Open File

Menu items removed:
Edit -> Find

This also adds a new resource string for "New", and NSTags to various
NSMenuItems in MainMenu.xib.

BUG=168080, 276052
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224687

[tapted, 2013-09-18 07:11:45]

+chrome-apps-syd-reviews

Make sure you highlight the xib changes in the CL description. I'd also say that
these are being added primarily for aesthetic reasons at this stage, on advice
from UX.

I'm even a bit wary of adding these now until the full story is sorted out
w.r.t. automatically detecting whether a particular app can support a New or an
Open. It might turn out that we can't do that reliably, and letting disabled
items appear now could get people's hopes up, or think that it is a bug ;)

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:195:
newDoppelganger_.reset([[DoppelgangerMenuItem alloc]
should this be setEnabled:NO as well? Same for Open.

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:202:
[[newDoppelganger_ menuItem] setTitle:l10n_util::GetNSString(IDS_NEW_MAC)];
Maybe a comment here, saying that, for apps, the "Window" part of "New Window"
is dropped to match the default menu set given to Cocoa Apps.

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:241: // "Start
Dictation" and "Special Characters" are added by OSX, so we can copy
This comment doesn't make clear to me the connection between Start Dictation /
Special Characters, and removing the two other items.

[tapted, 2013-09-18 07:12:17]

+chrome-apps-syd-reviews

Make sure you highlight the xib changes in the CL description. I'd also say that
these are being added primarily for aesthetic reasons at this stage, on advice
from UX.

I'm even a bit wary of adding these now until the full story is sorted out
w.r.t. automatically detecting whether a particular app can support a New or an
Open. It might turn out that we can't do that reliably, and letting disabled
items appear now could get people's hopes up, or think that it is a bug ;)

[jackhou1, 2013-09-19 03:05:05]

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:195:
newDoppelganger_.reset([[DoppelgangerMenuItem alloc]
On 2013/09/18 07:11:45, tapted wrote:
> should this be setEnabled:NO as well? Same for Open.

Not needed because only appMenuItem_ has autoenablesItems turned off. Moved the
setEnabled line lower down where it's more obvious.

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:202:
[[newDoppelganger_ menuItem] setTitle:l10n_util::GetNSString(IDS_NEW_MAC)];
On 2013/09/18 07:11:45, tapted wrote:
> Maybe a comment here, saying that, for apps, the "Window" part of "New Window"
> is dropped to match the default menu set given to Cocoa Apps.

Done.

https://codereview.chromium.org/24153009/diff/3001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:241: // "Start
Dictation" and "Special Characters" are added by OSX, so we can copy
On 2013/09/18 07:11:45, tapted wrote:
> This comment doesn't make clear to me the connection between Start Dictation /
> Special Characters, and removing the two other items.

Oops, it should say "can't copy them".

[jackhou1, 2013-09-19 07:55:51]

benwells, what do you think of this?

The intention is to implement About, New, and Open File in the near future.

[benwells, 2013-09-19 11:46:57]

On 2013/09/19 07:55:51, jackhou1 wrote:
> benwells, what do you think of this?
> 
> The intention is to implement About, New, and Open File in the near future.

How would you implement them?

[jackhou1, 2013-09-19 23:09:45]

On 2013/09/19 11:46:57, benwells wrote:
> On 2013/09/19 07:55:51, jackhou1 wrote:
> > benwells, what do you think of this?
> > 
> > The intention is to implement About, New, and Open File in the near future.
> 
> How would you implement them?

For About, I think initially it can just show an OSX style about panel. When the
Apps Developer Tool is ready, it could probably link to that.

For New and About, I was thinking it could check whether the app has any file
handlers. If so, New can just send an onLaunched, Open can show a file selector
and then send an onLaunched with that file.

[benwells, 2013-09-20 06:33:46]

On 2013/09/19 23:09:45, jackhou1 wrote:
> On 2013/09/19 11:46:57, benwells wrote:
> > On 2013/09/19 07:55:51, jackhou1 wrote:
> > > benwells, what do you think of this?
> > > 
> > > The intention is to implement About, New, and Open File in the near
future.
> > 
> > How would you implement them?
> 
> For About, I think initially it can just show an OSX style about panel. When
the
> Apps Developer Tool is ready, it could probably link to that.
> 
> For New and About, I was thinking it could check whether the app has any file
> handlers. If so, New can just send an onLaunched, Open can show a file
selector
> and then send an onLaunched with that file.

OK. I don't really like these ideas for implementation and don't think we should
do any of them. But, if the UI is better with these items in a disabled state we
should add them in anyway, I guess.

[tapted, 2013-09-20 06:45:05]

code lgtm, and seems reasonable for the aesthetics.

[jackhou1, 2013-09-20 06:47:12]

thakis, please review for OWNERS

[tapted, 2013-09-20 06:48:00]

(slgtm)

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

https://codereview.chromium.org/24153009/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:242: // "Paste and
Match Style" and "Find". This is because the last two items,
no-op/optional: also I'd consider the sentence "This is because.." an optional
addition. Copying the whole menu and removing two items is probably more elegant
anyway. So, up to you if you want to keep it.

[Nico, 2013-09-20 16:18:48]

lgtm

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

https://codereview.chromium.org/24153009/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:179:
keyEquivalent:@""]);
I'm wondering if there's some way to do this that doesn't duplicate quite a few
menu items from the xib file in code. Maybe the tag could have a high bit set if
the entry should have a duplicate – but I guess that's kind of messy too.

[jackhou1, 2013-09-21 00:53:05]

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

https://codereview.chromium.org/24153009/diff/11001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:179:
keyEquivalent:@""]);
On 2013/09/20 16:18:49, Nico wrote:
> I'm wondering if there's some way to do this that doesn't duplicate quite a
few
> menu items from the xib file in code. Maybe the tag could have a high bit set
if
> the entry should have a duplicate – but I guess that's kind of messy too.

I don't think that encodes enough information about the duplicate's behaviour.
It might make sense to add another xib file for app menus. I don't expect to add
any more items after this CL though, so maybe it's not necessary.

[commit-bot: I haz the power, 2013-09-21 00:53:50]

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

[commit-bot: I haz the power, 2013-09-21 03:44:11]

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

[commit-bot: I haz the power, 2013-09-21 07:24:20]

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

[commit-bot: I haz the power, 2013-09-21 10:17:03]

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

[commit-bot: I haz the power, 2013-09-23 04:32:57]

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

[commit-bot: I haz the power, 2013-09-23 07:27:31]

Change committed as 224687