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

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

When an app window is focused, a new menu item is created for that app 
and appended to the main menu. All of Chrome's menu items are hidden. 
When a Chrome window is focused, the app's menu item is removed and 
Chrome's menu items are unhidden.

This is a second attempt at https://codereview.chromium.org/18089012/
which had to be reverted because it broke keyboard shortcuts.

This moves all packaged app menu item handling into a new AppShimMenuController.

BUG=168080, 276052
Focus the app window, the only item in the main menu bar should be that app. 
Focus a browser window, the main menu bar returns to normal.

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

[jackhou1, 2013-08-15 01:29:30]

This does not yet fix keyboard shortcuts. I can either add that to this CL, or
get all CLs that depend on this one ready to commit and commit them close
together.

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

+ chrome-apps-syd-reviews

I started the review before the follow-up patchsets came in, so I think a
comment might be done already ;). Otherwise, consider the diff reviewed too.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:320:
appMenuController_.reset([[AppMenuController alloc] init]);
I think we should guard this with the enable app shims flag. If we check just
the flag (and not IsAppShimsEnabled), then maybe it's OK to land this with
shortcuts not yet implemented..

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:439: [self unregisterEventHandlers];
You should call into appMenuController_ to unregisterEventHandlers here too, and
do the same in that function (i.e. [[NSNotificationCenter defaultCenter]
removeObserver:self]; ).

And then perhaps appMenuController_.reset().

There's also the option of removing the observer in AppMenuController dealloc
and doing _just_ a reset(), but I think it's better to guarantee the observer
removal at this point explicitly

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:20: // The app id of the
currently focused packaged app.
nit: maybe in the comment call it an extension id in case someone thinks there's
a different id namespace for apps..

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:23:
base::scoped_nsobject<NSMutableArray> appMenuItems_;
I think until we add multiple items, this should just be a single NSMenuItem
rather han an array.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: - (id)init;
Don't need to redeclare this, since you inherit the dec from NSObject.

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:11: #include
"chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h"
nit: import

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:16: @interface
AppMenuController (Private)
I think the new trend is just to use an unnamed category, like

@interface AppMenuController ()

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:26: if (self == nil)
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.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:46: // If the window is
an app window, add or remove menu items.
nit: I think these function comments should be at the declaration, or inside the
function body.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:54:
apps::ShellWindowRegistry::GetShellWindowForNativeWindowAnyProfile(
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?

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:59: if ([notification
name] == NSWindowDidBecomeMainNotification) {
need to use isEqualToString: here - same below.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:63: }
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.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:69: NSString* title =
base::SysUTF8ToNSString(app->name());
I don't think you need this temporary - you can move the conversion to
initWithTitle:...

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:71: NSMenu* mainMenu =
[NSApp mainMenu];
nit: move closer to first use

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:73: if (appId_) {
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]);

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:101: appId_.reset();
nit: move this up? (i.e. swap with line 99). It's more relevant to the initial
`if` than the loop below.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:111: @end  //
@implementation AppMenuController
nit: I think current trend is to not have comments after @end

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:15: #import
"chrome/common/chrome_switches.h"
nit: #include

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:23:
AppMenuControllerBrowserTest() {}
Since we're C++, you need to initialize data members

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:26: // Start
two apps and wait for them to be launched.
nit: comment before declaration?

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:38: void
CheckAppMenu(const extensions::Extension* app) {
nit: CheckHasAppMenu? You could declare these functions `const` too I think

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:59: NSMenu*
main_menu_;
Maybe better without this data member, since it raises ownership questions.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:61: };
nit: private: DISALLOW_COPY_AND_ASSIGN(..)  (+ destructor that I think it will
force you to have)

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:103: }
Maybe a test for unloading/uninstalling an extension with windows open?

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:439:
'browser/ui/cocoa/apps/app_menu_controller_mac.h',
.h before .mm

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:1563:
'browser/ui/cocoa//autofill/autofill_dialog_cocoa_browsertest.mm',
hm.. why is there a double-slash on this line...  let's clean that up while
we're here (since otherwise our sorting is technically wrong :p)

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:320:
appMenuController_.reset([[AppMenuController alloc] init]);
On 2013/08/15 04:39:23, tapted wrote:
> I think we should guard this with the enable app shims flag. If we check just
> the flag (and not IsAppShimsEnabled), then maybe it's OK to land this with
> shortcuts not yet implemented..

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/app_controller...
chrome/browser/app_controller_mac.mm:439: [self unregisterEventHandlers];
On 2013/08/15 04:39:23, tapted wrote:
> You should call into appMenuController_ to unregisterEventHandlers here too,
and
> do the same in that function (i.e. [[NSNotificationCenter defaultCenter]
> removeObserver:self]; ).
> 
> And then perhaps appMenuController_.reset().
> 
> There's also the option of removing the observer in AppMenuController dealloc
> and doing _just_ a reset(), but I think it's better to guarantee the observer
> removal at this point explicitly

