Fixed "Close Window" shortcut in File Menu for Hosted/Packaged apps.

Fixed "Close Window" shortcut in File Menu for Hosted/Packaged apps.

There is a bug where the "Close Window" shortcut for hosted/packaged
apps would disappear after focusing the File menu in a Chrome browser.
This is because the shortcuts had the same key equivalent, and OS X was
choosing to only display one of them. This CL fixes this by keeping a
reference to the conflicting menu items and enabling/disabling the
shortcuts on focus.

BUG=455067
Committed: https://crrev.com/f9dffe3fa614d24dae753b47049d04cff721e121
Cr-Commit-Position: refs/heads/master@{#314690}

[mitchellj, 2015-02-04 03:34:37]

mitchelljones@chromium.org changed reviewers:
+ jackhou@chromium.org

[mitchellj, 2015-02-04 04:37:13]

mitchelljones@chromium.org changed reviewers:
+ tapted@chromium.org

[mitchellj, 2015-02-04 04:37:14]

tapted@ could you please take a look at patchset 2?

[jackhou1, 2015-02-04 04:40:04]

lgtm with nit

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:39:
base::scoped_nsobject<NSMenuItem> closeWindowDuplicateMenuItem_;
Put these above "// Menu items for the..." and add a comment like:
// The "Close Window" item needs special treatment.

[mitchellj, 2015-02-04 04:46:27]

New patchsets have been uploaded after l-g-t-m from jackhou@chromium.org

[tapted, 2015-02-04 05:02:50]

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

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:416:
[closeTabSourceMenuItem_ setKeyEquivalent:@""];
this logic is pretty close to what DoppelgangerMenuItem does -- is there a way
to re-use that? Maybe just pass a nil controller and, in initWithController,
copy the target from |sourceItem_| if the controller is nil.

[jackhou1, 2015-02-04 05:46:28]

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

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:416:
[closeTabSourceMenuItem_ setKeyEquivalent:@""];
On 2015/02/04 05:02:49, tapted wrote:
> this logic is pretty close to what DoppelgangerMenuItem does -- is there a way
> to re-use that? Maybe just pass a nil controller and, in initWithController,
> copy the target from |sourceItem_| if the controller is nil.

The "Close Window" item kind of has two source items: we get the string and
target from "Close Window", and clear the key equivalent in "Close Tab".

Mitchell, you could try adding another initializer to AppShimMenuDoppelganger.
Something like:
- (id)initWithMenuItem:(NSMenuItem*) sourceItem:(NSMenuItem*);

Then you pass in:
GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_WINDOW) copy]
and
GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_TAB) retain]

[mitchellj, 2015-02-04 22:33:40]

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:39:
base::scoped_nsobject<NSMenuItem> closeWindowDuplicateMenuItem_;
On 2015/02/04 04:40:04, jackhou1 wrote:
> Put these above "// Menu items for the..." and add a comment like:
> // The "Close Window" item needs special treatment.

Done.

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

https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:416:
[closeTabSourceMenuItem_ setKeyEquivalent:@""];
On 2015/02/04 05:46:28, jackhou1 wrote:
> On 2015/02/04 05:02:49, tapted wrote:
> > this logic is pretty close to what DoppelgangerMenuItem does -- is there a
way
> > to re-use that? Maybe just pass a nil controller and, in initWithController,
> > copy the target from |sourceItem_| if the controller is nil.
> 
> The "Close Window" item kind of has two source items: we get the string and
> target from "Close Window", and clear the key equivalent in "Close Tab".
> 
> Mitchell, you could try adding another initializer to AppShimMenuDoppelganger.
> Something like:
> - (id)initWithMenuItem:(NSMenuItem*) sourceItem:(NSMenuItem*);
> 
> Then you pass in:
> GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_WINDOW) copy]
> and
> GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_TAB) retain]

Good idea, this seems cleaner. Do you think the function should take these
NSMenuItems directly though? Or the tags?

I was thinking something like

- (id)initWithMenuTag:(NSInteger)menuTag
              itemTag:(NSInteger)itemTag
        sourceMenuTag:(NSInteger)sourceMenuTag
        sourceItemTag:(NSInteger)sourceItemTag
        keyEquivalent:(NSString*)keyEquivalent;

And I could just mention something along the lines: "Copy the menu item given
|menuTag| and |itemTag|. Retain the source item given |sourceMenuTag| and
|sourceItemTag|."

[tapted, 2015-02-04 22:49:59]

On 2015/02/04 22:33:40, mitchellj wrote:
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):
> 
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:39:
> base::scoped_nsobject<NSMenuItem> closeWindowDuplicateMenuItem_;
> On 2015/02/04 04:40:04, jackhou1 wrote:
> > Put these above "// Menu items for the..." and add a comment like:
> > // The "Close Window" item needs special treatment.
> 
> Done.
> 
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):
> 
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:416:
> [closeTabSourceMenuItem_ setKeyEquivalent:@""];
> On 2015/02/04 05:46:28, jackhou1 wrote:
> > On 2015/02/04 05:02:49, tapted wrote:
> > > this logic is pretty close to what DoppelgangerMenuItem does -- is there a
> way
> > > to re-use that? Maybe just pass a nil controller and, in
initWithController,
> > > copy the target from |sourceItem_| if the controller is nil.
> > 
> > The "Close Window" item kind of has two source items: we get the string and
> > target from "Close Window", and clear the key equivalent in "Close Tab".
> > 
> > Mitchell, you could try adding another initializer to
AppShimMenuDoppelganger.
> > Something like:
> > - (id)initWithMenuItem:(NSMenuItem*) sourceItem:(NSMenuItem*);
> > 
> > Then you pass in:
> > GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_WINDOW) copy]
> > and
> > GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_TAB) retain]
> 
> Good idea, this seems cleaner. Do you think the function should take these
> NSMenuItems directly though? Or the tags?
> 
> I was thinking something like
> 
> - (id)initWithMenuTag:(NSInteger)menuTag
>               itemTag:(NSInteger)itemTag
>         sourceMenuTag:(NSInteger)sourceMenuTag
>         sourceItemTag:(NSInteger)sourceItemTag
>         keyEquivalent:(NSString*)keyEquivalent;
> 
> And I could just mention something along the lines: "Copy the menu item given
> |menuTag| and |itemTag|. Retain the source item given |sourceMenuTag| and
> |sourceItemTag|."

