Added UMA statistics for changing the active window via click or touch events.

Added UMA statistics for changing the active window via click or touch events.

TEST=TaskSwitchMetricsRecorderTest.VerifyTaskSwitchesForDesktopAreRecorded
TEST=DesktopTaskSwitchMetricRecorderTest.ActivatePositionableWindowWhenNullWindowWasActivatedLast
TEST=DesktopTaskSwitchMetricRecorderTest.ActivatePositionableWindowWhenADifferentPositionableWindowWasActivatedLast
TEST=DesktopTaskSwitchMetricRecorderTest.ActivatePositionableWindowWhenTheSamePositionableWindowWasActivatedLast
TEST=DesktopTaskSwitchMetricRecorderTest.ActivatePositionableWindowWhenANonPositionableWindowWasActivatedLast
TEST=DesktopTaskSwitchMetricRecorderTest.ActivateNonPositionableWindowBetweenTwoPositionableWindowActivations
TEST=DesktopTaskSwitchMetricRecorderTest.ActivateNullWindow
TEST=DesktopTaskSwitchMetricRecorderTest.ActivateNonPositionableWindow
TEST=DesktopTaskSwitchMetricRecorderTest.ActivatePositionableWindowWithNonInputEventReason
TEST=DesktopTaskSwitchMetricRecorderWithShellIntegrationTest.ActivatePositionableWindowWithInputEvent
TEST=DesktopTaskSwitchMetricRecorderWithShellIntegrationTest.ActivatePositionableWindowWithNonInputEvent

