Prevent extension and bookmark popups from interfering with close window/tab shortcuts.

Prevent extension and bookmark popups from interfering with close window/tab shortcuts.

When a balloon popup is open, the target of the close window/tab keyboard shortcut is still the main browser window.  Update our custom menu item rejiggering code to pay attention to the main window, rather than to the popup window, when a popup is open.

BUG=95169
TEST=(see bug)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107666

[Ilya Sherman, 2011-10-15 04:31:38]

http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:415: [[NSApp keyWindow]
isKindOfClass:[ChromeEventProcessingWindow class]])) {
This seems a little hacky, but I wasn't sure of a better way to check whether
the key window is one that we're uninterested in.  Do you know why we try to
care about the key window, rather than just always querying the main window? 
Off the top of my head, I can't think of any cases where querying the main
window would do the wrong thing.

http://codereview.chromium.org/8302021/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/8302021/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_controller.mm:1002: break;
I think this code is obsolete.  It definitely only does part of what the comment
says it does, whereas |fixCloseMenuItemKeyEquivalents| in app_controller_mac
does both.

[Ilya Sherman, 2011-10-19 00:57:04]

+Scott

[Mark Mentovai, 2011-10-19 01:14:59]

LGTM

[commit-bot: I haz the power, 2011-10-19 02:37:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8302021/1

[Scott Hess - ex-Googler, 2011-10-19 02:43:41]

http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:415: [[NSApp keyWindow]
isKindOfClass:[ChromeEventProcessingWindow class]])) {
On 2011/10/15 04:31:38, Ilya Sherman wrote:
> This seems a little hacky, but I wasn't sure of a better way to check whether
> the key window is one that we're uninterested in.  Do you know why we try to
> care about the key window, rather than just always querying the main window? 
> Off the top of my head, I can't think of any cases where querying the main
> window would do the wrong thing.

I agree - I think that this is EXACTLY the kind of case where the distinction
between key and main window is useful.  The canonical example is a find panel. 
The main windows and key windows are the same until you pull up a find panel, at
which point the key window is the find panel while the document window is still
the main window.

http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:420: tabController = [self
mainWindowTabController];
Wow, this whole thing seems hokey.  AFAICT, the only caller of the two helpers
is this function, and it does this additional work on top of all that.  I think
it would make sense to just wrap the entire thing up as a -hasTabsThingyHelper.

Something that would be 23% more tricky would be to have the window which cares
about this define a -hasTabs: action method, and then instead of putzing around,
just use hasTabs = !![NSApp targetForAction:@selector(hasTabs:)];.  It'll fall
through the key window to the main window as desired.

Another option would be to unwire Close Tab from -commandDispatch: and rewire it
to -closeTab: (which can forward to -commandDispatch:), and key off that.

[Ilya Sherman, 2011-10-21 07:23:13]

On 2011/10/19 02:43:41, shess wrote:
>
http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
> File chrome/browser/app_controller_mac.mm (right):
> 
>
http://codereview.chromium.org/8302021/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:415: [[NSApp keyWindow]
> isKindOfClass:[ChromeEventProcessingWindow class]])) {
> On 2011/10/15 04:31:38, Ilya Sherman wrote:
> > This seems a little hacky, but I wasn't sure of a better way to check
whether
> > the key window is one that we're uninterested in.  Do you know why we try to
> > care about the key window, rather than just always querying the main window?

> > Off the top of my head, I can't think of any cases where querying the main
> > window would do the wrong thing.
> 
> I agree - I think that this is EXACTLY the kind of case where the distinction
> between key and main window is useful.  The canonical example is a find panel.

> The main windows and key windows are the same until you pull up a find panel,
at
> which point the key window is the find panel while the document window is
still
> the main window.

Argh, I figured out why we are so careful to check the key window rather than
the main window: the 'About Chrome' window is key but not main.  That basically
also breaks your other two suggestions, afaict :(

[Scott Hess - ex-Googler, 2011-10-21 14:40:23]

On 2011/10/21 07:23:13, Ilya Sherman wrote:
> Argh, I figured out why we are so careful to check the key window rather than
> the main window: the 'About Chrome' window is key but not main.  That
basically
> also breaks your other two suggestions, afaict :(

That seems kinda weird, since an About window is a floating app-wide thing not
associated with a particular document.  Is it an NSPanel instead of an NSWindow?
 Maybe that's wrong.

Still, doesn't your change already suffer from this?  The about panel will
return nil from the key-window call and fall through to the main-window variant,
won't it?  I think what we want is:
 - chrome-window-key gives close-tab/close-window magic.
 - bubble-key-chrome-window-main gives close-tab/close-window magic.
 - else fall back to regular-close-window.

I think the special case is whether the key window is a bubble.  How about a
test more like "Is the main window a tabbed window, and is the key window the
same window, or a child window of the same window?"  This gets across the sense
that the child window is not independently operational at the window level.

[It may also make sense to actively disable window-level operations in the
child-window's implementation, but AFAICT all the actual dispatch actually goes
to the right place already, as I can't seem to miniaturize a bubble.]

[Ilya Sherman, 2011-10-22 04:42:04]

On 2011/10/21 14:40:23, shess wrote:
> On 2011/10/21 07:23:13, Ilya Sherman wrote:
> > Argh, I figured out why we are so careful to check the key window rather
than
> > the main window: the 'About Chrome' window is key but not main.  That
> basically
> > also breaks your other two suggestions, afaict :(
> 
> That seems kinda weird, since an About window is a floating app-wide thing not
> associated with a particular document.  Is it an NSPanel instead of an
NSWindow?
>  Maybe that's wrong.

Yeah, I think it is an NSPanel.  It seems to be consistent with how other apps
behave, at least in the sense that pressing Escape closes the window.  I'm not
sure if it should just be a regular window that handles Escape specially,
though.

> Still, doesn't your change already suffer from this?  The about panel will
> return nil from the key-window call and fall through to the main-window
variant,
> won't it?

No, it would only fall through if the key window was nil or a bubble.  But I
like your suggestion to check whether the key window is a child (descendant) of
the main window, rather than baking in special knowledge of bubbles and their
ilk into the app controller.  Updated the code to do that, and ripped out the
silly helper methods that made things more confusing than they needed to be.

[Scott Hess - ex-Googler, 2011-10-25 23:40:59]

LGTM, except perhaps the change to only expect NSMenuItem*, that seems sketchy
to me.

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:183: }
I think this should probably be -isDescendentOfWindow:.

That said, is it justified?  I'd be uncomfortable knowing that we have child
windows who have child windows.  Lot's of scope for weak references.

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:410: if (!window || [window
isDescendantOf:mainWindow]) {
Wow, can window be nil and mainWindow not nil?  Crazy, if true.

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:988: -
(BOOL)validateUserInterfaceItem:(NSMenuItem*)item {
I'm not sure this is only NSMenuItem*.  If it definitely is, then the
-isKindOfClass: tests can probably be dispensed with.  But their presence makes
me suspect that it isn't definite.

[Ilya Sherman, 2011-10-26 23:22:40]

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:183: }
On 2011/10/25 23:40:59, shess wrote:
> I think this should probably be -isDescendentOfWindow:.
> 
> That said, is it justified?  I'd be uncomfortable knowing that we have child
> windows who have child windows.  Lot's of scope for weak references.

I just added this because I wasn't sure if we could have child windows that have
their own child windows.  I'm happy to just check parentWindow: if that 
shouldn't be possible.

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:410: if (!window || [window
isDescendantOf:mainWindow]) {
On 2011/10/25 23:40:59, shess wrote:
> Wow, can window be nil and mainWindow not nil?  Crazy, if true.

Apparently, assuming the comment that used to be here (and is still here, in
expanded form) is not a lie.

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/8302021/diff/8001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:988: -
(BOOL)validateUserInterfaceItem:(NSMenuItem*)item {
On 2011/10/25 23:40:59, shess wrote:
> I'm not sure this is only NSMenuItem*.  If it definitely is, then the
> -isKindOfClass: tests can probably be dispensed with.  But their presence
makes
> me suspect that it isn't definite.

Oh, I missed those below.  It seems weird that this might not be an NSMenuItem,
but I'll change it back and use the isKindOfClass test to be safe.

[commit-bot: I haz the power, 2011-10-27 22:57:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/8302021/15001

[commit-bot: I haz the power, 2011-10-28 00:43:46]

Change committed as 107666