add stylus metrics

ash/wm/stylus_metrics_recorder.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/wm/stylus_metrics_recorder.h"
+
+#include "ash/common/wm/maximize_mode/maximize_mode_controller.h"
+#include "ash/common/wm_shell.h"
+#include "ash/shell.h"
+#include "base/metrics/histogram.h"
+#include "ui/events/event.h"
+#include "ui/events/event_constants.h"
+#include "ui/events/event_utils.h"
+
+namespace ash {
+
+enum DownEventFormFactor {
+  DOWN_EVENT_FORMFACTOR_CLAMSHELL = 0,
+  DOWN_EVENT_FORMFACTOR_TOUCHVIEW,
+  DOWN_EVENT_FORMFACTOR_COUNT
+};
+
+enum DownEventSource {
+  DOWN_EVENT_SOURCE_MOUSE = 0,
+  DOWN_EVENT_SOURCE_TOUCH,
+  DOWN_EVENT_SOURCE_STYLUS,
+  DOWN_EVENT_SOURCE_COUNT
+};

[Ilya Sherman, 2016/08/23 20:50:29]

For each of these enums, please add a brief comment documenting that the enum is
used to back an UMA histogram, and therefore should be treated as append-only.

[xiaoyinh(OOO Sep 11-29), 2016/08/23 22:43:01]

Done.

+
+StylusMetricsRecorder::StylusMetricsRecorder() {}
+
+StylusMetricsRecorder::~StylusMetricsRecorder() {}
+
+void StylusMetricsRecorder::OnMouseEvent(ui::MouseEvent* event) {
+  if (event->type() != ui::ET_MOUSE_PRESSED)
+    return;
+  RecordUMA(event->pointer_details().pointer_type);
+}
+
+void StylusMetricsRecorder::OnTouchEvent(ui::TouchEvent* event) {
+  if (event->type() != ui::ET_TOUCH_PRESSED)
+    return;
+  RecordUMA(event->pointer_details().pointer_type);
+}
+
+void StylusMetricsRecorder::RecordUMA(ui::EventPointerType type) {
+  if (WmShell::Get()
+          ->maximize_mode_controller()
+          ->IsMaximizeModeWindowManagerEnabled()) {
+    UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor",
+                              DOWN_EVENT_FORMFACTOR_TOUCHVIEW,
+                              DOWN_EVENT_FORMFACTOR_COUNT);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor",
+                              DOWN_EVENT_FORMFACTOR_CLAMSHELL,
+                              DOWN_EVENT_FORMFACTOR_COUNT);
+  }

[Ilya Sherman, 2016/08/23 20:50:29]

I'd recommend writing this to reduce repetition, like so:

DownEventFormFactor form_factor;
if (...) {
  form_factor = DOWN_EVENT_FORMFACTOR_TOUCHVIEW;
} else {
  form_factor = DOWN_EVENT_FORMFACTOR_CLAMSHELL;
}
UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor",
                              form_factor,
                              DOWN_EVENT_FORMFACTOR_COUNT);

Ditto for the enum below.

[oshima, 2016/08/23 21:40:57]

IIRC, it has to be that way because this macro creates a static object?

[xiaoyinh(OOO Sep 11-29), 2016/08/23 22:43:01]

Done.

+
+  switch (type) {
+    case ui::EventPointerType::POINTER_TYPE_MOUSE:
+      UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerInput",
+                                DOWN_EVENT_SOURCE_MOUSE,
+                                DOWN_EVENT_SOURCE_COUNT);
+      break;
+    case ui::EventPointerType::POINTER_TYPE_PEN:
+      UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerInput",
+                                DOWN_EVENT_SOURCE_STYLUS,
+                                DOWN_EVENT_SOURCE_COUNT);
+      break;
+    case ui::EventPointerType::POINTER_TYPE_TOUCH:
+      UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerInput",
+                                DOWN_EVENT_SOURCE_TOUCH,
+                                DOWN_EVENT_SOURCE_COUNT);
+      break;
+    default:

[Ilya Sherman, 2016/08/23 20:50:29]

Please handle all possible cases explicitly, so that the compiler will warn if
there are ever additional point types added in the future.  I'd recommend having
an UNKNOWN bucket in the histogram, to match the C++ enum.  And, as in the C++
enum, I'd make that bucket be the first one.  That way, if additional types are
ever added, it won't end up trapped in the middle.

[xiaoyinh(OOO Sep 11-29), 2016/08/23 22:43:01]

Done.

+      break;
+  }
+}
+
+}  // namespace ash