Mac: Fix window raising for multiple desktops

Mac: Fix window raising for multiple desktops

When raising a set of windows in response to a dock icon click, only raise the
ones on the currently active space. We currently raise all of them and switch
spaces haphazardly.

Also tidy up the applicationShouldHandleReopen:hasVisibileWindows: callback. We
should return NO since we've reacted and don't need AppKit to deminiaturize for
us. (Although it seems to not make much difference, probably because we've
already picked a window to deminiaturize, so AppKit won't pick a second.) Also
remove the tabbed/popup check. As of r192264, those are the only browser window
types.

BUG=281674
TEST=Open a browser window in Desktop 1 and one in Desktop 2.
Switch to Desktop 2.
Open another application in Desktop 2 and make it foreground.
Click the dock icon for Chrome. Should remain on Desktop 2.

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

[davidben, 2013-08-29 20:08:09]

Alright, this is kind of hairy. So, I spent some time trying to figure out the
native Mac behavior... I believe it is something like this:

(So, there's this setting, defaulted on, "When switching to an application,
switch to a space with open windows for that application" under Mission Control.
I'm going to call this "the setting".)

1. If the application was previously active and there are other spaces with
visible windows, jump to a space with a visible window. Only raise that window,
*not* others in the space.

2. Else, if some windows in the current space are visible, raise *all* windows
in this space.

3. Else, if the setting is on, pick a space with visible windows and switch to
it. Raise *all* windows in this space.

4. Else, do nothing.

After/while/before all that happens (not sure how to distinguish, and I assume
the previous steps are done by the window manager while these are done by the
application), AppKit, if you return YES from
applicationShouldHandleReopen:hasVisibleWindows:, follows:

1. If *all* windows are minimized on *all* desktops, pick a window... somehow...
to restore. This switches to that workspace. Note that this is independent of
the value of the setting, which gives pretty bizarre behavior. If the setting is
*off* and *all* windows on *all* spaces are minimized, the setting is ignored
and you switch space anyway. If a single window on *any* space (not necessarily
this one) is visible, the setting is honored.

This CL doesn't actually successfully emulate this behavior, but it seems to
behave reasonably in my testing. I'm not sure we can. The main awkwardness is
that if the first set of steps results in a desktop switch, [window
isOnActiveSpace] resolves relative to the previous space. Which is pretty
awkward, since sometimes we switch spaces twice. There's a long comment that
describes the failure case.

I put this in FocusWindowSet instead of the AppController callback because it
looks at an unfiltered set of windows to determine whether to unminimize a
window. If I don't do that, it tends to trigger the failure case much more
often. And the real thing does this. Also, I imagine the other caller,
ExtensionAppShimHandler::OnShimFocus, cares too. But I couldn't figure out how
to test it. Shim apps seem to immediately disappear from the dock. (Are they
supposed to?)

[jackhou1, 2013-08-30 03:52:46]

> I put this in FocusWindowSet instead of the AppController callback because it
> looks at an unfiltered set of windows to determine whether to unminimize a
> window. If I don't do that, it tends to trigger the failure case much more
> often. And the real thing does this. Also, I imagine the other caller,
> ExtensionAppShimHandler::OnShimFocus, cares too. But I couldn't figure out how
> to test it. Shim apps seem to immediately disappear from the dock. (Are they
> supposed to?)

This looks CL fine for ExtensionAppShimHandler. App shims should work if you
have them switched on in chrome:flags (Enable packaged app shortcuts.
#enable-app-shims). Otherwise, if they appear but disappear immediately, you
might be running into this bug http://crbug.com/271347.

[davidben, 2013-08-30 22:55:28]

On 2013/08/30 03:52:46, jackhou1 wrote:
> > I put this in FocusWindowSet instead of the AppController callback because
it
> > looks at an unfiltered set of windows to determine whether to unminimize a
> > window. If I don't do that, it tends to trigger the failure case much more
> > often. And the real thing does this. Also, I imagine the other caller,
> > ExtensionAppShimHandler::OnShimFocus, cares too. But I couldn't figure out
how
> > to test it. Shim apps seem to immediately disappear from the dock. (Are they
> > supposed to?)
> 
> This looks CL fine for ExtensionAppShimHandler. App shims should work if you
> have them switched on in chrome:flags (Enable packaged app shortcuts.
> #enable-app-shims). Otherwise, if they appear but disappear immediately, you
> might be running into this bug http://crbug.com/271347.

Yeah, this is with that enabled. The bug says it's only been repro'd on Lion so
far? I'm on Mountain Lion.

[jackhou1, 2013-09-02 06:07:23]

Unfortunately, we don't yet know exactly why that bug happens. I patched this CL
to try out. It seems to work for Chrome windows but not apps, probably because
OSX does something extra. I suggested a fix inline.

https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
ui/base/cocoa/focus_window_set.mm:40: all_miniaturized = false;
Could this be:

      } else if ([win isVisible]) {
        all_miniaturized = false;
        frontmost_window = win;
        if ([win isOnActiveSpace])
          [win orderFront:nil];
      }

[davidben, 2013-09-03 20:01:53]

Does this version work better for app shims? (I can't test them myself because
of the crash.)

https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
ui/base/cocoa/focus_window_set.mm:40: all_miniaturized = false;
On 2013/09/02 06:07:23, jackhou1 wrote:
> Could this be:
> 
>       } else if ([win isVisible]) {
>         all_miniaturized = false;
>         frontmost_window = win;
>         if ([win isOnActiveSpace])
>           [win orderFront:nil];
>       }

That seems to cause it to get a bit confused. If frontmost_window wasn't one
that was ordered to front, you end up with key/main windows that aren't on the
current space. It seems to make things go crazy when you press keys.

I can't test the app shim thing, but is the problem that, when the app is on a
different workspace, it doesn't know to switch? What if FocusWindowSet took a
boolean allow_workspace_switch parameter. We track frontmost_window and
frontmost_window_all_spaces. If the former is nil and workspace switch is
allowed, we bring that to front and force a workspace switch. App shims would
pass true because activating the dock icon doesn't allow OS X to use its native
workspace switching behavior. The browser should probably pass false so we still
honor the "[...] switch to a space with open windows for the application"
setting as best we can.

[jackhou1, 2013-09-04 01:19:40]

On 2013/09/03 20:01:53, David Benjamin wrote:
> Does this version work better for app shims? (I can't test them myself because
> of the crash.)
> 
>
https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
> File ui/base/cocoa/focus_window_set.mm (right):
> 
>
https://codereview.chromium.org/23737003/diff/6001/ui/base/cocoa/focus_window...
> ui/base/cocoa/focus_window_set.mm:40: all_miniaturized = false;
> On 2013/09/02 06:07:23, jackhou1 wrote:
> > Could this be:
> > 
> >       } else if ([win isVisible]) {
> >         all_miniaturized = false;
> >         frontmost_window = win;
> >         if ([win isOnActiveSpace])
> >           [win orderFront:nil];
> >       }
> 
> That seems to cause it to get a bit confused. If frontmost_window wasn't one
> that was ordered to front, you end up with key/main windows that aren't on the
> current space. It seems to make things go crazy when you press keys.
> 
> I can't test the app shim thing, but is the problem that, when the app is on a
> different workspace, it doesn't know to switch? What if FocusWindowSet took a
> boolean allow_workspace_switch parameter. We track frontmost_window and
> frontmost_window_all_spaces. If the former is nil and workspace switch is
> allowed, we bring that to front and force a workspace switch. App shims would
> pass true because activating the dock icon doesn't allow OS X to use its
native
> workspace switching behavior. The browser should probably pass false so we
still
> honor the "[...] switch to a space with open windows for the application"
> setting as best we can.

Yeah this seems to work pretty well. LGTM

[davidben, 2013-09-04 14:21:55]

Oh, apparently I need more OWNERS now. tapted, thakis, could you guys take a
look at:

tapted: apps/app_shim/extension_app_shim_handler_mac.cc
thakis: the rest of the files. :-)

