Add Hide to app menu.

Add Hide to app menu.

This adds "Hide <app>" to the app menu that is shown when an app window
has focus. This also enables the Cmd+H shortcut. 

This also adds an IPC message to instruct the shim to hide.

This also adds a command ID for Hide and a corresponding tag to
MainMenu.xib.

BUG=168080, 276052
TEST=Start an app.
Press Cmd+H or select Hide from the main menu. The app should hide.
Clicking the app's dock icon, or Cmd+Tab to the app should show it again.

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

[tapted, 2013-08-30 07:46:22]

Don't forget to mention xib changes in the CL description.

And the code looks pretty good.. but the boilerplate/repeated constants in
app_shim_menu_controller_mac.mm give me an itch.. Will we have to do the same
for `About <app>` as well? And is there a plan for adding `Hide Others` and
`Show All`?

Not sure if it's needed, so see what you think. Something like

@interface DoppelgangerMenuItem : NSObject {
 @private
  scoped_nsobject<NSMenuItem> menuItem_;
  scoped_nsobject<NSMenuItem> sourceItem_;
  int resourceId_;
}

- (id)initWithController:(AppShimMenuController*)controller
                 menuTag:(NSInteger)menuTag
                 itemTag:(NSInteger)itemTag
              resourceId:(int)resourceId
           keyEquivalent:(NSString*)keyEquivalent
                  action:(SEL)action {
  if ((self = [super init])) {
    sourceItem_.reset(GetItemByTag(menuTag, itemTag) retain]);
    DCHECK(sourceItem);
    menuItem_.reset([sourceItem copy]);        
    resourceId_ = resourceId;

    [menuItem setKeyEquivalent_:keyEquivalent];
    [menuItem setTarget:controller]
    [menuItem setAction:action];
    [[controller appMenu] addItem:menuItem]
  }
  return self;
}

- (void)enableForApp(const extensions::Extension* app) {
  [sourceItem_ setKeyEquivalent:@""];
  if (!resourceId)
    return;

  [menuItem_ setTitle:l10n_util::GetNSStringF(resourceId_,
                                              base::UTF8ToUTF16(app->name()))];
}

- (void)disable {
  [sourceItem_ setKeyEquivalent:[menuItem keyEquivalent]];
}



Then, if you want, you could also set the same action for all the items and rely
on the tag a bit more. But I think it's a small gain, so maybe not worth it.
E.g, something like

- (void)onMenuActivated:(id)sender {
  apps::ShellWindow* shellWindow =
    apps::ShellWindowRegistry::GetShellWindowForNativeWindowAnyProfile(
          [NSApp keyWindow]);
  if (!shellWindow)
    return;

  switch ([sender tag]) {
    case IDC_HIDE_APP:
/* .. */
    case IDC_EXIT:
/* .. */
    case IDC_ABOUT:
/* .. */
  }
}

And use @selector(onMenuActivated:)

https://codereview.chromium.org/23514021/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/23514021/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:192: if (host) {
nit: no curlies

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids.h
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids...
chrome/app/chrome_command_ids.h:245: #define IDC_HIDE_APP                   
44003     // OSX only
I don't think this should be _APP, since in the default case it is Chrome (not a
Chrome App). Maybe HIDE_APPLICATION?

[jackhou1, 2013-09-03 01:20:49]

https://codereview.chromium.org/23514021/diff/1/apps/app_shim/extension_app_s...
File apps/app_shim/extension_app_shim_handler_mac.cc (right):

https://codereview.chromium.org/23514021/diff/1/apps/app_shim/extension_app_s...
apps/app_shim/extension_app_shim_handler_mac.cc:192: if (host) {
On 2013/08/30 07:46:25, tapted wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids.h
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids...
chrome/app/chrome_command_ids.h:245: #define IDC_HIDE_APP                   
44003     // OSX only
On 2013/08/30 07:46:25, tapted wrote:
> I don't think this should be _APP, since in the default case it is Chrome (not
a
> Chrome App). Maybe HIDE_APPLICATION?

The string is IDS_HIDE_APP_MAC, and I think in most cases APP is used in strings
and command ids to refer to Chrome and PACKAGED_APP is used to refer to a Chrome
App.

[tapted, 2013-09-03 08:17:25]

