Ensure the previously active browser window gets the focus after a browser window is closed on OSX.

Ensure the previously active browser window gets the focus after a browser window is closed on OSX.

Also cleaned up extra orderOut: call since browser shutdown process has improved since that line was added. See crbug 23959 for history.

BUG=112038
TEST=Manually open tabbed windows and panels and close them. Watch focus go back to last active window.


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

[jennb, 2012-02-02 23:47:25]

pinkerton@, viettrungluu@: I added you as reviewers because your names are
associated with the orderOut logic.


Thanks,
Jenn

[Dmitry Titov, 2012-02-03 03:01:45]

Will it also fix the mentioned bug if instead of closing the browser window it
was minimized?

[jennb, 2012-02-03 04:28:53]

No, minimized is a different issue. Daniel is working on that separately.

On Thu, Feb 2, 2012 at 7:01 PM, <dimich@chromium.org> wrote:

> Will it also fix the mentioned bug if instead of closing the browser
> window it
> was minimized?
>
>
http://codereview.chromium.**org/9310075/<http://codereview.chromium.org/9310...
>

[Dmitry Titov, 2012-02-03 08:44:21]

lgtm for panels.
Need either pink@ or viettrungluu@ to take a look though.

[pink (ping after 24hrs), 2012-02-03 14:24:55]

Fix the description, OXS -> OSX.

I'm ok with the changes themselves, but not yet convinced we want this behavior.

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

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:550: [BrowserWindowUtils
selectPreviousActiveBrowserWindow:browser_.get()];
What is the behavior of other applications on Mac with regard to panels and
window focus? Why are we usurping what the OS wants to do?

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:204: return
browser->window()->Activate();
you're returning a value from a void method. this doesn't trip the compiler?

[Mark Mentovai, 2012-02-03 16:22:53]

Jenn, can you please explain why you think it’s desirable to go against the
standard system behavior?

[jennb, 2012-02-03 18:12:06]

To address Mark's question, we are actually trying to make behavior look like
'standard system behavior' where the previously active application window gains
focus after the current active application window is closed. If panel browser
windows were not using a higher priority NSWindowLevel, the panels would not
interfere with the normal selection of window order. However, we need panels to
be 'always-in-front' and the only way to achieve that is to use a different
NSWindowLevel. 

Hope that clarifies,
Jenn

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

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:550: [BrowserWindowUtils
selectPreviousActiveBrowserWindow:browser_.get()];
On 2012/02/03 14:24:55, pink wrote:
> What is the behavior of other applications on Mac with regard to panels and
> window focus? Why are we usurping what the OS wants to do?

Mac's window ordering selects from among the application's windows so the panel
browser windows do not affect other applications.

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:204: return
browser->window()->Activate();
On 2012/02/03 14:24:55, pink wrote:
> you're returning a value from a void method. this doesn't trip the compiler?

return [expression] returns the value of expression. In this case, that's void
and this function returns void, so the compiler has no issues. I can separate
the return to a different line to avoid concerns. Was trying to be compact.

[pink (ping after 24hrs), 2012-02-03 18:42:22]

lgtm

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

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:550: [BrowserWindowUtils
selectPreviousActiveBrowserWindow:browser_.get()];
On 2012/02/03 18:12:06, jennb wrote:
> On 2012/02/03 14:24:55, pink wrote:
> > What is the behavior of other applications on Mac with regard to panels and
> > window focus? Why are we usurping what the OS wants to do?
> 
> Mac's window ordering selects from among the application's windows so the
panel
> browser windows do not affect other applications.

I meant what do other applications do in this similar circumstance?

[pink (ping after 24hrs), 2012-02-03 18:42:37]

Sorry, didn't mean to give LGTM.

[jennb, 2012-02-03 19:20:30]

I should also mention that I'm open to advice from Mac experts on better
alternatives to fixing this.

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

http://codereview.chromium.org/9310075/diff/3001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_controller.mm:550: [BrowserWindowUtils
selectPreviousActiveBrowserWindow:browser_.get()];
On 2012/02/03 18:42:23, pink wrote:
> On 2012/02/03 18:12:06, jennb wrote:
> > On 2012/02/03 14:24:55, pink wrote:
> > > What is the behavior of other applications on Mac with regard to panels
and
> > > window focus? Why are we usurping what the OS wants to do?
> > 
> > Mac's window ordering selects from among the application's windows so the
> panel
> > browser windows do not affect other applications.
> 
> I meant what do other applications do in this similar circumstance?

Most applications don't modify window levels. When they do, they want to get a
high priority window so it naturally should take precedence in window ordering.
We're using the higher priority without wanting to take precedence, making us a
unique case.

[Dmitry Titov, 2012-02-03 19:49:31]

Just to add some background: the use case we are after is a 'chat on a side'
which is normally going in parallel with some main activity for the user - for
example I can have some lazy chat with my friend while doing my homework. Having
small chat window on top in z-order is useful then because I can easily
'monitor' it and it does not get lost - however it is incorrect to force focus
into it when other windows change their order, since it is not an 'important
window that has to be dealt with first". It seems OSX treats always-on-top
windows that "canBecomeKey" as "more important" and prefers them in the next key
window selection. We don't really want that...