sgtm. Although if menuTag == sourceMenuTag for all current users I would just
have one argument. the `source` item should come first too. E.g.

- (id)initWithMenuTag:(NSInteger)menuTag
        sourceItemTag:(NSInteger)sourceItemTag
        targetItemTag:(NSInteger)targetItemTag
        keyEquivalent:(NSString*)keyEquivalent;

[mitchellj, 2015-02-04 23:13:31]

On 2015/02/04 22:49:59, tapted wrote:
> On 2015/02/04 22:33:40, mitchellj wrote:
> >
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> > File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h (right):
> > 
> >
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> > chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.h:39:
> > base::scoped_nsobject<NSMenuItem> closeWindowDuplicateMenuItem_;
> > On 2015/02/04 04:40:04, jackhou1 wrote:
> > > Put these above "// Menu items for the..." and add a comment like:
> > > // The "Close Window" item needs special treatment.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> > File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/898543008/diff/20001/chrome/browser/ui/cocoa/...
> > chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:416:
> > [closeTabSourceMenuItem_ setKeyEquivalent:@""];
> > On 2015/02/04 05:46:28, jackhou1 wrote:
> > > On 2015/02/04 05:02:49, tapted wrote:
> > > > this logic is pretty close to what DoppelgangerMenuItem does -- is there
a
> > way
> > > > to re-use that? Maybe just pass a nil controller and, in
> initWithController,
> > > > copy the target from |sourceItem_| if the controller is nil.
> > > 
> > > The "Close Window" item kind of has two source items: we get the string
and
> > > target from "Close Window", and clear the key equivalent in "Close Tab".
> > > 
> > > Mitchell, you could try adding another initializer to
> AppShimMenuDoppelganger.
> > > Something like:
> > > - (id)initWithMenuItem:(NSMenuItem*) sourceItem:(NSMenuItem*);
> > > 
> > > Then you pass in:
> > > GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_WINDOW) copy]
> > > and
> > > GetItemByTag(IDC_FILE_MENU, IDC_CLOSE_TAB) retain]
> > 
> > Good idea, this seems cleaner. Do you think the function should take these
> > NSMenuItems directly though? Or the tags?
> > 
> > I was thinking something like
> > 
> > - (id)initWithMenuTag:(NSInteger)menuTag
> >               itemTag:(NSInteger)itemTag
> >         sourceMenuTag:(NSInteger)sourceMenuTag
> >         sourceItemTag:(NSInteger)sourceItemTag
> >         keyEquivalent:(NSString*)keyEquivalent;
> > 
> > And I could just mention something along the lines: "Copy the menu item
given
> > |menuTag| and |itemTag|. Retain the source item given |sourceMenuTag| and
> > |sourceItemTag|."
> 
> sgtm. Although if menuTag == sourceMenuTag for all current users I would just
> have one argument. the `source` item should come first too. E.g.
> 
> - (id)initWithMenuTag:(NSInteger)menuTag
>         sourceItemTag:(NSInteger)sourceItemTag
>         targetItemTag:(NSInteger)targetItemTag
>         keyEquivalent:(NSString*)keyEquivalent;

Done. Could you please take a look at patch set 4?

[tapted, 2015-02-04 23:56:00]

lgtm % nits

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:190:
sourceItem_.reset([GetItemByTag(menuTag, sourceItemTag) retain]);
DCHECK(menuItem_);
DCHECK(sourceItem_);

ObjC likes to silently fail when calling methods on nil objects, so it's
important to check in case the model changes from under us.

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:192: resourceId_ =
0;
nit: this is redundant for ObjC (it's guaranteed to be default-initialized
already)

[mitchellj, 2015-02-05 00:01:00]

New patchsets have been uploaded after l-g-t-m from tapted@chromium.org

[mitchellj, 2015-02-05 00:01:34]

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:190:
sourceItem_.reset([GetItemByTag(menuTag, sourceItemTag) retain]);
On 2015/02/04 23:55:59, tapted wrote:
> DCHECK(menuItem_);
> DCHECK(sourceItem_);
> 
> ObjC likes to silently fail when calling methods on nil objects, so it's
> important to check in case the model changes from under us.

Done.

https://codereview.chromium.org/898543008/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:192: resourceId_ =
0;
On 2015/02/04 23:55:59, tapted wrote:
> nit: this is redundant for ObjC (it's guaranteed to be default-initialized
> already)

Done.

[mitchellj, 2015-02-05 00:01:54]

The CQ bit was checked by mitchelljones@chromium.org

[commit-bot: I haz the power, 2015-02-05 00:02:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/898543008/80001

[commit-bot: I haz the power, 2015-02-05 00:51:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-05 00:53:11]

Patchset 5 (id:??) landed as
https://crrev.com/f9dffe3fa614d24dae753b47049d04cff721e121
Cr-Commit-Position: refs/heads/master@{#314690}