Add user action for changing the active window in Ash overview mode

Add user action for changing the active window in Ash overview mode

Introduce the user action
WindowSelector_ActiveWindowChanged to track
the number of overview mode sessions in which
the user selected a window that is different
from the previously-active window.

BUG=486900
TEST=WindowSelectorTest.ActiveWindowChangedUserAction*

Committed: https://crrev.com/ec8ba66f8e43042b4115ef911955db19db62191f
Cr-Commit-Position: refs/heads/master@{#329915}

[tdanderson, 2015-05-12 16:02:41]

tdanderson@chromium.org changed reviewers:
+ oshima@chromium.org

[tdanderson, 2015-05-12 16:02:42]

@oshima, can you please take a look?

[bruthig, 2015-05-12 17:37:37]

bruthig@chromium.org changed reviewers:
+ bruthig@chromium.org

[bruthig, 2015-05-12 17:37:38]

Drive-by: Added some comments inline.

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.cc:89: aura::Window* active =
wm::GetActiveWindow();
I feel like this metric should only be recorded if the window change was
actually due to someone selecting a window from the WindowSelector UI.

I'm concerned that the |active| window here could be set by a different task
switching mechanism (ie alt+tab or clicking a docked window) and that this
metric will be double counted in the those two buckets.

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.cc:98: void
WindowSelectorController::OnSelectionStarting() {
It would be less invasive to WindowSelectorController and would allow the future
work for the catch-all TaskChanged metric to be centralized if
OnSelectionStarting, OnSelectionStarted, OnSelectionEnding and OnSelectionEnded
were events that a WindowSelectorObserver.

It is worth considering but may be too risky this close to branch cut.

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

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_unittest.cc:454: // when the oply open window is
closed while in overview mode.
Sp? "oply"

[oshima, 2015-05-12 20:25:13]

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.cc:89: aura::Window* active =
wm::GetActiveWindow();
On 2015/05/12 17:37:38, bruthig wrote:
> I feel like this metric should only be recorded if the window change was
> actually due to someone selecting a window from the WindowSelector UI.
> 
> I'm concerned that the |active| window here could be set by a different task
> switching mechanism (ie alt+tab or clicking a docked window) and that this
> metric will be double counted in the those two buckets.

This may also change if the window is closed or an app requested focus
explicitly. It's probably rare, and I don't have strong opinion here so let me
know when both of you agreed, and I'll review again.

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.h:68: // such a window exists
(nullptr otherwise).
document that the pointer may be stale (if it's closed during overview mode).
you may actually reset the pointer when the window is destroyed, but I'll leave
it to you, as its content isn't accessed.

[tdanderson, 2015-05-14 01:36:32]

@oshima and @bruthig, I've completely changed the approach of this CL. Can you
please take a look? Now we're only recording the user action when the user
actually uses the mouse/keyboard/touch to select a window while in overview mode
(different from the previously-active window).

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.cc:89: aura::Window* active =
wm::GetActiveWindow();
On 2015/05/12 20:25:13, oshima wrote:
> On 2015/05/12 17:37:38, bruthig wrote:
> > I feel like this metric should only be recorded if the window change was
> > actually due to someone selecting a window from the WindowSelector UI.
> > 
> > I'm concerned that the |active| window here could be set by a different task
> > switching mechanism (ie alt+tab or clicking a docked window) and that this
> > metric will be double counted in the those two buckets.
> 
> This may also change if the window is closed or an app requested focus
> explicitly. It's probably rare, and I don't have strong opinion here so let me
> know when both of you agreed, and I'll review again.

In patch set 2 the user action won't be recorded when closing windows in
overview mode. It will still be recorded if a window requests focus
programmatically, but that is rare and Ben and I have agreed we can live with
having that small amount of noise in the data.

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.cc:98: void
WindowSelectorController::OnSelectionStarting() {
On 2015/05/12 17:37:38, bruthig wrote:
> It would be less invasive to WindowSelectorController and would allow the
future
> work for the catch-all TaskChanged metric to be centralized if
> OnSelectionStarting, OnSelectionStarted, OnSelectionEnding and
OnSelectionEnded
> were events that a WindowSelectorObserver.
> 
> It is worth considering but may be too risky this close to branch cut.

Not pursuing this as per in-person discussion.

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_controller.h:68: // such a window exists
(nullptr otherwise).
On 2015/05/12 20:25:13, oshima wrote:
> document that the pointer may be stale (if it's closed during overview mode).
> you may actually reset the pointer when the window is destroyed, but I'll
leave
> it to you, as its content isn't accessed.

I've removed this pointer in patch set 2.

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

https://codereview.chromium.org/1138253002/diff/1/ash/wm/overview/window_sele...
ash/wm/overview/window_selector_unittest.cc:454: // when the oply open window is
closed while in overview mode.
On 2015/05/12 17:37:38, bruthig wrote:
> Sp? "oply"

Done.

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
ash/wm/overview/window_selector_item.h:165: WindowSelector* window_selector_;
Or should we instead have a non-owned pointer back to our owning WindowGrid
(which in turn has a non-owned pointer back to its owning WindowSelector)?

[bruthig, 2015-05-14 14:03:18]

lgtm

Ready for your review, oshima@.

[oshima, 2015-05-14 17:07:19]

lgtm with alternative approach.

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:406: }
Alternatively, you can create temporal activation change listener, and
update UMA when activation changed during the Activate below.

I'll leave it to you though.

[tdanderson, 2015-05-14 18:19:27]

tdanderson@chromium.org changed reviewers:
+ mpearson@chromium.org

[tdanderson, 2015-05-14 18:19:28]

oshima@, does this still LGTY with the tests I've added in patch set 3?

mpearson@, can you please take a look at tools/ ?

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/1138253002/diff/20001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:406: }
On 2015/05/14 17:07:19, oshima wrote:
> Alternatively, you can create temporal activation change listener, and
> update UMA when activation changed during the Activate below.
> 
> I'll leave it to you though.

Thanks for the suggestion, but I think I'll leave it as-is.

[Mark P, 2015-05-14 18:25:13]

histograms.xml lgtm

[oshima, 2015-05-14 19:55:42]

On 2015/05/14 18:25:13, Mark P wrote:
> histograms.xml lgtm

slgtm

[oshima, 2015-05-14 19:56:02]

ugh. still lgtm

[tdanderson, 2015-05-14 19:56:43]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2015-05-14 19:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/1138253002/#ps40001
(title: "tests added")

[commit-bot: I haz the power, 2015-05-14 19:57:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1138253002/40001

[commit-bot: I haz the power, 2015-05-14 20:09:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-14 20:10:05]

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