Mac: Give packaged app windows their own window cycle list.

Mac: Give packaged app windows their own window cycle list.

For now it's behind chrome://flags#app-window-cycling.

This is to hopefully give a better UX for App Shims. It also solves a
tricky problem with Chrome Remote Desktop: Since CRD swallows Cmd+`, it
breaks browser window cycling since the cycling will stop as soon as a
CRD window is encountered.

This works by hooking in to the same mechanism that changes the menu
bar. Switching to or away from a packaged app window will now also update
the window cycle list. The CL intentionally does *not*:
 - try to distinguish between the same app running in different profiles,
 - update the `Window` menu, nor
 - try to do anything for Cmd+Tab switching.

The hard part: AppKit does not immediately apply changes to
NSWindowCollectionBehaviorIgnoresCycle after a window is already shown.
An effective workaround seems to be call to [[NSApp keyWindow]
makeKeyAndOrderFront:nil]. However, this may have other consequences.
Lots of other things were tried (see code comments).

Adds an interactive UI test. Tests on mac using
ui_test_utils::ShowAndFocusNativeWindow(..) have a history of flakiness,
so this CL attempts to address that by combining with a notification
observer. This approach has successfully addressed flakiness around
key-window transitions elsewhere. It would commonly occur when multiple
windows are in the process -- a single RunLoop().RunUntilIdle() couldn't
guarantee that the window server had completed the switch.

BUG=484737, 278408
Committed: https://crrev.com/91f299c14448905eaeaf635c86d3d527241d02e7
Cr-Commit-Position: refs/heads/master@{#336063}

[tapted, 2015-06-17 05:47:41]

Patchset #4 (id:60001) has been deleted

[tapted, 2015-06-17 05:47:47]

Patchset #4 (id:80001) has been deleted

[tapted, 2015-06-19 10:18:49]

Patchset #9 (id:200001) has been deleted

[tapted, 2015-06-21 23:44:51]

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

[tapted, 2015-06-22 00:00:20]

Hi Jack - please take a look

[jackhou1, 2015-06-22 01:34:26]

lgtm

https://codereview.chromium.org/1186803003/diff/220001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.h (right):

https://codereview.chromium.org/1186803003/diff/220001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:36: // Waits for |minimumCount|
notifications to be observed and returns YES.
This quits the run loop as soon as a notification is received. So it only waits
until the next notification arrives, and may return NO if |minimumCount| has not
been reached by then. You can probably just change the comment here (and maybe
the method name), since the new tests don't actually need to wait for multiple
notifications.

[tapted, 2015-06-22 01:57:08]

PTAL

https://codereview.chromium.org/1186803003/diff/220001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.h (right):

https://codereview.chromium.org/1186803003/diff/220001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:36: // Waits for |minimumCount|
notifications to be observed and returns YES.
On 2015/06/22 01:34:26, jackhou1 wrote:
> This quits the run loop as soon as a notification is received. So it only
waits
> until the next notification arrives, and may return NO if |minimumCount| has
not
> been reached by then. You can probably just change the comment here (and maybe
> the method name), since the new tests don't actually need to wait for multiple
> notifications.

ooh! good catch. That wasn't intentional - I think the disabled tests on my
radar will be more robust if they can rely on the waiter possibly waiting
multiple times. Updated to do this.

[jackhou1, 2015-06-22 02:48:39]

lgtm

[tapted, 2015-06-22 02:53:31]

tapted@chromium.org changed reviewers:
+ asvitkine@chromium.org, thakis@chromium.org

[tapted, 2015-06-22 02:59:47]

+nico for chrome/test/base/interactive_test_utils_mac.mm
+asvitkine for histograms

(As well as for any sage Mac wisdom about the approach in general, since I'm a
bit spooked by it and don't want a regression in tricky bugs like the
AppKit-can-have-2-Key-Windows-simultaneously one in http://crbug.com/459306.
Plan is to try this out in lots of "real" workflows before making it default.)

Thanks!

[Nico, 2015-06-22 05:55:17]

I probably won't be around for the next 24h and maybe longer (dealing with
s minor emergency).
On Jun 21, 2015 7:59 PM, <tapted@chromium.org> wrote:

> +nico for chrome/test/base/interactive_test_utils_mac.mm
> +asvitkine for histograms
>
> (As well as for any sage Mac wisdom about the approach in general, since
> I'm a
> bit spooked by it and don't want a regression in tricky bugs like the
> AppKit-can-have-2-Key-Windows-simultaneously one in
> http://crbug.com/459306.
> Plan is to try this out in lots of "real" workflows before making it
> default.)
>
> Thanks!
>
> https://codereview.chromium.org/1186803003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2015-06-22 15:19:55]

asvitkine@chromium.org changed reviewers:
+ rsesek@chromium.org

[Alexei Svitkine (slow), 2015-06-22 15:19:55]

histograms lgtm

+rsesek for the tricky Mac bits

https://codereview.chromium.org/1186803003/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/1186803003/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:226: any_change =
SetWindowParticipatesInCycle(window, false) || any_change;
Nit: How about this:

if (is_browser || extension)
  any_change = SetWindowParticipatesInCycle(window, is_browser) || any_change;

[tapted, 2015-06-23 00:22:07]

Thanks!

I'll see what Robert thinks before looking for a chrome/test owner

https://codereview.chromium.org/1186803003/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/1186803003/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:226: any_change =
SetWindowParticipatesInCycle(window, false) || any_change;
On 2015/06/22 15:19:55, Alexei (OOO June 24 - July 6) wrote:
> Nit: How about this:
> 
> if (is_browser || extension)
>   any_change = SetWindowParticipatesInCycle(window, is_browser) || any_change;

Done.

[Robert Sesek, 2015-06-23 15:00:55]

LGTM

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:189: any_change =
SetWindowParticipatesInCycle(window, true) || any_change;
You could use |= instead of "|| any change" at the end of the expression.

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:225:
SetWindowParticipatesInCycle(window, is_browser) || any_change;
Same.

[tapted, 2015-06-23 23:33:33]

Patchset #13 (id:300001) has been deleted

[tapted, 2015-06-23 23:33:44]

Patchset #13 (id:320001) has been deleted

[tapted, 2015-06-23 23:36:10]

Nico: if you're back (and since you're here already..), are you able to provide
OWNERS for chrome/test/base/interactive_test_utils_mac.mm ?

Thanks!

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm (right):

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:189: any_change =
SetWindowParticipatesInCycle(window, true) || any_change;
On 2015/06/23 15:00:55, Robert Sesek wrote:
> You could use |= instead of "|| any change" at the end of the expression.

Done.

https://codereview.chromium.org/1186803003/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac.mm:225:
SetWindowParticipatesInCycle(window, is_browser) || any_change;
On 2015/06/23 15:00:55, Robert Sesek wrote:
> Same.

Done.

[Nico, 2015-06-24 19:53:20]

interactive_test_utils_mac.mm lgtm

[tapted, 2015-06-25 00:12:32]

The CQ bit was checked by tapted@chromium.org

[tapted, 2015-06-25 00:12:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jackhou@chromium.org, asvitkine@chromium.org, rsesek@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1186803003/#ps380001
(title: "rebase (overlap with r335812 and I want a fresh CQ run)")

[commit-bot: I haz the power, 2015-06-25 00:13:57]

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

[commit-bot: I haz the power, 2015-06-25 01:28:25]

Committed patchset #15 (id:380001)

[commit-bot: I haz the power, 2015-06-25 01:29:27]

Patchset 15 (id:??) landed as
https://crrev.com/91f299c14448905eaeaf635c86d3d527241d02e7
Cr-Commit-Position: refs/heads/master@{#336063}