Move gesture/touch UMA logs into gesture provider

Move gestureCreated, touchMaxDistance and touchDuration UMA logs into gesture provider, so that they can also record data from other platforms, such as Android, Windows, not only from ChromeOS.

BUG=352654
Committed: https://crrev.com/9e7d5b36d720e9b6f1c3818c62e7943477db5ebb
Cr-Commit-Position: refs/heads/master@{#292570}

[lanwei, 2014-08-26 15:57:21]

lanwei@chromium.org changed reviewers:
+ tdresser@chromium.org

[lanwei, 2014-08-26 20:55:59]

Please look at the last patch, thanks.

[tdresser, 2014-08-27 12:35:39]

https://codereview.chromium.org/501553004/diff/120001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (left):

https://codereview.chromium.org/501553004/diff/120001/ash/touch/touch_uma.cc#...
ash/touch/touch_uma.cc:73: : max_distance_from_start_squared_(0) {
You should be able to get rid of max_distance_from_start_squared_ completely.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:268: // Record and generate the
gesture UMA histograms for all platforms.
I think the method name is probably descriptive enough here.

Stating that this is for _all_ platforms isn't quite true (this isn't done for
mac for example). It's clear that it's done for all platforms which have a
GestureProvider instance.

It's also clear that we're recording data about gestures for uma histograms.

I'd just get rid of this comment.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:269: GestureTouchUMAHistogram
umaHistogram;
Why are you instantiating a new instance of GestureTouchUMAHistogram?

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:662: // Record and generate the
touch UMA histograms for all platforms.
Comment is unnecessary.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:106: // events
Reword comment (the fact that we're instantiating an object is obvious, only
state what isn't obvious.

Something like:
// Logs information on touch and gesture events.

(Also, add a period at the end.)

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:107: GestureTouchUMAHistogram
umaHistogram;
"Variable names are all lowercase, with underscores between words. Class member
variables have trailing underscores"

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.cc (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:41: EventType
eventType = TouchEventTypeFromMotionEvent(event);
Just use GetAction() from the MotionEvent directly.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:54:
"Event.TouchMaxDistance",
You'll need to modify histograms.xml to deprecate the old histogram names, and
add the new ones.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:67: EventType
GestureTouchUMAHistogram::TouchEventTypeFromMotionEvent(
This is unnecessary.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.cc.save (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc.save:1: // Copyright
2014 The Chromium Authors. All rights reserved.
This file looks like an accidental commit.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.h (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:57: // accordingly.
Obey the comment! (Move tap unconfirmed above the comment, and update
histograms.xml)

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:73: struct
GESTURE_DETECTION_EXPORT WindowTouchDetails {
I don't see any reason to put this information in a struct.

We should probably also have either:
is_single_finger_
or some things in scoped ptrs, not both.

[lanwei, 2014-08-27 23:32:00]

https://codereview.chromium.org/501553004/diff/120001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (left):

https://codereview.chromium.org/501553004/diff/120001/ash/touch/touch_uma.cc#...
ash/touch/touch_uma.cc:73: : max_distance_from_start_squared_(0) {
On 2014/08/27 12:35:38, tdresser wrote:
> You should be able to get rid of max_distance_from_start_squared_ completely.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.cc (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:268: // Record and generate the
gesture UMA histograms for all platforms.
On 2014/08/27 12:35:38, tdresser wrote:
> I think the method name is probably descriptive enough here.
> 
> Stating that this is for _all_ platforms isn't quite true (this isn't done for
> mac for example). It's clear that it's done for all platforms which have a
> GestureProvider instance.
> 
> It's also clear that we're recording data about gestures for uma histograms.
> 
> I'd just get rid of this comment.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:269: GestureTouchUMAHistogram
umaHistogram;
On 2014/08/27 12:35:38, tdresser wrote:
> Why are you instantiating a new instance of GestureTouchUMAHistogram?

The send function does not know uma_histogram_. As you suggested, I made the
RecordGestureEvent function static.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.cc:662: // Record and generate the
touch UMA histograms for all platforms.
On 2014/08/27 12:35:38, tdresser wrote:
> Comment is unnecessary.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_provider.h (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:106: // events
On 2014/08/27 12:35:39, tdresser wrote:
> Reword comment (the fact that we're instantiating an object is obvious, only
> state what isn't obvious.
> 
> Something like:
> // Logs information on touch and gesture events.
> 
> (Also, add a period at the end.)

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_provider.h:107: GestureTouchUMAHistogram
umaHistogram;
On 2014/08/27 12:35:38, tdresser wrote:
> "Variable names are all lowercase, with underscores between words. Class
member
> variables have trailing underscores"
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.cc (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:41: EventType
eventType = TouchEventTypeFromMotionEvent(event);
On 2014/08/27 12:35:39, tdresser wrote:
> Just use GetAction() from the MotionEvent directly.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:54:
"Event.TouchMaxDistance",
On 2014/08/27 12:35:39, tdresser wrote:
> You'll need to modify histograms.xml to deprecate the old histogram names, and
> add the new ones.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.cc:67: EventType
GestureTouchUMAHistogram::TouchEventTypeFromMotionEvent(
On 2014/08/27 12:35:39, tdresser wrote:
> This is unnecessary.

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.h (right):

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:57: // accordingly.
On 2014/08/27 12:35:39, tdresser wrote:
> Obey the comment! (Move tap unconfirmed above the comment, and update
> histograms.xml)

Done.

https://codereview.chromium.org/501553004/diff/120001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:73: struct
GESTURE_DETECTION_EXPORT WindowTouchDetails {
On 2014/08/27 12:35:39, tdresser wrote:
> I don't see any reason to put this information in a struct.
> 
> We should probably also have either:
> is_single_finger_
> or some things in scoped ptrs, not both.

Done.

[tdresser, 2014-08-28 12:47:50]

Nice!
LGTM with nits.

Note that you'll need an additional reviewer.

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:656: +    Deprecated 08/2014 in Issue
352654, and replaced by Event.TouchDuration2.
We can just name this Event.TouchDuration (without the 2), throughout the patch.

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6253: +    recognizer, a new version of
Ash.GestureCreated, as it also records events
It would be a bit clearer to say:
The gesture-events recognized and dispatched by the browser gesture
recognizer. This replaces Ash.GestureCreated, which did not record events on
Android and Windows. (and below)

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6625: +    also records events from
Android and Window.
Mention that this is only recorded for single finger gestures.

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.h (right):

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:74: // The first
finger's press time
Add a period at the end of the comment.

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:76: // The first
finger's press location
Add a period.

[lanwei, 2014-08-28 17:26:02]

lanwei@chromium.org changed reviewers:
+ isherman@chromium.org, jamescook@chromium.org

[lanwei, 2014-08-28 17:26:02]

jamescook@chromium.org: Please review changes in 

isherman@chromium.org: Please review changes in

[lanwei, 2014-08-28 18:12:15]

change the comments

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:656: +    Deprecated 08/2014 in Issue
352654, and replaced by Event.TouchDuration2.
On 2014/08/28 12:47:50, tdresser wrote:
> We can just name this Event.TouchDuration (without the 2), throughout the
patch.

Done.

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6253: +    recognizer, a new version of
Ash.GestureCreated, as it also records events
On 2014/08/28 12:47:50, tdresser wrote:
> It would be a bit clearer to say:
> The gesture-events recognized and dispatched by the browser gesture
> recognizer. This replaces Ash.GestureCreated, which did not record events on
> Android and Windows. (and below)

Done.

https://codereview.chromium.org/501553004/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6625: +    also records events from
Android and Window.
On 2014/08/28 12:47:50, tdresser wrote:
> Mention that this is only recorded for single finger gestures.

Done.

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
File ui/events/gesture_detection/gesture_touch_uma_histogram.h (right):

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:74: // The first
finger's press time
On 2014/08/28 12:47:50, tdresser wrote:
> Add a period at the end of the comment.

Done.

https://codereview.chromium.org/501553004/diff/140001/ui/events/gesture_detec...
ui/events/gesture_detection/gesture_touch_uma_histogram.h:76: // The first
finger's press location
On 2014/08/28 12:47:50, tdresser wrote:
> Add a period.

Done.

[James Cook, 2014-08-28 18:52:43]

ash/* LGTM  (I presume that's what you wanted me to look at)

[Ilya Sherman, 2014-08-29 00:44:20]

histograms lgtm

https://codereview.chromium.org/501553004/diff/160001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/501553004/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6634: +    The maximum euclidean
distance in dips which a touch point has travelled
nit: I'm not sure what "dips" means.  Is this an acronym or abbreviation of some
sort?  Please clarify.

[lanwei, 2014-08-29 01:16:23]

https://codereview.chromium.org/501553004/diff/160001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/501553004/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:6634: +    The maximum euclidean
distance in dips which a touch point has travelled
On 2014/08/29 00:44:20, Ilya Sherman wrote:
> nit: I'm not sure what "dips" means.  Is this an acronym or abbreviation of
some
> sort?  Please clarify.

A Device independent pixel (also: density-independent pixel) is a physical unit
of measurement based on a co-ordinate system held by a computer and represents
an abstraction of a pixel for use by an application that an underlying system
then converts to physical pixels.

[lanwei, 2014-08-29 01:23:04]

add description to the comment

[lanwei, 2014-08-29 01:23:11]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2014-08-29 01:25:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lanwei@chromium.org/501553004/180001

[commit-bot: I haz the power, 2014-08-29 02:39:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-29 02:44:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-29 02:44:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lanwei, 2014-08-29 03:53:31]

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

[lanwei, 2014-08-29 03:57:02]

Sadrul, could you please take a look at the ui/events/BUILD.gn file, thanks.

[sadrul, 2014-08-29 04:01:53]

lgtm

[lanwei, 2014-08-29 04:02:33]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2014-08-29 04:03:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lanwei@chromium.org/501553004/180001

[commit-bot: I haz the power, 2014-08-29 04:05:47]

Committed patchset #10 (id:180001) as fd1540d6b8049f10e62d219e7688e307d49e6fd0

[commit-bot: I haz the power, 2014-09-10 03:06:13]

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

[lanwei, 2014-09-11 00:50:21]

Patchset #11 (id:200001) has been deleted