Start Alt+Tab on MRU instead of MRU+1 if all windows are minimized.

Start Alt+Tab on MRU instead of MRU+1 if all windows are minimized.

BUG=647124
Committed: https://crrev.com/3cd56dc1584748a918f89ba8b8b42afaa94dcb6f
Cr-Commit-Position: refs/heads/master@{#423552}

[Evan Stade, 2016-10-04 22:41:35]

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

[commit-bot: I haz the power, 2016-10-04 22:42:06]

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

[Evan Stade, 2016-10-04 22:43:24]

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

[commit-bot: I haz the power, 2016-10-04 23:08:28]

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

[commit-bot: I haz the power, 2016-10-04 23:08:29]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-05 23:51:41]

On 2016/10/04 22:43:24, Evan Stade wrote:

ping

[varkha, 2016-10-06 03:31:08]

lgtm with nits / suggestions.

https://codereview.chromium.org/2389323002/diff/1/ash/common/wm/window_cycle_...
File ash/common/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/2389323002/diff/1/ash/common/wm/window_cycle_...
ash/common/wm/window_cycle_list.cc:527: current_index_ = -1;
Optional nit:
There logic is correct but still takes some effort to follow. For your
consideration - maybe just set a bool flag here instead of setting current_index
to an out of range value (-1) even though that is temporary. Then use that flag
below to skip the increment. I think this would be easier to read.

https://codereview.chromium.org/2389323002/diff/1/ash/wm/window_cycle_control...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://codereview.chromium.org/2389323002/diff/1/ash/wm/window_cycle_control...
ash/wm/window_cycle_controller_unittest.cc:298: // If all windows are minimized,
start on the first one rather than the second
Nit: "Tests that when all windows are minimized, cycling starts with the first
one rather than the second".

https://codereview.chromium.org/2389323002/diff/1/ash/wm/window_cycle_control...
ash/wm/window_cycle_controller_unittest.cc:319: window1_state->Minimize();
Redundant?

[Evan Stade, 2016-10-06 15:40:33]

https://chromiumcodereview.appspot.com/2389323002/diff/1/ash/common/wm/window...
File ash/common/wm/window_cycle_list.cc (right):

https://chromiumcodereview.appspot.com/2389323002/diff/1/ash/common/wm/window...
ash/common/wm/window_cycle_list.cc:527: current_index_ = -1;
On 2016/10/06 03:31:08, varkha wrote:
> Optional nit:
> There logic is correct but still takes some effort to follow. For your
> consideration - maybe just set a bool flag here instead of setting
current_index
> to an out of range value (-1) even though that is temporary. Then use that
flag
> below to skip the increment. I think this would be easier to read.

I did write this in several different formats because I agree it's not the most
intuitive code ever. The brevity of this one won out. (I think tracing the value
of a bool that's conditionally set inside a conditional block has its own set of
readability issues.)

https://chromiumcodereview.appspot.com/2389323002/diff/1/ash/wm/window_cycle_...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://chromiumcodereview.appspot.com/2389323002/diff/1/ash/wm/window_cycle_...
ash/wm/window_cycle_controller_unittest.cc:298: // If all windows are minimized,
start on the first one rather than the second
On 2016/10/06 03:31:08, varkha wrote:
> Nit: "Tests that when all windows are minimized, cycling starts with the first
> one rather than the second".

Done.

https://chromiumcodereview.appspot.com/2389323002/diff/1/ash/wm/window_cycle_...
ash/wm/window_cycle_controller_unittest.cc:319: window1_state->Minimize();
On 2016/10/06 03:31:08, varkha wrote:
> Redundant?

seems harmless but improves legibility

[Evan Stade, 2016-10-06 15:40:40]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-06 15:40:41]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2389323002/#ps20001
(title: "adopt Valery's comment")

[commit-bot: I haz the power, 2016-10-06 15:41:00]

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-10-06 16:09:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-06 16:14:25]

Description was changed from

==========
Start Alt+Tab on MRU instead of MRU+1 if all windows are minimized.

BUG=647124
==========

to

==========
Start Alt+Tab on MRU instead of MRU+1 if all windows are minimized.

BUG=647124

Committed: https://crrev.com/3cd56dc1584748a918f89ba8b8b42afaa94dcb6f
Cr-Commit-Position: refs/heads/master@{#423552}
==========

[commit-bot: I haz the power, 2016-10-06 16:14:26]

Patchset 2 (id:??) landed as
https://crrev.com/3cd56dc1584748a918f89ba8b8b42afaa94dcb6f
Cr-Commit-Position: refs/heads/master@{#423552}