BUG=489813, 489814
Committed: https://crrev.com/9a312138c831f3203ddfa6dff5650bf663acb44f
Cr-Commit-Position: refs/heads/master@{#333104}

[bruthig, 2015-05-25 17:46:02]

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

[bruthig, 2015-05-25 17:46:03]

tdanderson@, can you please take a look from a design/feature correctness
perspective before I harden it with documentation and tests?  I'm also
interested in different variable/class name suggestions.

[tdanderson, 2015-05-26 21:13:53]

Ben, please see my feedback below:

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/screen_event_ta...
File ash/metrics/screen_event_task_switch_metric_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/screen_event_ta...
ash/metrics/screen_event_task_switch_metric_recorder.h:35: aura::Window*
active_window_before_activation_ = nullptr;
initialize to null in constructor

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.cc:16:
"Ash.Screen.TimeBetweenNavigateToTaskSwitches";
don't forget to update histograms.xml

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.cc:75: return window && (window->type()
== ui::wm::WINDOW_TYPE_NORMAL ||
It might be nice to define this as a static utility function somewhere in
ash/wm. Six other places in the code perform this same check, and they could
instead call into the utility function:

* CanHandleWindowSnapOrDock() in accelerator_controller.cc

* RootWindowController::GetWindowForFullscreenMode()

* DragWindowResizer::ShouldAllowMouseWarp()

* WindowSelector::IsSelectable()

...and be sure to dust off your notes from logic class because you'll be needing
DeMorgan's law for the last two:

* WindowWatcher::OnWindowAdded()

* DefaultState::ProcessWorkspaceEvents()

The benefit here is not just saving ~6 lines of code, but in naming the utility
function to describe a unique characteristic of windows which are normal or
panel types. One suggestion is IsWindowUserPositionable() but perhaps that isn't
the best. Whatever name you pick can be used in place of "task window".

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.h:35: kScreen,
Since these values are being used to indicate the UI surface where the task
switch took place, how about naming this kDesktop or kWorkspace?

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/user_metrics_re...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/user_metrics_re...
ash/metrics/user_metrics_recorder.cc:298:
base::RecordAction(base::UserMetricsAction("ScreenEvent_SwitchTask"));
don't forget to update actions.xml

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/focus_controller.h
File ui/wm/core/focus_controller.h (left):

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/focus_controller...
ui/wm/core/focus_controller.h:31: // . input events (implemented here in
ui::EventHandler).
Be sure the documentation in FocusController is still up to date.

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/window_activatio...
File ui/wm/core/window_activation_pre_target_handler.h (right):

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/window_activatio...
ui/wm/core/window_activation_pre_target_handler.h:24: class WM_EXPORT
WindowActivationPreTargetHandler : public ui::EventHandler {
How about WindowActivationEventHandler?

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/window_activatio...
ui/wm/core/window_activation_pre_target_handler.h:42: void
ActivateWindowFromEvent(ui::Event* event);
nit: members below methods

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/window_activatio...
File ui/wm/core/window_activation_pre_target_handler_observer.h (right):

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/window_activatio...
ui/wm/core/window_activation_pre_target_handler_observer.h:22: virtual void
BeforeWindowActivatedByPreTargetHandler() = 0;
I don't think you need to say "ByPreTargetHandler" in your method names here.

[bruthig, 2015-05-26 21:39:50]

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

[bruthig, 2015-05-26 21:39:51]

sky@, would you be able to take an early look from a design perspective before I
harden it with comments and tests?

[sky, 2015-05-27 15:30:27]

On 2015/05/26 21:39:51, bruthig wrote:
> sky@, would you be able to take an early look from a design perspective before
I
> harden it with comments and tests?

Sure. One meta question though. What do we hope to learn and gain from these
metrics? Will they cause us to do anything different?

[sky, 2015-05-27 15:37:52]

https://codereview.chromium.org/1153633006/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/1153633006/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:484:
window_activation_pretarget_handler_.reset(
I don't like separating out activation from the focus controller. It means you
either have to duplicate checks done in the focuscontroller (bad) or skip them
(worse).

I'm still not clear on what you hope to gain from these metrics, but why not add
the information you need to ActivationChangeObserver::OnWindowActivated?

[bruthig, 2015-06-01 21:21:36]

I've reworked the CL to use the alternative approach.

sky@, can you please have a look at the design/approach before I add unit tests?

tdanderson@, feel free to have a look as well.

[sky, 2015-06-02 15:43:03]

This is the approach I recommended previously, right? Seems fine to me.

https://codereview.chromium.org/1153633006/diff/20001/ui/wm/public/activation...
File ui/wm/public/activation_change_observer.h (right):

https://codereview.chromium.org/1153633006/diff/20001/ui/wm/public/activation...
ui/wm/public/activation_change_observer.h:20: kActivationClient,
In chrome we don't use this style for enums, we use ALL_CAPS. See chrome style
guide for specifics.

[bruthig, 2015-06-03 21:59:30]

bruthig@chromium.org changed reviewers:
+ asvitkine@chromium.org, danakj@chromium.org

[bruthig, 2015-06-03 21:59:32]

*** NOTE: This CL is based off of https://codereview.chromium.org/1157843009/.

tdanderson@, can you please have a look from a feature correctness perspective?
 - */*

sky@, can you please have a look from an owners perspective?
 - Everything thing except histograms.xml and actions.xml

asvitkine@, can you please have a look?
 - histograms.xml
 - actions.xml
 - ash/metrics/desktop_task_switch_metric_recorder.(h|cc)

danakj@, can you please have a look at:
 - ui/aura/window.cc

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/screen_event_ta...
File ash/metrics/screen_event_task_switch_metric_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/screen_event_ta...
ash/metrics/screen_event_task_switch_metric_recorder.h:35: aura::Window*
active_window_before_activation_ = nullptr;
On 2015/05/26 21:13:52, tdanderson wrote:
> initialize to null in constructor

Done.

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
File ash/metrics/task_switch_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.cc:16:
"Ash.Screen.TimeBetweenNavigateToTaskSwitches";
On 2015/05/26 21:13:52, tdanderson wrote:
> don't forget to update histograms.xml

Done.

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.cc:75: return window && (window->type()
== ui::wm::WINDOW_TYPE_NORMAL ||
On 2015/05/26 21:13:52, tdanderson wrote:
> It might be nice to define this as a static utility function somewhere in
> ash/wm. Six other places in the code perform this same check, and they could
> instead call into the utility function:
> 
> * CanHandleWindowSnapOrDock() in accelerator_controller.cc
> 
> * RootWindowController::GetWindowForFullscreenMode()
> 
> * DragWindowResizer::ShouldAllowMouseWarp()
> 
> * WindowSelector::IsSelectable()
> 
> ...and be sure to dust off your notes from logic class because you'll be
needing
> DeMorgan's law for the last two:
> 
> * WindowWatcher::OnWindowAdded()
> 
> * DefaultState::ProcessWorkspaceEvents()
> 
> The benefit here is not just saving ~6 lines of code, but in naming the
utility
> function to describe a unique characteristic of windows which are normal or
> panel types. One suggestion is IsWindowUserPositionable() but perhaps that
isn't
> the best. Whatever name you pick can be used in place of "task window".

Done.

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/task_switch_met...
ash/metrics/task_switch_metrics_recorder.h:35: kScreen,
On 2015/05/26 21:13:52, tdanderson wrote:
> Since these values are being used to indicate the UI surface where the task
> switch took place, how about naming this kDesktop or kWorkspace?

Done.

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/user_metrics_re...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/1/ash/metrics/user_metrics_re...
ash/metrics/user_metrics_recorder.cc:298:
base::RecordAction(base::UserMetricsAction("ScreenEvent_SwitchTask"));
On 2015/05/26 21:13:52, tdanderson wrote:
> don't forget to update actions.xml

Done.

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/focus_controller.h
File ui/wm/core/focus_controller.h (left):

https://codereview.chromium.org/1153633006/diff/1/ui/wm/core/focus_controller...
ui/wm/core/focus_controller.h:31: // . input events (implemented here in
ui::EventHandler).
On 2015/05/26 21:13:52, tdanderson wrote:
> Be sure the documentation in FocusController is still up to date.

Done.

https://codereview.chromium.org/1153633006/diff/20001/ui/wm/public/activation...
File ui/wm/public/activation_change_observer.h (right):

https://codereview.chromium.org/1153633006/diff/20001/ui/wm/public/activation...
ui/wm/public/activation_change_observer.h:20: kActivationClient,
On 2015/06/02 15:43:03, sky wrote:
> In chrome we don't use this style for enums, we use ALL_CAPS. See chrome style
> guide for specifics.

Done.

[danakj, 2015-06-03 22:02:50]

https://codereview.chromium.org/1153633006/diff/40001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/1153633006/diff/40001/ui/aura/window.cc#newco...
ui/aura/window.cc:205: if (layer()) {
Not having a layer means you made a Window but didn't call Init() on it. Fix the
callsite?

[bruthig, 2015-06-03 22:13:54]

Reverted changes to window.cc as per danakj@'s comments.

[sky, 2015-06-03 23:31:14]

Oy, large patches are painful all around. How about separating this into some
smaller patches? At a minimum separating out adding the reason. Additionally the
change to ash/accelerators/accelerator_controller.cc seems separate as well.

[bruthig, 2015-06-04 17:47:32]

*** NOTE: This CL is based off of https://codereview.chromium.org/1151133003/.

I've split out the changes to the ActivationChangedObserver.

tdanderson@, can you please have a look from a feature correctness perspective?
 - */*

sky@, can you please have a look from an owners perspective?
 - Everything thing except histograms.xml and actions.xml

asvitkine@, can you please have a look?
 - histograms.xml
 - actions.xml
 - ash/metrics/desktop_task_switch_metric_recorder.(h|cc)

[Alexei Svitkine (slow), 2015-06-04 18:16:04]

https://codereview.chromium.org/1153633006/diff/100001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1153633006/diff/100001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:2509: it in the 2-dimensional screen space.
I find this action description pretty confusing. I think this needs some context
- i.e. that this action is ChromeOS specific and related to the task switcher.
(Also not sure why you mention "2-dimensional screen space" ... as opposed to
what?)

[sky, 2015-06-04 20:10:52]

LGTM

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/task_switc...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/task_switc...
ash/metrics/task_switch_metrics_recorder.h:33: kDesktop,
For the record this style is wrong for chrome code and should be converted.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
ash/metrics/user_metrics_recorder.cc:608: if
(!desktop_task_switch_metric_recorder_) {
Please add comment as to why the conditional.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
ash/metrics/user_metrics_recorder.cc:615: if
(desktop_task_switch_metric_recorder_)
Conditional is not needed.

[tdanderson, 2015-06-04 22:15:52]

lgtm with a few comments below

https://chromiumcodereview.appspot.com/1153633006/diff/100001/ash/metrics/use...
File ash/metrics/user_metrics_recorder.h (right):

https://chromiumcodereview.appspot.com/1153633006/diff/100001/ash/metrics/use...
ash/metrics/user_metrics_recorder.h:192: // The metric recorder for click and
tap input events.
nit: specify in this comment that it's specifically for activating windows by
click/tap.

https://chromiumcodereview.appspot.com/1153633006/diff/100001/tools/metrics/a...
File tools/metrics/actions/actions.xml (right):

https://chromiumcodereview.appspot.com/1153633006/diff/100001/tools/metrics/a...
tools/metrics/actions/actions.xml:2508: Recorded when the user activates an
existing task by clicking or tapping on
Specify this is only recorded for tasks which are windows.

https://chromiumcodereview.appspot.com/1153633006/diff/100001/tools/metrics/a...
tools/metrics/actions/actions.xml:2509: it in the 2-dimensional screen space.
On 2015/06/04 18:16:04, Alexei Svitkine wrote:
> I find this action description pretty confusing. I think this needs some
context
> - i.e. that this action is ChromeOS specific and related to the task switcher.
> (Also not sure why you mention "2-dimensional screen space" ... as opposed to
> what?)

Would it be clearer if "in the 2-dimensional screen space" were omitted enirely?
Whatever change is made to the description here should be applied to the other
documentation in this CL.

[bruthig, 2015-06-05 17:28:21]

I've addressed the comments from patch set 6.

tdanderson@, can you please have another look from a feature correctness
perspective?
 - */*

asvitkine@, can you please have another look?
 - histograms.xml
 - actions.xml
 - ash/metrics/desktop_task_switch_metric_recorder.(h|cc)

sky@, review if you wish, but not required.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/task_switc...
File ash/metrics/task_switch_metrics_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/task_switc...
ash/metrics/task_switch_metrics_recorder.h:33: kDesktop,
On 2015/06/04 20:10:52, sky wrote:
> For the record this style is wrong for chrome code and should be converted.

I've added a TODO.  I will do this in a follow up CL.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
File ash/metrics/user_metrics_recorder.cc (right):

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
ash/metrics/user_metrics_recorder.cc:608: if
(!desktop_task_switch_metric_recorder_) {
On 2015/06/04 20:10:52, sky wrote:
> Please add comment as to why the conditional.

Done.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
ash/metrics/user_metrics_recorder.cc:615: if
(desktop_task_switch_metric_recorder_)
On 2015/06/04 20:10:52, sky wrote:
> Conditional is not needed.

Done.

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
File ash/metrics/user_metrics_recorder.h (right):

https://codereview.chromium.org/1153633006/diff/100001/ash/metrics/user_metri...
ash/metrics/user_metrics_recorder.h:192: // The metric recorder for click and
tap input events.
On 2015/06/04 22:15:51, tdanderson wrote:
> nit: specify in this comment that it's specifically for activating windows by
> click/tap.

Done.

https://codereview.chromium.org/1153633006/diff/100001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1153633006/diff/100001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:2508: Recorded when the user activates an
existing task by clicking or tapping on
On 2015/06/04 22:15:51, tdanderson wrote:
> Specify this is only recorded for tasks which are windows.

Done.

https://codereview.chromium.org/1153633006/diff/100001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:2509: it in the 2-dimensional screen space.
Attempted to make it better.

[Alexei Svitkine (slow), 2015-06-05 17:39:24]

lgtm

https://codereview.chromium.org/1153633006/diff/120001/ash/metrics/desktop_ta...
File ash/metrics/desktop_task_switch_metric_recorder_unittest.cc (right):

https://codereview.chromium.org/1153633006/diff/120001/ash/metrics/desktop_ta...
ash/metrics/desktop_task_switch_metric_recorder_unittest.cc:77:
metrics_recorder_.reset(new DesktopTaskSwitchMetricRecorder());
Nit: Remove ()'s since you don't use them on the line below. Or vice versa.

[tdanderson, 2015-06-05 17:50:19]

Patch Set 7 lgtm

[bruthig, 2015-06-05 17:52:01]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-06-05 17:52:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
asvitkine@chromium.org, tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1153633006/#ps160001
(title: "Fixed the branch that the CL was based on.")

[commit-bot: I haz the power, 2015-06-05 17:53:39]

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

[commit-bot: I haz the power, 2015-06-05 18:57:12]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-05 19:06:27]

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

[Jeffrey Yasskin, 2015-06-05 22:53:50]

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

[Jeffrey Yasskin, 2015-06-05 22:53:51]

https://codereview.chromium.org/1153633006/diff/160001/ash/metrics/desktop_ta...
File ash/metrics/desktop_task_switch_metric_recorder_unittest.cc (right):

https://codereview.chromium.org/1153633006/diff/160001/ash/metrics/desktop_ta...
ash/metrics/desktop_task_switch_metric_recorder_unittest.cc:104: new
aura::Window(new aura::test::TestWindowDelegate));
aura::Window doesn't appear to take ownership of its delegate, so this is
leaking the TestWindowDelegate:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v....

[Jeffrey Yasskin, 2015-06-05 23:17:50]

On 2015/06/05 22:53:51, Jeffrey Yasskin wrote:
>
https://codereview.chromium.org/1153633006/diff/160001/ash/metrics/desktop_ta...
> File ash/metrics/desktop_task_switch_metric_recorder_unittest.cc (right):
> 
>
https://codereview.chromium.org/1153633006/diff/160001/ash/metrics/desktop_ta...
> ash/metrics/desktop_task_switch_metric_recorder_unittest.cc:104: new
> aura::Window(new aura::test::TestWindowDelegate));
> aura::Window doesn't appear to take ownership of its delegate, so this is
> leaking the TestWindowDelegate:
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28v....

Reverting in https://codereview.chromium.org/1165003010.