Separated alt-tab window cycle from overview mode

ash/wm/window_cycle_controller.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[tdanderson, 2014/05/01 18:51:39]

Make sure you have the right headers for new files (correct format is at
http://www.chromium.org/developers/coding-style).

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_WM_WINDOW_CYCLE_CONTROLLER_H_
+#define ASH_WM_WINDOW_CYCLE_CONTROLLER_H_
+
+#include "ash/ash_export.h"
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+
+namespace ui {
+class EventHandler;
+}
+
+namespace ash {
+
+class WindowCycleList;
+
+// Controls cycling through windows with the keyboard, for example, via alt-tab.

[tdanderson, 2014/05/01 18:51:39]

"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?

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

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.

+// Windows are sorted primarily by most recently used, and then by screen order.
+// We activate windows as you cycle through them, so the order on the screen
+// may change during the gesture, but the most recently used list isn't updated
+// until the cycling ends.  Thus we maintain the state of the windows
+// at the beginning of the gesture so you can cycle through in a consistent
+// order.
+class ASH_EXPORT WindowCycleController {
+ public:
+  enum Direction {

[tdanderson, 2014/05/01 18:51:39]

It feels unnecessary to define identical enums in both WindowCycleController and
WindowCycleList.

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

Done.

+    FORWARD,
+    BACKWARD
+  };
+  WindowCycleController();
+  virtual ~WindowCycleController();
+
+  // Returns true if cycling through windows is enabled. This is false at
+  // certain times, such as when the lock screen is visible.
+  static bool CanCycle();
+
+  // Cycles between windows in the given |direction|. If |is_alt_down| then

[tdanderson, 2014/05/01 18:51:39]

You mention |is_alt_down| in this comment, but I don't see the variable
anywhere.

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

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

+  // interprets this call as the start of a multi-step cycle sequence and
+  // installs a key filter to watch for alt being released.
+  void HandleCycleWindow(Direction direction);
+
+  // Cycles between windows without maintaining a multi-step cycle sequence

[tdanderson, 2014/05/01 18:51:39]

I don't understand what a "multi-step cycle sequence" is based on the comments
in this header file.

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

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

+  // (see above).

[tdanderson, 2014/05/01 18:51:39]

Avoid saying above/below in comments, since the order of functions may change,
etc. 

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

Gotcha.

+  void HandleLinearCycleWindow();

[tdanderson, 2014/05/01 18:51:39]

I don't see where HandleLinearCycleWindow() is called.

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

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

+
+  // Informs the controller that the Alt key has been released and it can
+  // terminate the existing multi-step cycle.
+  void AltKeyReleased();
+
+  // Returns true if we are in the middle of a window cycling gesture.
+  bool IsCycling() const { return windows_.get() != NULL; }
+
+  // Returns the WindowCycleList. Really only useful for testing.
+  const WindowCycleList* windows() const { return windows_.get(); }
+
+ private:
+  // Call to start cycling windows.  You must call StopCycling() when done.

[tdanderson, 2014/05/01 18:51:39]

"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.

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

Done.

+  void StartCycling();
+
+  // Cycles to the next or previous window based on |direction|.
+  void Step(Direction direction);
+
+  // Installs an event filter to watch for release of the alt key.
+  void InstallEventFilter();
+
+  // Stops the current window cycle and cleans up the event filter.

[tdanderson, 2014/05/01 18:51:39]

"cleans up" -> "removes"

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

Done.

+  void StopCycling();
+
+  scoped_ptr<WindowCycleList> windows_;

[tdanderson, 2014/05/01 18:51:39]

I'd prefer this to be called window_cycle_list_ or something other than just
windows_.

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

Done.

+
+  // Event handler to watch for release of alt key.
+  scoped_ptr<ui::EventHandler> event_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowCycleController);
+};
+
+}  // namespace ash
+
+#endif  // ASH_WM_WINDOW_CYCLE_CONTROLLER_H_