Enable ash window cycle UI by default.

Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111
Committed: https://crrev.com/f7648dcc7f7474f89028af0e4643678a6a97a846
Cr-Commit-Position: refs/heads/master@{#411909}

[Evan Stade, 2016-08-11 01:56:45]

Description was changed from

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=636594
==========

to

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111
==========

[Evan Stade, 2016-08-11 01:56:45]

estade@chromium.org changed reviewers:
+ jamescook@chromium.org, varkha@chromium.org

[Evan Stade, 2016-08-11 01:57:11]

varkha:
  ash/common/wm/
jamescook:
  ash/common/ash_switches.*

[Evan Stade, 2016-08-11 01:57:14]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 01:58:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 02:33:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 02:33:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2016-08-11 16:50:55]

ash/common/ash_switches.* lgtm but it looks like you've got some test failures
to sort out

[varkha, 2016-08-11 17:26:28]

The change LGTM, but I would consider replacing this with a check for
--ash-md=(default | experimental). It is a one liner for a flag that is already
encapsulated in MDC (maybe with a one line helper method that we can drop later)
and you get the benefit of being able to turn it off for troubleshooting or if
something fatal is introduced like a graphics resource leak.
--ash-md=default is currently on by default in M-53.

[Evan Stade, 2016-08-11 18:19:01]

On 2016/08/11 17:26:28, varkha wrote:
> The change LGTM, but I would consider replacing this with a check for
> --ash-md=(default | experimental). It is a one liner for a flag that is
already
> encapsulated in MDC (maybe with a one line helper method that we can drop
later)
> and you get the benefit of being able to turn it off for troubleshooting or if
> something fatal is introduced like a graphics resource leak.
> --ash-md=default is currently on by default in M-53.

Wouldn't that mean that to disable it we'd have to disable all of ash md? I
still think this feature is orthogonal to the rest of MD, and if we have to back
it out it's still a one line change/merge.

I do need to look into the test failures.

[varkha, 2016-08-11 19:54:11]

On 2016/08/11 18:19:01, Evan Stade wrote:
> On 2016/08/11 17:26:28, varkha wrote:
> > The change LGTM, but I would consider replacing this with a check for
> > --ash-md=(default | experimental). It is a one liner for a flag that is
> already
> > encapsulated in MDC (maybe with a one line helper method that we can drop
> later)
> > and you get the benefit of being able to turn it off for troubleshooting or
if
> > something fatal is introduced like a graphics resource leak.
> > --ash-md=default is currently on by default in M-53.
> 
> Wouldn't that mean that to disable it we'd have to disable all of ash md? I
> still think this feature is orthogonal to the rest of MD, and if we have to
back
> it out it's still a one line change/merge.
> 
> I do need to look into the test failures.

>> we'd have to disable all of ash md
Yes (btw, currently this only means disabling overview mode), but I agree that
you
would not need to worry about end users needing to disable it long term. This
just
gives you some immediate way to A/B test or troubleshoot without rebuild.

[Evan Stade, 2016-08-12 21:03:31]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 21:03:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 21:36:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 21:36:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-08-12 22:51:57]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-08-12 22:51:58]

Most test failures just required updating expectations but there were some real
bugs in there too, which I've now addressed (hopefully comprehensively). Since
these are fairly edge-casey and the fixes aren't necessarily self-evident, I've
notated my changes inline and tried to leave clear comments in the code.

+sky could you have a look?

https://codereview.chromium.org/2234233002/diff/40001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (left):

https://codereview.chromium.org/2234233002/diff/40001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1144:
generator.PressKey(ui::VKEY_TAB, ui::EF_ALT_DOWN);
before, alt+tab was enough for activation, now you must release alt. This is
also a handy way to test that alt+tab/release works even before the UI's show
delay is finished.

https://codereview.chromium.org/2234233002/diff/40001/ash/common/wm/window_cy...
File ash/common/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/2234233002/diff/40001/ash/common/wm/window_cy...
ash/common/wm/window_cycle_controller.cc:61: // there will be a flicker as the
target window changes.
this is really annoying and gross. See ash/common/wm/switchable_windows.{cc,h}.
In particular, the shelf's hide/show state depends on me not changing the
definition of "switchable" windows, even though the current definition is very
deceptive. There may be other places not covered by unit tests that also rely on
the app list being included in this window list as a lot of places seem to use
the MRU list.