lgtm % IPC review

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids.h
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/23514021/diff/1/chrome/app/chrome_command_ids...
chrome/app/chrome_command_ids.h:245: #define IDC_HIDE_APP                   
44003     // OSX only
On 2013/09/03 01:20:49, jackhou1 wrote:
> On 2013/08/30 07:46:25, tapted wrote:
> > I don't think this should be _APP, since in the default case it is Chrome
(not
> a
> > Chrome App). Maybe HIDE_APPLICATION?
> 
> The string is IDS_HIDE_APP_MAC, and I think in most cases APP is used in
strings
> and command ids to refer to Chrome and PACKAGED_APP is used to refer to a
Chrome
> App.

Cool - that makes sense.

https://codereview.chromium.org/23514021/diff/15001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/23514021/diff/15001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:28: // Instructs the shim to hide the app.
nit: should we say something about the actual effect? e.g. "Effectively, doesn't
manipulate any windows, but marks the shim application as hidden in order to
display appropriate options in its Dock context menu."

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

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:16: @interface
DoppelgangerMenuItem : NSObject {
I think you can forward declare this, using 
@class DoppelgangerMenuItem

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

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:64: // Returns the
menu item.
nit: Maybe declare this as a @property(readonly, nonatomic) and remove the
comment. (otherwise, specify which of the two menu items in the comment..). The
function definition shouldn't need to change.

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:67: -
(void)enableForApp:(const extensions::Extension*) app;
nit: no space before the last `app`

[jackhou1, 2013-09-04 01:31:30]

https://codereview.chromium.org/23514021/diff/15001/apps/app_shim/app_shim_me...
File apps/app_shim/app_shim_messages.h (right):

https://codereview.chromium.org/23514021/diff/15001/apps/app_shim/app_shim_me...
apps/app_shim/app_shim_messages.h:28: // Instructs the shim to hide the app.
On 2013/09/03 08:17:25, tapted wrote:
> nit: should we say something about the actual effect? e.g. "Effectively,
doesn't
> manipulate any windows, but marks the shim application as hidden in order to
> display appropriate options in its Dock context menu."

Actually it does use the shim to hide. I don't think it's possible to mark the
shim as hidden without having it fire applicationWillHide.

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

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:16: @interface
DoppelgangerMenuItem : NSObject {
On 2013/09/03 08:17:25, tapted wrote:
> I think you can forward declare this, using 
> @class DoppelgangerMenuItem

Done.

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

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:64: // Returns the
menu item.
On 2013/09/03 08:17:25, tapted wrote:
> nit: Maybe declare this as a @property(readonly, nonatomic) and remove the
> comment. (otherwise, specify which of the two menu items in the comment..).
The
> function definition shouldn't need to change.

Done.

https://codereview.chromium.org/23514021/diff/15001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:67: -
(void)enableForApp:(const extensions::Extension*) app;
On 2013/09/03 08:17:25, tapted wrote:
> nit: no space before the last `app`

Done.

[jackhou1, 2013-09-04 01:32:21]

thakis, please review for OWNERS in chrome/app/ and chrome/browser/ui/cocoa/

palmer, please review IPC changes

[Nico, 2013-09-04 15:55:11]

lgtm

Not crazy about the Doppelganger name, but since it's limited to a single mm
file it's ok :-)

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

https://codereview.chromium.org/23514021/diff/18001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:93:
keyEquivalent:(NSString*)keyEquivalent {
Is it worth adding some DCHECk that catches if two doppelgangers are created for
a single menu item?

[palmer, 2013-09-05 19:10:32]

IPC security LGTM

[jackhou1, 2013-09-06 05:25:18]

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

https://codereview.chromium.org/23514021/diff/18001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:93:
keyEquivalent:(NSString*)keyEquivalent {
On 2013/09/04 15:55:11, Nico wrote:
> Is it worth adding some DCHECk that catches if two doppelgangers are created
for
> a single menu item?

I can't think of an easy way to do this without adding a reference to the
doppelganger to the source item. Changed this so that the source's original key
equivalent is saved. It should be okay now to have doppelgangers with the same
source item.

[commit-bot: I haz the power, 2013-09-06 05:25:40]

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

[commit-bot: I haz the power, 2013-09-06 11:06:40]

Change committed as 221676