Done.

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/08/15 04:39:23, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:20: // The app id of the
currently focused packaged app.
On 2013/08/15 04:39:23, tapted wrote:
> nit: maybe in the comment call it an extension id in case someone thinks
there's
> a different id namespace for apps..

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:23:
base::scoped_nsobject<NSMutableArray> appMenuItems_;
On 2013/08/15 04:39:23, tapted wrote:
> I think until we add multiple items, this should just be a single NSMenuItem
> rather han an array.

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: - (id)init;
On 2013/08/15 04:39:23, tapted wrote:
> Don't need to redeclare this, since you inherit the dec from NSObject.

Done.

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/08/15 04:39:23, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:11: #include
"chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h"
On 2013/08/15 04:39:23, tapted wrote:
> nit: import

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:16: @interface
AppMenuController (Private)
On 2013/08/15 04:39:23, tapted wrote:
> I think the new trend is just to use an unnamed category, like
> 
> @interface AppMenuController ()

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:26: if (self == nil)
On 2013/08/15 04:39:23, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:46: // If the window is
an app window, add or remove menu items.
On 2013/08/15 04:39:23, tapted wrote:
> nit: I think these function comments should be at the declaration, or inside
the
> function body.

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:54:
apps::ShellWindowRegistry::GetShellWindowForNativeWindowAnyProfile(
On 2013/08/15 04:39:23, tapted wrote:
> 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?

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:59: if ([notification
name] == NSWindowDidBecomeMainNotification) {
On 2013/08/15 04:39:23, tapted wrote:
> need to use isEqualToString: here - same below.

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?

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:63: }
On 2013/08/15 04:39:23, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:69: NSString* title =
base::SysUTF8ToNSString(app->name());
On 2013/08/15 04:39:23, tapted wrote:
> I don't think you need this temporary - you can move the conversion to
> initWithTitle:...

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:71: NSMenu* mainMenu =
[NSApp mainMenu];
On 2013/08/15 04:39:23, tapted wrote:
> nit: move closer to first use

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:73: if (appId_) {
On 2013/08/15 04:39:23, tapted wrote:
> 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]);

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:101: appId_.reset();
On 2013/08/15 04:39:23, tapted wrote:
> nit: move this up? (i.e. swap with line 99). It's more relevant to the initial
> `if` than the loop below.

Done.

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:111: @end  //
@implementation AppMenuController
On 2013/08/15 04:39:23, tapted wrote:
> nit: I think current trend is to not have comments after @end

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm
(right):

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/08/15 04:39:23, tapted wrote:
> nit: no (c)

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:15: #import
"chrome/common/chrome_switches.h"
On 2013/08/15 04:39:23, tapted wrote:
> nit: #include

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:23:
AppMenuControllerBrowserTest() {}
On 2013/08/15 04:39:23, tapted wrote:
> Since we're C++, you need to initialize data members

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:26: // Start
two apps and wait for them to be launched.
On 2013/08/15 04:39:23, tapted wrote:
> nit: comment before declaration?

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:38: void
CheckAppMenu(const extensions::Extension* app) {
On 2013/08/15 04:39:23, tapted wrote:
> nit: CheckHasAppMenu? You could declare these functions `const` too I think

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:59: NSMenu*
main_menu_;
On 2013/08/15 04:39:23, tapted wrote:
> Maybe better without this data member, since it raises ownership questions.

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:61: };
On 2013/08/15 04:39:23, tapted wrote:
> nit: private: DISALLOW_COPY_AND_ASSIGN(..)  (+ destructor that I think it will
> force you to have)

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/browser/ui/cocoa/ap...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac_browsertest.mm:103: }
On 2013/08/15 04:39:23, tapted wrote:
> Maybe a test for unloading/uninstalling an extension with windows open?

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_browser_ui.g...
chrome/chrome_browser_ui.gypi:439:
'browser/ui/cocoa/apps/app_menu_controller_mac.h',
On 2013/08/15 04:39:23, tapted wrote:
> .h before .mm

Done.

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/22867009/diff/5001/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:1563:
'browser/ui/cocoa//autofill/autofill_dialog_cocoa_browsertest.mm',
On 2013/08/15 04:39:23, tapted wrote:
> hm.. why is there a double-slash on this line...  let's clean that up while
> we're here (since otherwise our sorting is technically wrong :p)

Done.

[tapted, 2013-08-19 00:35:56]

