ash: Add some more touch UMA.

ash: Add some more touch UMA.

BUG=138846
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151117

[sadrul, 2012-08-10 15:54:42]

rbyers@ Please review

isherman@ Please review that the usage of STATIC_HISTOGRAM_POINTER_BLOCK is
correct.

http://codereview.chromium.org/10826247/diff/3/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/3/ash/touch/touch_uma.cc#newcode309
ash/touch/touch_uma.cc:309: 1, kMaxTouchPoints, kMaxTouchPoints);
This will effectively be linear, right?

[Rick Byers, 2012-08-10 18:07:09]

lgtm, thanks!

http://codereview.chromium.org/10826247/diff/3/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/3/ash/touch/touch_uma.cc#newcode275
ash/touch/touch_uma.cc:275: STATIC_HISTOGRAM_POINTER_BLOCK("Ash.TouchPositionX",
Too bad there's not a linear version of UMA_HISTOGRAM_COUNTS - I wasn't
expecting you'd need to do the division into buckets yourself, etc..  Oh well.

[sadrul, 2012-08-10 18:57:33]

+sky

[sky, 2012-08-10 19:18:33]

LGTM with the following change.

http://codereview.chromium.org/10826247/diff/8001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/8001/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:278: const int kBucketSizeX = bounds.width() /
kBucketCount;
Don't use kStyle for computed values like this.

[sadrul, 2012-08-10 19:20:24]

http://codereview.chromium.org/10826247/diff/8001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/8001/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:278: const int kBucketSizeX = bounds.width() /
kBucketCount;
On 2012/08/10 19:18:33, sky wrote:
> Don't use kStyle for computed values like this.

Done.

[Ilya Sherman, 2012-08-13 22:22:41]

Thanks for including me on this review -- histograms can be a little subtle...

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:276: const int kBucketCount = 100;
Optional nit: Since you have another histogram further down in this method that
uses a different bucket count, perhaps name this more clearly to describe what
histogram's buckets this is counting?

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:276: const int kBucketCount = 100;
100 buckets is rather a lot -- are you sure you need that level of granularity? 
If so, that's ok; 100 buckets isn't totally unreasonable.  But, if not, I would
recommend something closer to 20.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:309:
base::LinearHistogram::FactoryGet("Ash.TouchPositionX", 1, kBucketCount,
Optional nit: Since logging a sample of "0" seems reachable, I think it would be
slightly clearer to declare the lower bound to be 0 rather than 1.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:314: kBucketCount + 1,
base::Histogram::kUmaTargetedHistogramFlag));
I don't think that you should be using STATIC_HISTOGRAM_POINTER_BLOCK here --
it's logically meant to be private to the histogram.h file, but macros can't
really be declared as private.  Why doesn't an UMA_HISTOGRAM_CUSTOM_COUNTS work
here?

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:340: kMaxTouchPoints),
nit: No need for the std::min; all samples above the maximum will just fall into
an overflow bucket.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:341: 1, kMaxTouchPoints, kMaxTouchPoints);
nit: The bucket count should be kMaxTouchPoints + 1 -- you want to have buckets
from 0 through kMaxTouchPoints, with the last one serving as the overflow
bucket.

[sadrul, 2012-08-14 05:21:32]

Addressed the comments in http://codereview.chromium.org/10831303/

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:276: const int kBucketCount = 100;
On 2012/08/13 22:22:42, Ilya Sherman wrote:
> Optional nit: Since you have another histogram further down in this method
that
> uses a different bucket count, perhaps name this more clearly to describe what
> histogram's buckets this is counting?

Done.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:309:
base::LinearHistogram::FactoryGet("Ash.TouchPositionX", 1, kBucketCount,
On 2012/08/13 22:22:42, Ilya Sherman wrote:
> Optional nit: Since logging a sample of "0" seems reachable, I think it would
be
> slightly clearer to declare the lower bound to be 0 rather than 1.

Done.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:340: kMaxTouchPoints),
On 2012/08/13 22:22:42, Ilya Sherman wrote:
> nit: No need for the std::min; all samples above the maximum will just fall
into
> an overflow bucket.

Done.

http://codereview.chromium.org/10826247/diff/4003/ash/touch/touch_uma.cc#newc...
ash/touch/touch_uma.cc:341: 1, kMaxTouchPoints, kMaxTouchPoints);
On 2012/08/13 22:22:42, Ilya Sherman wrote:
> nit: The bucket count should be kMaxTouchPoints + 1 -- you want to have
buckets
> from 0 through kMaxTouchPoints, with the last one serving as the overflow
> bucket.

Done.