Swap main menu with app-specific menu when app window focused.

chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[tapted, 2013/08/15 04:39:23]

nit: no (c)

[jackhou1, 2013/08/16 02:50:41]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h"
+
+#include "apps/app_shim/extension_app_shim_handler_mac.h"
+#include "apps/shell_window.h"
+#include "apps/shell_window_registry.h"
+#include "base/strings/sys_string_conversions.h"
+#include "chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h"

[tapted, 2013/08/15 04:39:23]

nit: import

[jackhou1, 2013/08/16 02:50:41]

Done.

+#include "chrome/common/extensions/extension.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+@interface AppMenuController (Private)

[tapted, 2013/08/15 04:39:23]

I think the new trend is just to use an unnamed category, like

@interface AppMenuController ()

[jackhou1, 2013/08/16 02:50:41]

Done.

+- (void)windowMainStatusChanged:(NSNotification*)notification;
+- (void)addMenuItems:(const extensions::Extension*)app;
+- (void)removeMenuItems:(NSString*)appId;
+@end
+
+@implementation AppMenuController
+
+- (id)init {
+  self = [super init];
+  if (self == nil)

[tapted, 2013/08/15 04:39:23]

Obj-C initializers all follow the pattern

- (id)init {
  if ((self = [super init])) {
    /* initialize stuff here */
  }
  return self;
}


It breaks the "prefer early return" idiom, but all Objc-C initializers are this
way. Makes them feel special.

[jackhou1, 2013/08/16 02:50:41]

Done.

+    return nil;
+
+  appMenuItems_.reset([[NSMutableArray alloc] initWithCapacity:0]);
+
+  [[NSNotificationCenter defaultCenter]
+      addObserver:self
+         selector:@selector(windowMainStatusChanged:)
+             name:NSWindowDidBecomeMainNotification
+           object:nil];
+
+  [[NSNotificationCenter defaultCenter]
+      addObserver:self
+         selector:@selector(windowMainStatusChanged:)
+             name:NSWindowDidResignMainNotification
+           object:nil];
+
+  return self;
+}
+
+// If the window is an app window, add or remove menu items.

[tapted, 2013/08/15 04:39:23]

nit: I think these function comments should be at the declaration, or inside the
function body.

[jackhou1, 2013/08/16 02:50:41]

Done.

+- (void)windowMainStatusChanged:(NSNotification*)notification {
+  id window = [notification object];
+  id windowController = [window windowController];
+  if (![windowController isKindOfClass:[NativeAppWindowController class]])
+    return;
+
+  apps::ShellWindow* shellWindow =
+      apps::ShellWindowRegistry::GetShellWindowForNativeWindowAnyProfile(

[tapted, 2013/08/15 04:39:23]

is there any danger this can return NULL for a window that's being closed? Or if
a profile is being deleted while it has app windows open?

[jackhou1, 2013/08/16 02:50:41]

I don't think so. ShellWindow::Close closes the native window first. Profile
deletion doesn't delete the Profile instance.

[jackhou1, 2013/08/19 03:47:57]

Changed to use isEqualToString

+          window);
+  if (!shellWindow)
+    return;
+
+  if ([notification name] == NSWindowDidBecomeMainNotification) {

[tapted, 2013/08/15 04:39:23]

need to use isEqualToString: here - same below.

[jackhou1, 2013/08/16 02:50:41]

I copied this from app_controller_mac.mm, e.g. line 546:
if ([notify name] == NSWindowDidResignKeyNotification) {

I think it works because the notifications name is set to that same string. Is
it still better to use isEqualToString?

+    [self addMenuItems:shellWindow->extension()];
+  } else if ([notification name] == NSWindowDidResignMainNotification) {
+    [self removeMenuItems:base::SysUTF8ToNSString(shellWindow->extension_id())];
+  }

[tapted, 2013/08/15 04:39:23]

else { NOTREACHED() } ?

And... don't need curlies on this if - pretty sure the rule still applies when
there is an 'else if' -- but it will probably become multi line with the other
comments.

[jackhou1, 2013/08/16 02:50:41]

Done.

+}
+
+// Add menu items for an app and hide Chrome menu items.
+- (void)addMenuItems:(const extensions::Extension*)app {
+  NSString* appId = base::SysUTF8ToNSString(app->id());
+  NSString* title = base::SysUTF8ToNSString(app->name());

[tapted, 2013/08/15 04:39:23]

I don't think you need this temporary - you can move the conversion to
initWithTitle:...

[jackhou1, 2013/08/16 02:50:41]

There will be other menu items added later that use the title.

+
+  NSMenu* mainMenu = [NSApp mainMenu];

[tapted, 2013/08/15 04:39:23]

nit: move closer to first use

[jackhou1, 2013/08/16 02:50:41]

Done.

+
+  if (appId_) {

[tapted, 2013/08/15 04:39:23]

You can lean on Obj-C's "yes you really can call a function on a null pointer",
and do (instead of these 8 lines)

if ([appId_ isEqualToString:appId])
  return;

[self removeMenuItems:appId_];
appId_.reset([appId retain]);

[jackhou1, 2013/08/16 02:50:41]

Done.

+    if ([appId_ isEqualToString:appId])
+      return;
+    else
+      [self removeMenuItems:appId_];
+  }
+  [appId retain];
+  appId_.reset(appId);
+
+  for (NSMenuItem* item in [mainMenu itemArray])
+    [item setHidden:YES];
+
+  NSMenuItem* item = [mainMenu addItemWithTitle:appId
+                                         action:nil
+                                  keyEquivalent:@""];
+  base::scoped_nsobject<NSMenu> appMenu([[NSMenu alloc] initWithTitle:title]);
+  [item setSubmenu:appMenu];
+  [appMenuItems_ addObject:item];
+}
+
+// If the window belongs to the currently focused app, remove the menu items and
+// unhide Chrome menu items.
+- (void)removeMenuItems:(NSString*)appId {
+  if (![appId_ isEqualToString:appId])
+    return;
+
+  NSMenu* mainMenu = [NSApp mainMenu];
+
+  appId_.reset();

[tapted, 2013/08/15 04:39:23]

nit: move this up? (i.e. swap with line 99). It's more relevant to the initial
`if` than the loop below.

[jackhou1, 2013/08/16 02:50:41]

Done.

+  for (NSMenuItem* item in appMenuItems_.get())
+    [mainMenu removeItem:item];
+  [appMenuItems_ removeAllObjects];
+
+  // Restore the Chrome main menu bar.
+  for (NSMenuItem* item in [mainMenu itemArray])
+    [item setHidden:NO];
+}
+
+@end  // @implementation AppMenuController

[tapted, 2013/08/15 04:39:23]

nit: I think current trend is to not have comments after @end

[jackhou1, 2013/08/16 02:50:41]

Done.