[pink (ping after 24hrs), 2012-02-06 14:31:40]

Ok, LGTM. Just make sure this doesn't impact any other window layering in the
app.

[commit-bot: I haz the power, 2012-02-06 17:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/9310075/9001

[commit-bot: I haz the power, 2012-02-06 20:06:13]

Change committed as 120593

[Scott Hess - ex-Googler, 2012-02-09 22:51:54]

Sorry for the late comments, was OOT.

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:193: return;
Are you sure about the -isActive test?  You would presumably want the right
window to be key if a page closes itself using close() while the app is not
active.  What you don't want is for the app to force the window to the front,
which is what Activate() will do.  Will -becomeKeyWindow to the native window
handle work, instead of Activate()?

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:208: }
This will subvert the ordering logic if there are any non-browser windows on the
stack (like Task Manager - yes, I checked, this changed).  I think it would be
more reasonable to iterate the app's window list, and pick the first one which
-canBecomeKeyWindow && isNotPanel.  You could PROBABLY do this by doing
something like setting a global flag for your panels to return from
-canBecomeKeyWindow, then calling something like:

  [[NSApp makeWindowsPerform:@selector(canBecomeKeyWindow) inOrder:YES]
makeKeyWindow];

though you might have to order the outbound window out before calling that.

By having a "global" above I really mean something like a method on
BrowserCrApplication to let your panels return NO to -canBecomeKeyWindow during
this, though I don't know how your panels are implemented.

I was going to suggest having your panels return NO to -canBecomeKeyWindow
scoped to the close, but I don't think that'll work if this is the last browser
window open (in which case one of your panels should get key, I guess?).

[jennb, 2012-02-09 23:25:24]

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:193: return;
On 2012/02/09 22:51:54, shess wrote:
> Are you sure about the -isActive test?  You would presumably want the right
> window to be key if a page closes itself using close() while the app is not
> active.  What you don't want is for the app to force the window to the front,
> which is what Activate() will do.  Will -becomeKeyWindow to the native window
> handle work, instead of Activate()?

My thinking is that if Chrome is not active, then we'll let the system do it's
usual thing.

http://codereview.chromium.org/9310075/diff/9001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_utils.mm:208: }
On 2012/02/09 22:51:54, shess wrote:
> This will subvert the ordering logic if there are any non-browser windows on
the
> stack (like Task Manager - yes, I checked, this changed).  I think it would be
> more reasonable to iterate the app's window list, and pick the first one which
> -canBecomeKeyWindow && isNotPanel.  You could PROBABLY do this by doing
> something like setting a global flag for your panels to return from
> -canBecomeKeyWindow, then calling something like:
> 
>   [[NSApp makeWindowsPerform:@selector(canBecomeKeyWindow) inOrder:YES]
> makeKeyWindow];
> 
> though you might have to order the outbound window out before calling that.
> 
> By having a "global" above I really mean something like a method on
> BrowserCrApplication to let your panels return NO to -canBecomeKeyWindow
during
> this, though I don't know how your panels are implemented.
> 
> I was going to suggest having your panels return NO to -canBecomeKeyWindow
> scoped to the close, but I don't think that'll work if this is the last
browser
> window open (in which case one of your panels should get key, I guess?).

Thanks for pointing out the non-browser window case. I wasn't aware of such
windows. I want the last active Chrome window to become key, whether it's a
browser or non-browser window. If a Panel was last active, it is allowed to
become key so I can't rule out all Panels.

Perhaps I can use a combination of iterating the app's window list and comparing
against browserlist's last active list to find an alternate solution. Will work
on it more.

[Scott Hess - ex-Googler, 2012-02-10 00:11:20]

On 2012/02/09 23:25:24, jennb wrote:
> Thanks for pointing out the non-browser window case. I wasn't aware of such
> windows. I want the last active Chrome window to become key, whether it's a
> browser or non-browser window. If a Panel was last active, it is allowed to
> become key so I can't rule out all Panels.
> 
> Perhaps I can use a combination of iterating the app's window list and
comparing
> against browserlist's last active list to find an alternate solution. Will
work
> on it more.

Hmm.

Since windows can be closed without becoming active (click close button while
window is not key, or calling close from script), in the limit this ends up
requiring that you maintain a set of windows ordered by when they last became
key.  Which is doable, but I think maybe not preferable.  I can see your
reasoning for why you might want the panel to become active again if you went
from a panel to a browser window and then closed the browser window, but at that
point I'm starting to wonder if we're abusing the system.

I think it would be reasonable to special-case panels so that they can become
key when directly selected, but otherwise they can only become key if all the
non-panel windows are closed or minimized.  I could see also allowing the next
panel in order to become key if a panel is closed when key (but not if closed
without making key), but I could also see not special-casing that.

BTW, as you might have guessed from earlier reviewer comments, how ordering goes
on Windows is probably a red herring.  If there are specific non-hypothetical
use cases, it might be useful to have an example app for people to look at to
see if it feels weird.  Meanwhile, I think there's some utility to having a
clear ordering to who gets focus next, rather than potentially breaking the
ordering now to fix hypothetical problems (unless you think we need to flush out
potential API changes early?).