[Nico, 2013-09-04 15:50:09]

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
ui/base/cocoa/focus_window_set.mm:28: // we deminiaturize the browser, and that
triggers switching back.
Would using the private api CGSGetWindowWorkspace() allow fixing this problem?

[Nico, 2013-09-04 15:50:57]

But if you want to address this in a follow-up, this lgtm too since it seems to
improve things over what's in the tree today.

Maybe you want to take a stab at
https://code.google.com/p/chromium/issues/detail?id=74812 too while you're
looking at spaces :-)

[davidben, 2013-09-04 16:05:58]

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
ui/base/cocoa/focus_window_set.mm:28: // we deminiaturize the browser, and that
triggers switching back.
On 2013/09/04 15:50:09, Nico wrote:
> Would using the private api CGSGetWindowWorkspace() allow fixing this problem?

Probably not? Is there a way to figure out where OS X decided to switch desktops
to, so we have something to compare against? The problem is that we don't know
at the time FocusWindowSet is currently called whether OS X triggered its
desktop switching logic. (And that logic is dependent on a user setting, so
trying to fake it ourselves seems a bad plan.) I'm not familiar enough with OS X
to know if there's another hook we should be using instead.

Fortunately, raising the wrong desktop's windows *while another workspace switch
is already happening* seems to not trigger an additional workspace switch. I'm
not too happy to be relying on that, but that's what I mean by "This seems to
still work okay."

What does trigger a second switch is deminiaturizing during a workspace switch.
But at that point, we've already lost. We want that second switch. The real
trouble is that OS X's desktop switching logic doesn't actually know that the
app windows are logically a different app from the browser. It sees that we're
on both spaces 1 and 2 and decides to go into "cycle between workspaces with
this app" mode and switches to space 1. But space 1 only contains an app window,
not a browser window, so we should never have switched to it in the first place.

I suspect we can't actually behave completely properly unless the app shims
actually host the app's NSWindow. Otherwise we're always cleaning up after the
default window server behavior, and sometimes we confuse it. But I imagine
separating the browser process from the NSWindow host would require quite a bit
of work.

[davidben, 2013-09-04 16:19:16]

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
File ui/base/cocoa/focus_window_set.mm (right):

https://codereview.chromium.org/23737003/diff/21001/ui/base/cocoa/focus_windo...
ui/base/cocoa/focus_window_set.mm:28: // we deminiaturize the browser, and that
triggers switching back.
On 2013/09/04 16:05:58, David Benjamin wrote:
> Fortunately, raising the wrong desktop's windows *while another workspace
switch
> is already happening* seems to not trigger an additional workspace switch. I'm
> not too happy to be relying on that, but that's what I mean by "This seems to
> still work okay."

Thinking about that some more, another explanation is that it's not that a
workspace switch is in progress but that, at the time, it is actually still the
current workspace (by whatever relevant metric) and doesn't trigger a second
switch. I'm slightly happier relying on that one. But that doesn't explain why
deminitiarizing a window on the (previously) current workspace does trigger a
switch back. Maybe terrible races with the fact that some things are animated
and some things aren't...

*shrug* Secretly, I have no idea how anything on Mac works. I just spent a bunch
of time experimenting with TextEdit's and our behavior here. :-)

[tapted, 2013-09-05 00:07:48]

lgtm

[commit-bot: I haz the power, 2013-09-05 13:56:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/23737003/21001

[commit-bot: I haz the power, 2013-09-05 19:38:51]

Change committed as 221495