>
https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
> chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:59: if ([notification
> name] == NSWindowDidBecomeMainNotification) {
> On 2013/08/15 04:39:23, tapted wrote:
> > need to use isEqualToString: here - same below.
> 
> 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?

Yeah. Pretty sure the precedent in app_controller_mac.mm is a mistake. I've been
picked up on doing ~pointer comparisons in objective C in review in the past.

https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:56: #import
"chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h"
as you raised, this would be better under chrome/browser/ui/apps/ -- this makes
it more aligned with the direction of chrome/browser/ui/app_list/.

Ben has suggested chrome/browser/ui/apps/cocoa/

https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:659: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAppShims))
Still a bit nervous about regressions - many who will be affected will have
already enabled app shims in their flags. Might be nice to have the follow-up CL
that restores behaviour of copy-paste and Cmd+W ready to go before landing this.

[tapted, 2013-08-19 00:43:11]

On 2013/08/19 00:35:56, tapted wrote:
>
https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
> chrome/browser/app_controller_mac.mm:56: #import
> "chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h"
> as you raised, this would be better under chrome/browser/ui/apps/ -- this
makes
> it more aligned with the direction of chrome/browser/ui/app_list/.
> 
> Ben has suggested chrome/browser/ui/apps/cocoa/

Actually, native_app_window_cocoa.h moved from

"chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h"
to --> "chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h"
in http://crrev.com/21344002 with a comment "The platform specific
implementations should also move to apps/"

So maybe how you have app_menu_controller_mac.h now is correct.

**pokes benwells@: to clarify whether the final resting place for
native_app_window foo should be 
"chrome/browser/ui/apps/cocoa/native_app_window_cocoa.h" or elsewhere.

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

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

https://codereview.chromium.org/22867009/diff/1/chrome/browser/ui/cocoa/apps/...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:54:
apps::ShellWindowRegistry::GetShellWindowForNativeWindowAnyProfile(
On 2013/08/16 02:50:41, jackhou1 wrote:
> On 2013/08/15 04:39:23, tapted wrote:
> > 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?
> 
> I don't think so. ShellWindow::Close closes the native window first. Profile
> deletion doesn't delete the Profile instance.

Changed to use isEqualToString

https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:659: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableAppShims))
On 2013/08/19 00:35:57, tapted wrote:
> Still a bit nervous about regressions - many who will be affected will have
> already enabled app shims in their flags. Might be nice to have the follow-up
CL
> that restores behaviour of copy-paste and Cmd+W ready to go before landing
this.

Agreed.

[benwells, 2013-08-19 06:15:26]

On 2013/08/19 00:43:11, tapted wrote:
> On 2013/08/19 00:35:56, tapted wrote:
> >
>
https://codereview.chromium.org/22867009/diff/23001/chrome/browser/app_contro...
> > chrome/browser/app_controller_mac.mm:56: #import
> > "chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h"
> > as you raised, this would be better under chrome/browser/ui/apps/ -- this
> makes
> > it more aligned with the direction of chrome/browser/ui/app_list/.
> > 
> > Ben has suggested chrome/browser/ui/apps/cocoa/
> 
> Actually, native_app_window_cocoa.h moved from
> 
> "chrome/browser/ui/cocoa/extensions/native_app_window_cocoa.h"
> to --> "chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h"
> in http://crrev.com/21344002 with a comment "The platform specific
> implementations should also move to apps/"
> 
> So maybe how you have app_menu_controller_mac.h now is correct.
> 
> **pokes benwells@: to clarify whether the final resting place for
> native_app_window foo should be 
> "chrome/browser/ui/apps/cocoa/native_app_window_cocoa.h" or elsewhere.

Where it is now is fine :)

[jackhou1, 2013-08-21 02:51:33]

rsesek, could you please review for OWNERS of chrome/browser/ui/cocoa/

[Robert Sesek, 2013-08-21 14:21:49]

This looks pretty good. I have a few questions, though;