https://codereview.chromium.org/2234233002/diff/40001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller_unittest.cc (left):

https://codereview.chromium.org/2234233002/diff/40001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:176:
EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
here and in many places in this file: Alt+Tab no longer activates windows one by
one, it only activates the one that's selected when you release alt.

https://codereview.chromium.org/2234233002/diff/40001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:307:
EXPECT_TRUE(wm::IsActiveWindow(window2.get()));
this doesn't really seem worth updating. Instead I deleted it because it's
redundant with other tests and not crucial to the (seeming) intent of this test.

https://codereview.chromium.org/2234233002/diff/40001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:349: controller->StopCycling();
ditto

https://codereview.chromium.org/2234233002/diff/40001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:419: controller->StopCycling();
ditto

https://codereview.chromium.org/2234233002/diff/40001/ui/app_list/presenter/a...
File ui/app_list/presenter/app_list_presenter_impl.cc (right):

https://codereview.chromium.org/2234233002/diff/40001/ui/app_list/presenter/a...
ui/app_list/presenter/app_list_presenter_impl.cc:89: if
(view_->GetWidget()->IsActive())
This is necessary because otherwise Alt+Tabbing while the app list is up causes
the app list to deactivate and pass activation to some other random window,
stealing activation from the window cycle ui.

I've added test coverage for this in
WindowCycleControllerTest.SelectingHidesAppList

[Evan Stade, 2016-08-12 22:52:45]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 22:53:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 00:07:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 00:07:17]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-13 15:56:06]

LGTM

https://codereview.chromium.org/2234233002/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2234233002/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1127: auto press_alt_tab =
[&generator]() {
optional: press_alt_tab is easy to assume means only press. Maybe
press_and_release_alt_tab?

https://codereview.chromium.org/2234233002/diff/60001/ash/common/wm/window_cy...
File ash/common/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/2234233002/diff/60001/ash/common/wm/window_cy...
ash/common/wm/window_cycle_controller.cc:65: bool foo = window->GetRootWindow()
foo huh? Maybe is_app_list_window, or no temporary and just return (I suspect
the name is left over from earlier debugging).

[Evan Stade, 2016-08-14 01:43:13]

https://codereview.chromium.org/2234233002/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/2234233002/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:1127: auto press_alt_tab =
[&generator]() {
On 2016/08/13 15:56:06, sky wrote:
> optional: press_alt_tab is easy to assume means only press. Maybe
> press_and_release_alt_tab?

Done.

https://codereview.chromium.org/2234233002/diff/60001/ash/common/wm/window_cy...
File ash/common/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/2234233002/diff/60001/ash/common/wm/window_cy...
ash/common/wm/window_cycle_controller.cc:65: bool foo = window->GetRootWindow()
On 2016/08/13 15:56:06, sky wrote:
> foo huh? Maybe is_app_list_window, or no temporary and just return (I suspect
> the name is left over from earlier debugging).

haha yes, that is accurate. should just return.

[Evan Stade, 2016-08-14 01:43:21]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-08-14 01:43:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, varkha@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2234233002/#ps80001
(title: "sky review")

[commit-bot: I haz the power, 2016-08-14 01:43:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-14 02:30:48]

Description was changed from

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111
==========

to

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111
==========

[commit-bot: I haz the power, 2016-08-14 02:30:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-14 02:32:07]

Description was changed from

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111
==========

to

==========
Enable ash window cycle UI by default.

I removed the enable flag without replacing it with a disable flag
because I don't foresee a need for individuals to disable this feature.
In the case that we decide against launching, this patch should be
reverted (which is the same thing we'd have to do if we changed it to
a disable flag).

BUG=626111

Committed: https://crrev.com/f7648dcc7f7474f89028af0e4643678a6a97a846
Cr-Commit-Position: refs/heads/master@{#411909}
==========

[commit-bot: I haz the power, 2016-08-14 02:32:09]

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

[hiroshige, 2016-08-15 08:07:12]

On 2016/08/14 02:32:09, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/f7648dcc7f7474f89028af0e4643678a6a97a846
> Cr-Commit-Position: refs/heads/master@{#411909}

Reverting this CL: https://codereview.chromium.org/2249583002/
Reason for revert:
Suspected to cause crbug.com/637671:
failures in ash_unittests on Linux Chromium OS ASan LSan Tests (1)