Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831303:
ash: Add UMA for taps on webpage, bezel and tabstrip. (Closed)
|
Created:
8 years, 4 months ago by sadrul Modified:
8 years, 4 months ago CC:
chromium-reviews, ben+watch_chromium.org, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionash: Add UMA for taps on webpage, bezel and tabstrip.
Also add some review nits (from crrev.com/151117).
BUG=138846
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151551
Patch Set 1 #
Total comments: 6
Patch Set 2 : bezel-downs #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 4
Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode105 ash/touch/touch_uma.cc:105: return GESTURE_BEZEL_TAP; rbyers@: Do you think counting long-press here would be useful?
http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode325 ash/touch/touch_uma.cc:325: base::Histogram::kUmaTargetedHistogramFlag)); As I mentioned on the other CL, 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/10831303/diff/1/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode325 ash/touch/touch_uma.cc:325: base::Histogram::kUmaTargetedHistogramFlag)); On 2012/08/14 05:27:38, Ilya Sherman wrote: > As I mentioned on the other CL, 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? We expect that a linear histogram would be more useful here, and there doesn't appear to be a good way of using linear-histograms using the current UMA_HISTOGRAM_ macros. Perhaps I should add UMA_HISTOGRAM_CUSTOM_COUNT_LINEAR?
lgtm http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode105 ash/touch/touch_uma.cc:105: return GESTURE_BEZEL_TAP; On 2012/08/14 05:21:05, sadrul wrote: > rbyers@: Do you think counting long-press here would be useful? Couldn't hurt, but I'm not sure it adds that much value. Your call. http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode325 ash/touch/touch_uma.cc:325: base::Histogram::kUmaTargetedHistogramFlag)); On 2012/08/14 05:27:38, Ilya Sherman wrote: > As I mentioned on the other CL, 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? CUSTOM_COUNTS uses an exponential distribution, right? That's not appropriate for pixel co-ordinates - we want a linear distribution. Perhaps we need a new macro.
+cc jar http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode325 ash/touch/touch_uma.cc:325: base::Histogram::kUmaTargetedHistogramFlag)); On 2012/08/14 05:30:41, Rick Byers wrote: > On 2012/08/14 05:27:38, Ilya Sherman wrote: > > As I mentioned on the other CL, 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? > > CUSTOM_COUNTS uses an exponential distribution, right? That's not appropriate > for pixel co-ordinates - we want a linear distribution. Perhaps we need a new > macro. If you create a CUSTOM_COUNTS histogram with 101 buckets between 0 and 99 (plus an overflow bucket, and with the 0 bucket implicitly serving as an underflow bucket), you should end up with a linear histogram. I'm not entirely sure why we have a separate LinearHistogram class; perhaps Jim can comment on that...
On 2012/08/14 05:45:00, Ilya Sherman wrote: > +cc jar > > http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc > File ash/touch/touch_uma.cc (right): > > http://codereview.chromium.org/10831303/diff/1/ash/touch/touch_uma.cc#newcode325 > ash/touch/touch_uma.cc:325: base::Histogram::kUmaTargetedHistogramFlag)); > On 2012/08/14 05:30:41, Rick Byers wrote: > > On 2012/08/14 05:27:38, Ilya Sherman wrote: > > > As I mentioned on the other CL, 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? > > > > CUSTOM_COUNTS uses an exponential distribution, right? That's not appropriate > > for pixel co-ordinates - we want a linear distribution. Perhaps we need a new > > macro. > > If you create a CUSTOM_COUNTS histogram with 101 buckets between 0 and 99 (plus > an overflow bucket, and with the 0 bucket implicitly serving as an underflow > bucket), you should end up with a linear histogram. Indeed. You are right. I have made this change. I have also fixed an issue where the positions weren't corrected for the device-scale factor.
LGTM, thanks http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... ash/touch/touch_uma.cc:311: position = position.Scale(1. / target->layer()->device_scale_factor()); nit: I'm not 100% sure of this, but I think Chromium code generally prefers "1.0" to "1." http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... ash/touch/touch_uma.cc:348: 1, kMaxTouchPoints, kMaxTouchPoints + 1); Note that this change will probably cause one of the bucket ranges to be different between this version and the current uploads (from Canary builds?). That's probably ok since the histogram hasn't been around very long, and 10 touch points is pretty unlikely itself. But, if you want to make sure that the data from older versions doesn't cloud your analysis, you'll need to be careful to only select appropriate versions on the dashboard, or just rename this histogram so that there's no mixing of data.
+sky for OWNERS http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc File ash/touch/touch_uma.cc (right): http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... ash/touch/touch_uma.cc:311: position = position.Scale(1. / target->layer()->device_scale_factor()); On 2012/08/14 18:54:01, Ilya Sherman wrote: > nit: I'm not 100% sure of this, but I think Chromium code generally prefers > "1.0" to "1." It looks like we use "1." in other places in the code. http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... ash/touch/touch_uma.cc:348: 1, kMaxTouchPoints, kMaxTouchPoints + 1); On 2012/08/14 18:54:01, Ilya Sherman wrote: > Note that this change will probably cause one of the bucket ranges to be > different between this version and the current uploads (from Canary builds?). > That's probably ok since the histogram hasn't been around very long, and 10 > touch points is pretty unlikely itself. But, if you want to make sure that the > data from older versions doesn't cloud your analysis, you'll need to be careful > to only select appropriate versions on the dashboard, or just rename this > histogram so that there's no mixing of data. Thanks for the heads-up. I think at this point, this is still new enough that this won't be an issue. rbyers@, would you agree? Or should we rename the histogram as isherman@ suggests?
On 2012/08/14 19:57:05, sadrul wrote: > +sky for OWNERS > > http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc > File ash/touch/touch_uma.cc (right): > > http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... > ash/touch/touch_uma.cc:311: position = position.Scale(1. / > target->layer()->device_scale_factor()); > On 2012/08/14 18:54:01, Ilya Sherman wrote: > > nit: I'm not 100% sure of this, but I think Chromium code generally prefers > > "1.0" to "1." > > It looks like we use "1." in other places in the code. > > http://codereview.chromium.org/10831303/diff/4003/ash/touch/touch_uma.cc#newc... > ash/touch/touch_uma.cc:348: 1, kMaxTouchPoints, kMaxTouchPoints + 1); > On 2012/08/14 18:54:01, Ilya Sherman wrote: > > Note that this change will probably cause one of the bucket ranges to be > > different between this version and the current uploads (from Canary builds?). > > That's probably ok since the histogram hasn't been around very long, and 10 > > touch points is pretty unlikely itself. But, if you want to make sure that > the > > data from older versions doesn't cloud your analysis, you'll need to be > careful > > to only select appropriate versions on the dashboard, or just rename this > > histogram so that there's no mixing of data. > > Thanks for the heads-up. I think at this point, this is still new enough that > this won't be an issue. rbyers@, would you agree? Or should we rename the > histogram as isherman@ suggests? Yes, that's fine - this is really new and it's just me analyzing the data, so I'll be sure to watch out for this. Thanks for pointing it out!
LGTM |
