mus: Fix activation cycle direction.

mus: Fix activation cycle direction.

When moving activation (e.g. either because of window-switch accelerator,
or when the active window is destroyed/hidden), make sure the previous
active window is activated first, instead of the oldest active window.
However, when explicitly cycling through the active windows (e.g. by using
the window-cycle accelerator), make sure it cycles through all activatable
windows, instead of just the two topmost windows.

BUG=548422
Committed: https://crrev.com/f1503e37c092083c0efde38f8f64884e5fb852fa
Cr-Commit-Position: refs/heads/master@{#368460}

[sadrul, 2016-01-05 16:29:06]

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

[sky, 2016-01-05 16:42:38]

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
File components/mus/ws/focus_controller.cc (right):

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
components/mus/ws/focus_controller.cc:19: ServerWindow*
GetDeepestFirstDescendant(ServerWindow* window) {
Update function name to reflect new meaning.

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
File components/mus/ws/focus_controller.h (right):

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
components/mus/ws/focus_controller.h:84: std::vector<WindowId> cycle_windows_;
As far as I can tell you don't update this as windows are destroyed or moved
around in the hierarchy. Doesn't it need to be updated when either of those
happen?

[sadrul, 2016-01-05 17:14:53]

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
File components/mus/ws/focus_controller.h (right):

https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
components/mus/ws/focus_controller.h:84: std::vector<WindowId> cycle_windows_;
On 2016/01/05 16:42:38, sky wrote:
> As far as I can tell you don't update this as windows are destroyed or moved
> around in the hierarchy. Doesn't it need to be updated when either of those
> happen?

We don't need to handle new windows, since those would either be activated
explicitly, thus breaking the cycle (|activation_reason_| would change), or will
be activated based on the usual stacking order traversal.

For windows that are destroyed: we won't find them as activation-candidate, so
even if they are in the list, the right window will be activated. Two caveats
though: (1) we would be cycling over all windows multiple times when we erase an
already destroyed window from |cycle_windows_|, and (2) cycle_windows_ keeps
growing until the activation-cycle is broken. To fix this: we could either
introduce a ServerWindowTracker and use that, instead of this std::vector<>, or
introduce a IsWindowAlive()/IsWindowValid() to FocusControllerDelegate, and use
that when removing from |cycle_windows_|, and keep removing until we remove a
valid window. I prefer the latter option; I think it's simpler. WDYT?

[sky, 2016-01-05 19:17:20]

On Tue, Jan 5, 2016 at 9:14 AM,  <sadrul@chromium.org> wrote:
>
>
https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
> File components/mus/ws/focus_controller.h (right):
>
>
https://codereview.chromium.org/1560063003/diff/1/components/mus/ws/focus_con...
> components/mus/ws/focus_controller.h:84: std::vector<WindowId>
> cycle_windows_;
> On 2016/01/05 16:42:38, sky wrote:
>>
>> As far as I can tell you don't update this as windows are destroyed or
>
> moved
>>
>> around in the hierarchy. Doesn't it need to be updated when either of
>
> those
>>
>> happen?
>
>
> We don't need to handle new windows, since those would either be
> activated explicitly, thus breaking the cycle (|activation_reason_|
> would change), or will be activated based on the usual stacking order
> traversal.
>
> For windows that are destroyed: we won't find them as
> activation-candidate, so even if they are in the list, the right window
> will be activated.

That's assuming an id isn't reused, right? Seems safest to me to
listen for windows being destroyed and prune from the list. I much
prefer maintaining only valid objects.

> Two caveats though: (1) we would be cycling over all
> windows multiple times when we erase an already destroyed window from
> |cycle_windows_|, and (2) cycle_windows_ keeps growing until the
> activation-cycle is broken. To fix this: we could either introduce a
> ServerWindowTracker and use that, instead of this std::vector<>, or
> introduce a IsWindowAlive()/IsWindowValid() to FocusControllerDelegate,
> and use that when removing from |cycle_windows_|, and keep removing
> until we remove a valid window. I prefer the latter option; I think it's
> simpler. WDYT?

I prefer having the vector only contain valid objects (ids). Much less
error prone that way.

  -Scott

>
> https://codereview.chromium.org/1560063003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2016-01-06 18:00:38]

Updated to make sure cycle_windows_ never holds on to a destroyed window. (I
didn't introduce a ServerWindowTracker though)

[sky, 2016-01-06 21:02:39]

LGTM

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
File components/mus/ws/focus_controller.cc (right):

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
components/mus/ws/focus_controller.cc:100: } else if
(std::find(cycle_windows_.begin(), cycle_windows_.end(),
FYI: you can use ContainsValue(cycle_windows_, activate) if you prefer (recently
added to stl_util.h).

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
components/mus/ws/focus_controller.cc:158: active_window_->AddObserver(this);
nit: add an AddWindowToCycleWindows (or something similarly named) and use it on
147-148 as well.

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
components/mus/ws/focus_controller.cc:314: std::find(cycle_windows_.begin(),
cycle_windows_.end(), window));
also remove the observer here too.

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
File components/mus/ws/focus_controller.h (right):

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
components/mus/ws/focus_controller.h:91: std::vector<ServerWindow*>
cycle_windows_;
It's not obvious what this is, or how it changes based on the name (nor do I
think you could come up with a name that would convey it). Please add a
description.

[sadrul, 2016-01-08 20:40:26]

I have updated the CL to use ServerWindowTracker. Would you mind taking another
look? Thanks!

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
File components/mus/ws/focus_controller.h (right):

https://codereview.chromium.org/1560063003/diff/20001/components/mus/ws/focus...
components/mus/ws/focus_controller.h:91: std::vector<ServerWindow*>
cycle_windows_;
On 2016/01/06 21:02:39, sky wrote:
> It's not obvious what this is, or how it changes based on the name (nor do I
> think you could come up with a name that would convey it). Please add a
> description.

Done.

[sky, 2016-01-08 22:06:13]

SLGTM

[sadrul, 2016-01-08 22:49:52]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-01-08 22:49:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1560063003/#ps60001
(title: "tot.merge")

[commit-bot: I haz the power, 2016-01-08 22:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1560063003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1560063003/60001

[commit-bot: I haz the power, 2016-01-08 23:49:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-08 23:51:06]

Description was changed from

==========
mus: Fix activation cycle direction.

When moving activation (e.g. either because of window-switch accelerator,
or when the active window is destroyed/hidden), make sure the previous
active window is activated first, instead of the oldest active window.
However, when explicitly cycling through the active windows (e.g. by using
the window-cycle accelerator), make sure it cycles through all activatable
windows, instead of just the two topmost windows.

BUG=548422
==========

to

==========
mus: Fix activation cycle direction.

When moving activation (e.g. either because of window-switch accelerator,
or when the active window is destroyed/hidden), make sure the previous
active window is activated first, instead of the oldest active window.
However, when explicitly cycling through the active windows (e.g. by using
the window-cycle accelerator), make sure it cycles through all activatable
windows, instead of just the two topmost windows.

BUG=548422

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

[commit-bot: I haz the power, 2016-01-08 23:51:07]

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