Separated alt-tab window cycle from overview mode

Separated alt-tab window cycle from overview mode

This change separates the logic into two different classes. It also
- Brings back a bounce animation when there is only one window that was
lost at some point in the past.
- Removes the minimized animation if there is only one window while alt tabbing.

Basically I brought back from the past two classes,
WindowCycleController and WindowCycleList and adapted them
to work with the new codebase. Also, I moved some tests from
WindowSelectorUnittest to WindowCycleControllerUnittest, and 
removed the ones that are now irrelevant. 

Finally, we are also dropping support for alt-tabbing while
in overview mode in favour of cancelling the latter.
BUG=365786
TEST=none

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

[Nina, 2014-05-01 17:00:01]

I know this is big, sorry :(
The new tests are almost a copy paste of the old tests + some aditional tests
that consider panel windows and were in the WindowSelectorUnittests before.

Have fun!
https://memegen.googleplex.com/6722096667295744

[tdanderson, 2014-05-01 18:51:38]

Hi Nico, please see my comments below:

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
ash/wm/overview/window_overview.cc:264: event->StopPropagation();
What is this change for?

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:86: aura::Window*
GetWindowBelow(aura::Window* window) {
Is this still needed here now that you've moved it to WindowCycleList?

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_unittest.cc:403: }*/
Fix this.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:31: virtual void OnKeyEvent(ui::KeyEvent*
event) OVERRIDE;
newline after 31

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:80: Step(direction);
Can you factor out the call to Step(direction) here and at line 84 and just call
it at the end of this function?

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:83: // We're in the middle of an alt-tab
cycle, just step forward.
"forward" -> |direction| is not necessarily FORWARD, so update the comment here.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:125: void
WindowCycleController::InstallEventFilter() {
I don't think this needs to be its own helper. I'd remove it and instead move
lines 126-127 to the bottom of StartCycling(). This would be consistent with the
fact you're removing the handler at the end of StopCycling().

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
Make sure you have the right headers for new files (correct format is at
http://www.chromium.org/developers/coding-style).

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:20: // Controls cycling through windows with
the keyboard, for example, via alt-tab.
"for example" -> this implies that there are other ways to cycle through windows
with the keyboard. Doesn't this implementation make the assumption that only
Alt+Tab is being used?

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:29: enum Direction {
It feels unnecessary to define identical enums in both WindowCycleController and
WindowCycleList.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:40: // Cycles between windows in the given
|direction|. If |is_alt_down| then
You mention |is_alt_down| in this comment, but I don't see the variable
anywhere.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:45: // Cycles between windows without
maintaining a multi-step cycle sequence
I don't understand what a "multi-step cycle sequence" is based on the comments
in this header file.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:46: // (see above).
Avoid saying above/below in comments, since the order of functions may change,
etc.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:47: void HandleLinearCycleWindow();
I don't see where HandleLinearCycleWindow() is called.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:60: // Call to start cycling windows.  You must
call StopCycling() when done.
"You must call" implies that you're expecting another class to call
StopCycling(). But that is a private helper in this class, so you're the only
one who should be calling it. I would remove the second sentence of this
comment.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:69: // Stops the current window cycle and
cleans up the event filter.
"cleans up" -> "removes"

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:72: scoped_ptr<WindowCycleList> windows_;
I'd prefer this to be called window_cycle_list_ or something other than just
windows_.

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller_unittest.cc:67: TEST_F(WindowCycleControllerTest,
HandleCycleWindowBaseCases) {
Include TEST=... in the issue description with any tests that are brand new, not
just ones that have been changed slightly and moved to a new file. If there are
no new tests, say TEST=none. This makes it easier for your reviewers.

What are the new things I should be looking at in this file? Also, do you have a
test that explicitly checks that OM is exited on Alt+Tab?

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:144: // We weren't able to find our active window in
the shell delegate's
This worries me a little bit. I feel that a precondition of Step() should be
that current_index_ != 1. AFAICT, the only reason you have this here is for the
case when wm::GetActiveWindow() doesn't return a window in the MRU list. Are
there any other times when current_index_ == -1 here?

I suggest adding a DCHECK to verify that current_index_ is a valid index for the
window list, and move this logic into the constructor: If wm::GetActiveWindow()
returns a window not in the MRU list, then just set current_index_ = 0. Would
that work? Or better yet, simply initialize current_index_ to 0 in the
constructor initialization list.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:146: current_index_ = (direction == FORWARD ? 0 :
windows_.size() - 1);
No () needed

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:148: // When there is only one window, we should
give a feedback to user.
Double-check with the logs (or Rob if you can't find anything) to see if the
bounce animation was removed for a reason.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:151: ::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
Spacing

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:155: current_index_ += (direction == FORWARD ? 1 :
-1);
No ()

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:180: if (current_index_ > removed_index)
combine lines 180-183 into a single check

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:45: virtual void OnWindowDestroyed(aura::Window*
window) OVERRIDE;
IIUC, a WindowCycleList only exists while Alt+Tab cycling is in progress. How
can windows get destroyed during cycling? (i.e., why is it necessary to have
WindowCycleList be a WindowObserver?)

If it is possible for windows to be destroyed during a cycle session, you should
definitely explain that (perhaps in your comment block at line 47)

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:53: // Current position in the |windows_| list or -1
if we're not cycling.
Similar comment to the one above: if this object exists, doesn't that mean we're
currently cycling? I don't think this should ever be allowed to have a -1 value.
See my comment in window_cycle_list.cc, line 144.

[Nina, 2014-05-01 20:41:16]

Please keep in mind that I didn't code WindowCycleList nor
WindowCycleController... just adjusted them to work now, so the issues you found
(especially those related to style) were already in the codebase before. For
that reason, I thought they were fine.

Function you can use to test closing windows automatically (paste this into the
JS console):
function close () {myWindow.close();}
myWindow = window.open("","myWindow","width=200,height=100");    // Opens a new
window
myWindow.document.write("<p>This is 'myWindow'</p>");
window.setTimeout(close,3000);

This isn't an edge case, as many WebApps use this functionality.

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
ash/wm/overview/window_overview.cc:264: event->StopPropagation();
On 2014/05/01 18:51:39, tdanderson wrote:
> What is this change for?

The reason was that with SetHandled the mouse event gets through and activates
the window on /mouse_down/, instead of /mouse_release/ as intended by the code.
Let's discuss that on a later patch.

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector.cc:86: aura::Window*
GetWindowBelow(aura::Window* window) {
On 2014/05/01 18:51:39, tdanderson wrote:
> Is this still needed here now that you've moved it to WindowCycleList?

Nope, removed.

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_selec...
ash/wm/overview/window_selector_unittest.cc:403: }*/
On 2014/05/01 18:51:39, tdanderson wrote:
> Fix this.

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:31: virtual void OnKeyEvent(ui::KeyEvent*
event) OVERRIDE;
On 2014/05/01 18:51:39, tdanderson wrote:
> newline after 31

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:80: Step(direction);
On 2014/05/01 18:51:39, tdanderson wrote:
> Can you factor out the call to Step(direction) here and at line 84 and just
call
> it at the end of this function?

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:83: // We're in the middle of an alt-tab
cycle, just step forward.
On 2014/05/01 18:51:39, tdanderson wrote:
> "forward" -> |direction| is not necessarily FORWARD, so update the comment
here.

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.cc:125: void
WindowCycleController::InstallEventFilter() {
On 2014/05/01 18:51:39, tdanderson wrote:
> I don't think this needs to be its own helper. I'd remove it and instead move
> lines 126-127 to the bottom of StartCycling(). This would be consistent with
the
> fact you're removing the handler at the end of StopCycling().

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2014/05/01 18:51:39, tdanderson wrote:
> Make sure you have the right headers for new files (correct format is at
> http://www.chromium.org/developers/coding-style).

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:20: // Controls cycling through windows with
the keyboard, for example, via alt-tab.
On 2014/05/01 18:51:39, tdanderson wrote:
> "for example" -> this implies that there are other ways to cycle through
windows
> with the keyboard. Doesn't this implementation make the assumption that only
> Alt+Tab is being used?

Ah, I forgot to update the comments. The previous implementation also allowed
changing windows with the F5 button, I think, but now there's no other way to
switch windows (at I didn't find another one) but Alt-Tab.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:29: enum Direction {
On 2014/05/01 18:51:39, tdanderson wrote:
> It feels unnecessary to define identical enums in both WindowCycleController
and
> WindowCycleList.

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:40: // Cycles between windows in the given
|direction|. If |is_alt_down| then
On 2014/05/01 18:51:39, tdanderson wrote:
> You mention |is_alt_down| in this comment, but I don't see the variable
> anywhere.

My bad, again, I removed that boolean because we no longer need it and forgot to
update the comment.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:45: // Cycles between windows without
maintaining a multi-step cycle sequence
On 2014/05/01 18:51:39, tdanderson wrote:
> I don't understand what a "multi-step cycle sequence" is based on the comments
> in this header file.

Sorry, this function should be removed, it was used when you could cycle between
windows with one button.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:46: // (see above).
On 2014/05/01 18:51:39, tdanderson wrote:
> Avoid saying above/below in comments, since the order of functions may change,
> etc. 

Gotcha.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:47: void HandleLinearCycleWindow();
On 2014/05/01 18:51:39, tdanderson wrote:
> I don't see where HandleLinearCycleWindow() is called.

It was used when you could cycle between
windows with one button. Removed.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:60: // Call to start cycling windows.  You must
call StopCycling() when done.
On 2014/05/01 18:51:39, tdanderson wrote:
> "You must call" implies that you're expecting another class to call
> StopCycling(). But that is a private helper in this class, so you're the only
> one who should be calling it. I would remove the second sentence of this
> comment.

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:69: // Stops the current window cycle and
cleans up the event filter.
On 2014/05/01 18:51:39, tdanderson wrote:
> "cleans up" -> "removes"

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller.h:72: scoped_ptr<WindowCycleList> windows_;
On 2014/05/01 18:51:39, tdanderson wrote:
> I'd prefer this to be called window_cycle_list_ or something other than just
> windows_.

Done.

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller_unittest.cc:67: TEST_F(WindowCycleControllerTest,
HandleCycleWindowBaseCases) {
On 2014/05/01 18:51:39, tdanderson wrote:
> Include TEST=... in the issue description with any tests that are brand new,
not
> just ones that have been changed slightly and moved to a new file. If there
are
> no new tests, say TEST=none. This makes it easier for your reviewers.
> 
> What are the new things I should be looking at in this file? Also, do you have
a
> test that explicitly checks that OM is exited on Alt+Tab?

This unittest class existed in the past. I had to do the following modifications
for it to work with the "new" window_cycle_controller:

* Remove tests related to one button window cycle (we now use that key for the
overview mode).

* Modify usage of HandleWindowCycle function to only take one argument.

* Bring some tests from WindowSelector:
** SelectingHidesAppList
** CyclePreservesMinimization
** CyclePanels
** CyclePanelsDestroyed
** CyclePanelsDestroyed

Apparently panels didn't exist when WindowCycleController was used.

So, I don't really know if we should include TEST=… maybe
TEST=WindowCycleControllerTest.* ?

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:144: // We weren't able to find our active window in
the shell delegate's
On 2014/05/01 18:51:39, tdanderson wrote:
> This worries me a little bit. I feel that a precondition of Step() should be
> that current_index_ != 1. AFAICT, the only reason you have this here is for
the
> case when wm::GetActiveWindow() doesn't return a window in the MRU list. Are
> there any other times when current_index_ == -1 here?
> 
> I suggest adding a DCHECK to verify that current_index_ is a valid index for
the
> window list, and move this logic into the constructor: If
wm::GetActiveWindow()
> returns a window not in the MRU list, then just set current_index_ = 0. Would
> that work? Or better yet, simply initialize current_index_ to 0 in the
> constructor initialization list.

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:146: current_index_ = (direction == FORWARD ? 0 :
windows_.size() - 1);
On 2014/05/01 18:51:39, tdanderson wrote:
> No () needed

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:148: // When there is only one window, we should
give a feedback to user.
On 2014/05/01 18:51:39, tdanderson wrote:
> Double-check with the logs (or Rob if you can't find anything) to see if the
> bounce animation was removed for a reason.

Could not find anything on the logs, I asked Rob and he said it was OK to bring
the animation back. Do you want to file a bug for it and see if someone raises
their hand?

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:151: ::wm::WINDOW_ANIMATION_TYPE_BOUNCE);
On 2014/05/01 18:51:39, tdanderson wrote:
> Spacing

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:155: current_index_ += (direction == FORWARD ? 1 :
-1);
On 2014/05/01 18:51:39, tdanderson wrote:
> No ()

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:180: if (current_index_ > removed_index)
On 2014/05/01 18:51:39, tdanderson wrote:
> combine lines 180-183 into a single check

Done.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:45: virtual void OnWindowDestroyed(aura::Window*
window) OVERRIDE;
On 2014/05/01 18:51:39, tdanderson wrote:
> IIUC, a WindowCycleList only exists while Alt+Tab cycling is in progress. How
> can windows get destroyed during cycling? (i.e., why is it necessary to have
> WindowCycleList be a WindowObserver?)
> 
> If it is possible for windows to be destroyed during a cycle session, you
should
> definitely explain that (perhaps in your comment block at line 47)

Some factors like JS close method can trigger a window destruction without
interaction from the user. If we don't take that into account, we could try to
dereference a dead pointer. Should I write that in a comment, then? I don't
think there's a design to allow the user to close windows from alt-tab mode.

[tdanderson, 2014-05-01 21:58:33]

Hi Nico, thanks for the changes! I have a few more comments below. Feel free to
pass this along to Rob for his feedback when you're ready.

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/overview/window_overv...
ash/wm/overview/window_overview.cc:264: event->StopPropagation();
On 2014/05/01 20:41:16, Nicolás wrote:
> On 2014/05/01 18:51:39, tdanderson wrote:
> > What is this change for?
> 
> The reason was that with SetHandled the mouse event gets through and activates
> the window on /mouse_down/, instead of /mouse_release/ as intended by the
code.
> Let's discuss that on a later patch.

Sounds good (please file a separate bug for that).

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_controll...
ash/wm/window_cycle_controller_unittest.cc:67: TEST_F(WindowCycleControllerTest,
HandleCycleWindowBaseCases) {
On 2014/05/01 20:41:16, Nicolás wrote:
> On 2014/05/01 18:51:39, tdanderson wrote:
> > Include TEST=... in the issue description with any tests that are brand new,
> not
> > just ones that have been changed slightly and moved to a new file. If there
> are
> > no new tests, say TEST=none. This makes it easier for your reviewers.
> > 
> > What are the new things I should be looking at in this file? Also, do you
have
> a
> > test that explicitly checks that OM is exited on Alt+Tab?
> 
> This unittest class existed in the past. I had to do the following
modifications
> for it to work with the "new" window_cycle_controller:
> 
> * Remove tests related to one button window cycle (we now use that key for the
> overview mode).
> 
> * Modify usage of HandleWindowCycle function to only take one argument.
> 
> * Bring some tests from WindowSelector:
> ** SelectingHidesAppList
> ** CyclePreservesMinimization
> ** CyclePanels
> ** CyclePanelsDestroyed
> ** CyclePanelsDestroyed
> 
> Apparently panels didn't exist when WindowCycleController was used.
> 
> So, I don't really know if we should include TEST=… maybe
> TEST=WindowCycleControllerTest.* ?

Ok, so if you're not adding new tests, then you can say TEST=none. However,
please move the above explanation of what you did into the issue description so
that anyone who looks at this gets a nice high-level summary of what you changed
without needing to dive into the diffs.

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

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.cc#...
ash/wm/window_cycle_list.cc:148: // When there is only one window, we should
give a feedback to user.
On 2014/05/01 20:41:16, Nicolás wrote:
> On 2014/05/01 18:51:39, tdanderson wrote:
> > Double-check with the logs (or Rob if you can't find anything) to see if the
> > bounce animation was removed for a reason.
> 
> Could not find anything on the logs, I asked Rob and he said it was OK to
bring
> the animation back. Do you want to file a bug for it and see if someone raises
> their hand?

I don't think that is necessary. If Rob is OK with adding it back in here then
let's just keep it in.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:45: virtual void OnWindowDestroyed(aura::Window*
window) OVERRIDE;
On 2014/05/01 20:41:16, Nicolás wrote:
> On 2014/05/01 18:51:39, tdanderson wrote:
> > IIUC, a WindowCycleList only exists while Alt+Tab cycling is in progress.
How
> > can windows get destroyed during cycling? (i.e., why is it necessary to have
> > WindowCycleList be a WindowObserver?)
> > 
> > If it is possible for windows to be destroyed during a cycle session, you
> should
> > definitely explain that (perhaps in your comment block at line 47)
> 
> Some factors like JS close method can trigger a window destruction without
> interaction from the user. 

OK, sounds good. Does a similar possibility exist for new windows being opened
during cycling?

> If we don't take that into account, we could try to
> dereference a dead pointer. Should I write that in a comment, then? I don't
> think there's a design to allow the user to close windows from alt-tab mode.

Yes, I would add a brief comment to explain why it's there, since someone may
otherwise get the impression that the user is somehow able to close windows
during cycling.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
This header will also need to be updated (same for
window_cycle_controller_unittest.cc, window_cycle_list.h, window_cycle_list.cc.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:37: // Watch for all keyboard events by
filtering the root window.
I don't see what value this comment adds, so I suggest removing it.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:85: void
WindowCycleController::AltKeyReleased() {
Since all AltKeyReleased() does is call StopCycling(), I think we should remove
AltKeyReleased() and instead just call StopCycling() directly.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.h:52: const { return window_cycle_list_.get(); }
Change formatting:

const WindowCycleList* window_cycle_list() const {
  return window_cycle_list_.get();
}

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.h:55: // Call to start cycling windows.
Also mention that this function adds the pre-target handler.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list.cc
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:116: ++i)
{} around loop body since multiple lines

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:147: DCHECK(windows_[current_index_]);
new line after line 147

[Nina, 2014-05-02 13:27:33]

I took care of the issues Terry found. Can you review this patch, Rob? Most of
it is code you are probably familiar with - classes WindowCycleController and
WindowCycleList

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:45: virtual void OnWindowDestroyed(aura::Window*
window) OVERRIDE;
On 2014/05/01 21:58:33, tdanderson wrote:
> On 2014/05/01 20:41:16, Nicolás wrote:
> > On 2014/05/01 18:51:39, tdanderson wrote:
> > > IIUC, a WindowCycleList only exists while Alt+Tab cycling is in progress.
> How
> > > can windows get destroyed during cycling? (i.e., why is it necessary to
have
> > > WindowCycleList be a WindowObserver?)
> > > 
> > > If it is possible for windows to be destroyed during a cycle session, you
> > should
> > > definitely explain that (perhaps in your comment block at line 47)
> > 
> > Some factors like JS close method can trigger a window destruction without
> > interaction from the user. 
> 
> OK, sounds good. Does a similar possibility exist for new windows being opened
> during cycling?
> 
> > If we don't take that into account, we could try to
> > dereference a dead pointer. Should I write that in a comment, then? I don't
> > think there's a design to allow the user to close windows from alt-tab mode.
> 
> Yes, I would add a brief comment to explain why it's there, since someone may
> otherwise get the impression that the user is somehow able to close windows
> during cycling.

Done. I don't know if it's possible to observe the creation of new windows.
Anyway that is not too important - if a window is created while window cycling,
you won't be able to get to it until you release alt and start cycling again but
chrome won't explode.

https://codereview.chromium.org/260883005/diff/1/ash/wm/window_cycle_list.h#n...
ash/wm/window_cycle_list.h:53: // Current position in the |windows_| list or -1
if we're not cycling.
On 2014/05/01 18:51:39, tdanderson wrote:
> Similar comment to the one above: if this object exists, doesn't that mean
we're
> currently cycling? I don't think this should ever be allowed to have a -1
value.
> See my comment in window_cycle_list.cc, line 144.

Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2014/05/01 21:58:33, tdanderson wrote:
> This header will also need to be updated (same for
> window_cycle_controller_unittest.cc, window_cycle_list.h,
window_cycle_list.cc.

Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:37: // Watch for all keyboard events by
filtering the root window.
On 2014/05/01 21:58:33, tdanderson wrote:
> I don't see what value this comment adds, so I suggest removing it.

I think it slipped from a deleted function. Removed.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.cc:85: void
WindowCycleController::AltKeyReleased() {
On 2014/05/01 21:58:33, tdanderson wrote:
> Since all AltKeyReleased() does is call StopCycling(), I think we should
remove
> AltKeyReleased() and instead just call StopCycling() directly.

Good catch. Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.h:52: const { return window_cycle_list_.get(); }
On 2014/05/01 21:58:33, tdanderson wrote:
> Change formatting:
> 
> const WindowCycleList* window_cycle_list() const {
>   return window_cycle_list_.get();
> }

Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_cont...
ash/wm/window_cycle_controller.h:55: // Call to start cycling windows.
On 2014/05/01 21:58:33, tdanderson wrote:
> Also mention that this function adds the pre-target handler.

Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list.cc
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:116: ++i)
On 2014/05/01 21:58:33, tdanderson wrote:
> {} around loop body since multiple lines

Done.

https://codereview.chromium.org/260883005/diff/20001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:147: DCHECK(windows_[current_index_]);
On 2014/05/01 21:58:33, tdanderson wrote:
> new line after line 147

Done.

[flackr, 2014-05-05 13:58:56]

Can you try uploading with a very low similarity value, i.e.:
git cl upload --similarity=20 or so, in order to get the window_cycle_* files to
show how they have changed from window_selector_*?

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
File ash/wm/overview/window_overview.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.h:87: void PositionWindowsOnRoot(aura::Window*
root_window,
Remove this, can be merged with PositionWindowsFromRoot since this is no longer
called directly.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.h:115: aura::Window* single_root_window_;
Remove this.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:37: enum Direction {
This isn't necessary anymore right?

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:54: new WindowSelector(windows,
this));
nit: one line.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list.cc
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:165: // If the first window was destroyed, move the
index to the new first window.
If the first window was destroyed, either current_index == removed_index in
which case is still 0, or current_index > removed_index in which case is at
least 0 now. The only way it would become -1 is if the windows list is now
empty, which we should probably have a way of handling.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.h:18: class ScopedShowWindow;
nit: newline after "namespace ash {"

[Nina, 2014-05-05 15:44:28]

Fixed tests, fixed comments. Would you mind reviewing again? Thanks!

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
File ash/wm/overview/window_overview.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.h:87: void PositionWindowsOnRoot(aura::Window*
root_window,
On 2014/05/05 13:58:56, flackr wrote:
> Remove this, can be merged with PositionWindowsFromRoot since this is no
longer
> called directly.

Done.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_o...
ash/wm/overview/window_overview.h:115: aura::Window* single_root_window_;
On 2014/05/05 13:58:56, flackr wrote:
> Remove this.

Done.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:37: enum Direction {
On 2014/05/05 13:58:56, flackr wrote:
> This isn't necessary anymore right?

TBH we can remove it until we land the arrow key navigation, yes.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.cc:54: new WindowSelector(windows,
this));
On 2014/05/05 13:58:56, flackr wrote:
> nit: one line.

Done.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list.cc
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.cc:165: // If the first window was destroyed, move the
index to the new first window.
On 2014/05/05 13:58:56, flackr wrote:
> If the first window was destroyed, either current_index == removed_index in
> which case is still 0, or current_index > removed_index in which case is at
> least 0 now. The only way it would become -1 is if the windows list is now
> empty, which we should probably have a way of handling.

Hmm this is true, and if there are no windows calling step has no effect. I
think I'll remove the if.

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/60001/ash/wm/window_cycle_list...
ash/wm/window_cycle_list.h:18: class ScopedShowWindow;
On 2014/05/05 13:58:56, flackr wrote:
> nit: newline after "namespace ash {"

Done.

[tdanderson, 2014-05-05 15:54:56]

LGTM with some nits below.

https://codereview.chromium.org/260883005/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/260883005/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_overview.cc:152: // Send an a11y alert
. at end

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.h:45: // List is built when the user initiates the
gesture (e.g. hits alt-tab the
i.e. instead of e.g.

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.h:46: // first time) and is emptied when the gesture is
complete (e.g. releases the
same here

[Nina, 2014-05-05 16:07:30]

Fixed nits

https://codereview.chromium.org/260883005/diff/140001/ash/wm/overview/window_...
File ash/wm/overview/window_overview.cc (right):

https://codereview.chromium.org/260883005/diff/140001/ash/wm/overview/window_...
ash/wm/overview/window_overview.cc:152: // Send an a11y alert
On 2014/05/05 15:54:57, tdanderson wrote:
> . at end

Done.

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_list.h
File ash/wm/window_cycle_list.h (right):

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.h:45: // List is built when the user initiates the
gesture (e.g. hits alt-tab the
On 2014/05/05 15:54:57, tdanderson wrote:
> i.e. instead of e.g.

Done.

https://codereview.chromium.org/260883005/diff/140001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.h:46: // first time) and is emptied when the gesture is
complete (e.g. releases the
On 2014/05/05 15:54:57, tdanderson wrote:
> same here

Done.

[flackr, 2014-05-06 02:21:36]

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector.h:24: class ScopedShowWindow;
Remove this.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector.h:44: void SelectWindow();
No such thing as "selected" window at present.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector.h:90: size_t selected_window_;
I'd probably remove this for now.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:159: void FireOverviewStartTimer() {
This shouldn't be necessary anymore. This was for entering overview on cycling.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:77: // listen for the alt key being released
to stop cycling.
This comment doesn't really seem to match the code, it's probably fine to have
no comment here.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.h:32: };
nit: newline before constructor

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:555: // Cycling again should now
select window3.
window2?

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:582: // Cycling again should now
select window3.
panel1?

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:166: current_index_ ==
static_cast<int>(windows_.size()))
nit: braces { } since if condition is multi-line.

[Nina, 2014-05-06 12:46:04]

Hi Rob, can you give the patch another look? Thanks!

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector.h:44: void SelectWindow();
On 2014/05/06 02:21:37, flackr wrote:
> No such thing as "selected" window at present.

Done.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:159: void FireOverviewStartTimer() {
On 2014/05/06 02:21:37, flackr wrote:
> This shouldn't be necessary anymore. This was for entering overview on
cycling.

Done.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:77: // listen for the alt key being released
to stop cycling.
On 2014/05/06 02:21:37, flackr wrote:
> This comment doesn't really seem to match the code, it's probably fine to have
> no comment here.

Removed.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.h:32: };
On 2014/05/06 02:21:37, flackr wrote:
> nit: newline before constructor

Done.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller_unittest.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:555: // Cycling again should now
select window3.
On 2014/05/06 02:21:37, flackr wrote:
> window2?

I updated the name of the vars and forgot to change the comments. Fixed.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller_unittest.cc:582: // Cycling again should now
select window3.
On 2014/05/06 02:21:37, flackr wrote:
> panel1?

Done.

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/160001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:166: current_index_ ==
static_cast<int>(windows_.size()))
On 2014/05/06 02:21:37, flackr wrote:
> nit: braces { } since if condition is multi-line.

Done.

[flackr, 2014-05-07 00:07:54]

Few more comments, but otherwise good.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:7: #include <algorithm>
Don't think this is used.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:11: #include "ash/shell_window_ids.h"
This isn't used either.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:14: #include "ash/wm/window_util.h"
or this.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:15: #include "ash/wm/workspace_controller.h"
Possibly not this either.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:61: StopCycling();
With the comment on line 86 fixed, this probably doesn't need to be called.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:86:
Shell::GetInstance()->AddPreTargetHandler(event_handler_.get());
Add and remove the pretarget handler from the WindowCycleEventFilter constructor
/ destructor to avoid possibly forgetting to remove it.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:100: if (event_handler_) {
And with the comment on line 86, event_handler_ can be reset unconditionally.

[Nina, 2014-05-07 14:55:42]

I think I nailed your comments, Rob, but I'm not 100% sure if the implementation
is as you expected. Could you look at the patch again?

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:7: #include <algorithm>
On 2014/05/07 00:07:55, flackr wrote:
> Don't think this is used.

Removed. However, I get a warning from lint if I remove it (the code still
works).

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:11: #include "ash/shell_window_ids.h"
On 2014/05/07 00:07:55, flackr wrote:
> This isn't used either.

Done.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:14: #include "ash/wm/window_util.h"
On 2014/05/07 00:07:55, flackr wrote:
> or this.

Done.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:15: #include "ash/wm/workspace_controller.h"
On 2014/05/07 00:07:55, flackr wrote:
> Possibly not this either.

Done.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:61: StopCycling();
On 2014/05/07 00:07:55, flackr wrote:
> With the comment on line 86 fixed, this probably doesn't need to be called.

Done.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:86:
Shell::GetInstance()->AddPreTargetHandler(event_handler_.get());
On 2014/05/07 00:07:55, flackr wrote:
> Add and remove the pretarget handler from the WindowCycleEventFilter
constructor
> / destructor to avoid possibly forgetting to remove it.

Done.

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:100: if (event_handler_) {
On 2014/05/07 00:07:55, flackr wrote:
> And with the comment on line 86, event_handler_ can be reset unconditionally.

Done.

[flackr, 2014-05-07 16:03:45]

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/200001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:85: event_handler_.reset(new
WindowCycleEventFilter());
Sorry, didn't mean to move the event_handler_ construction, just to move the add
pretarget handler to the WindowCycleEventFilter constructor.

https://codereview.chromium.org/260883005/diff/220001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/220001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:53:
Shell::GetInstance()->AddPreTargetHandler(event_handler_.get());
I meant to move these to the WindowCycleEventFilter constructor and destructor.
See for example WindowSelectorEventFilter
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi...

https://codereview.chromium.org/260883005/diff/220001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:58: event_handler_.reset();
This is not necessary, the scoped_ptr is destructed when the object is
destructed.

[Nina, 2014-05-07 17:07:34]

Mind taking another look? This time I think I got it right.

[flackr, 2014-05-07 21:21:32]

LGTM with nit.

https://codereview.chromium.org/260883005/diff/240001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/240001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:51: 
nit: no double newline here. Was this meant to be all /'s like line 84?

[Nina, 2014-05-07 21:36:04]

Sky, can you review this patch? Especially files shell.cc, shell.h,
accelerator_controller_unittest.cc and accelerator_controller.cc need review
from an owner.

Thanks!

https://codereview.chromium.org/260883005/diff/240001/ash/wm/window_cycle_con...
File ash/wm/window_cycle_controller.cc (right):

https://codereview.chromium.org/260883005/diff/240001/ash/wm/window_cycle_con...
ash/wm/window_cycle_controller.cc:51: 
On 2014/05/07 21:21:32, flackr wrote:
> nit: no double newline here. Was this meant to be all /'s like line 84?

Yep, but the fact I suck at vim plus my keyboard has no caps lock light
sometimes works sorcery against me.

[sky, 2014-05-07 22:28:16]

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:21: aura::Window* below = NULL;
Can't you use std::find here?

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:62: };
nit: when you wrap one param per line.

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:68: void ScopedShowWindow::Show(aura::Window*
window) {
definition/declaration order should match.

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:112: : windows_(windows), current_index_(0) {
nit: wrap.

[Nina, 2014-05-08 13:35:59]

Sky, I took care of the issues you found. Mind giving it another look?

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
File ash/wm/window_cycle_list.cc (right):

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:21: aura::Window* below = NULL;
On 2014/05/07 22:28:17, sky wrote:
> Can't you use std::find here?

Refactored to use std::find

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:62: };
On 2014/05/07 22:28:17, sky wrote:
> nit: when you wrap one param per line.

Done.

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:68: void ScopedShowWindow::Show(aura::Window*
window) {
On 2014/05/07 22:28:17, sky wrote:
> definition/declaration order should match.

Done.

https://codereview.chromium.org/260883005/diff/260001/ash/wm/window_cycle_lis...
ash/wm/window_cycle_list.cc:112: : windows_(windows), current_index_(0) {
On 2014/05/07 22:28:17, sky wrote:
> nit: wrap.

Done.

[sky, 2014-05-08 16:04:52]

LGTM

[tdanderson, 2014-05-12 15:16:03]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-05-12 15:16:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/260883005/300001

[tdanderson, 2014-05-12 19:03:33]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-05-12 19:05:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/260883005/320001

[commit-bot: I haz the power, 2014-05-12 22:57:09]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-13 03:44:16]

Change committed as 269997