- Is this only creating a single main menu item for the app?
- What is its contents?
- Should the Edit and Help menus be removed? They are pretty universal.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.h:87: base::scoped_nsobject<AppMenuController>
appMenuController_;
Move this to be under historyMenuBridge_.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: -
(void)unregisterEventHandlers;
nit: please document. Is this unnecessary if you just do it in -dealloc?

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:40: -
(void)buildAppMenuItems {
Method order should be:

- Any class methods (none here)
- init
- dealloc
- public methods in header order
- private methods in declaration order

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:87: [[notification name]
isEqualToString:NSWindowDidResignMainNotification] ||
If you pull [notification name] out into a local, this may wrap a little better.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:103: appId_.reset([appId
retain]);
Strings should be -copy'ed

[jackhou1, 2013-08-21 23:54:48]

> - Is this only creating a single main menu item for the app?

Yup, I wanted to check that this is the right approach first. I'll get the
followup CLs reviewed before committing this.

> - What is its contents?

The first followup CL adds Quit. I intend to add Hide later, and also About if
there's a sensible behaviour for it.

> - Should the Edit and Help menus be removed? They are pretty universal.

Keyboard shortcuts won't work without a corresponding menu item, so the second
followup CL adds File, Edit, and Window. Each containing only the items relevant
to a packaged app.

I'm not sure how the Help should look yet since packaged apps don't have
individual help documentation.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.h:87: base::scoped_nsobject<AppMenuController>
appMenuController_;
On 2013/08/21 14:21:49, rsesek wrote:
> Move this to be under historyMenuBridge_.

Done.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: -
(void)unregisterEventHandlers;
On 2013/08/21 14:21:49, rsesek wrote:
> nit: please document. Is this unnecessary if you just do it in -dealloc?

It's called in AppController-applicationWillTerminate. tapted@ suggested having
this so that unregistering is explicit since appMenuController_.reset() does not
dealloc immediately.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:40: -
(void)buildAppMenuItems {
On 2013/08/21 14:21:49, rsesek wrote:
> Method order should be:
> 
> - Any class methods (none here)
> - init
> - dealloc
> - public methods in header order
> - private methods in declaration order

Done.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:87: [[notification name]
isEqualToString:NSWindowDidResignMainNotification] ||
On 2013/08/21 14:21:49, rsesek wrote:
> If you pull [notification name] out into a local, this may wrap a little
better.

Done.

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.mm:103: appId_.reset([appId
retain]);
On 2013/08/21 14:21:49, rsesek wrote:
> Strings should be -copy'ed

Done.

[tapted, 2013-08-22 00:27:01]

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: -
(void)unregisterEventHandlers;
On 2013/08/21 23:54:48, jackhou1 wrote:
> On 2013/08/21 14:21:49, rsesek wrote:
> > nit: please document. Is this unnecessary if you just do it in -dealloc?
> 
> It's called in AppController-applicationWillTerminate. tapted@ suggested
having
> this so that unregistering is explicit since appMenuController_.reset() does
not
> dealloc immediately.

Hehe - dealing with weird OjbC lifetime issues in UI code may have made me
overly paranoid ;). dealloc should be safe here, since nothing else should be
retaining the AppMenuController. And probably better, since NSNotificationCenter
doesn't retain observers so unregistering in dealloc makes it clearer we are
avoiding zombie observers.

[jackhou1, 2013-08-22 01:44:02]

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/41001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:26: -
(void)unregisterEventHandlers;
On 2013/08/22 00:27:02, tapted wrote:
> On 2013/08/21 23:54:48, jackhou1 wrote:
> > On 2013/08/21 14:21:49, rsesek wrote:
> > > nit: please document. Is this unnecessary if you just do it in -dealloc?
> > 
> > It's called in AppController-applicationWillTerminate. tapted@ suggested
> having
> > this so that unregistering is explicit since appMenuController_.reset() does
> not
> > dealloc immediately.
> 
> Hehe - dealing with weird OjbC lifetime issues in UI code may have made me
> overly paranoid ;). dealloc should be safe here, since nothing else should be
> retaining the AppMenuController. And probably better, since
NSNotificationCenter
> doesn't retain observers so unregistering in dealloc makes it clearer we are
> avoiding zombie observers.

Done.

[Robert Sesek, 2013-08-22 16:22:44]

LGTM w/ nits

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:436: [self unregisterEventHandlers];
nit: blank line after

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:18: @interface
AppMenuController : NSObject {
This may be better named "AppShimMenuController" since it's so similar to
AppController.

[jackhou1, 2013-08-23 02:36:41]

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/app_contro...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/app_contro...
chrome/browser/app_controller_mac.mm:436: [self unregisterEventHandlers];
On 2013/08/22 16:22:45, rsesek wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/ui/cocoa/a...
File chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h (right):

https://codereview.chromium.org/22867009/diff/56001/chrome/browser/ui/cocoa/a...
chrome/browser/ui/cocoa/apps/app_menu_controller_mac.h:18: @interface
AppMenuController : NSObject {
On 2013/08/22 16:22:45, rsesek wrote:
> This may be better named "AppShimMenuController" since it's so similar to
> AppController.

Done.

[jackhou1, 2013-08-23 05:34:10]

thakis, please review for OWNERS in chrome/browser/

[Nico, 2013-08-23 13:39:23]

chrome/ lgtm

[commit-bot: I haz the power, 2013-08-30 05:07:48]

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

[commit-bot: I haz the power, 2013-08-30 09:03:23]

Change committed as 220540