Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
895903003:
Adding UMA logging to touch text selection (Closed)
|
Created:
5 years, 10 months ago by mfomitchev Modified:
5 years, 10 months ago CC:
chromium-reviews, tfarina, Rick Byers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding UMA logging to touch text selection
For the Aura path we log the duration of the selection sequence -
from the the moment the text selection handles are brought up, and until
their dismissal, and also whether the selection was "successful" or not,
i.e. whether it ended with execution of one of the a command from the
quick menu.
For the Android path we only log the duration.
BUG=NONE
Committed: https://crrev.com/7e58cd2ca21f020e349893fbd310ade83cf27f47
Cr-Commit-Position: refs/heads/master@{#315894}
Patch Set 1 #Patch Set 2 : Histograms.xml #
Total comments: 2
Patch Set 3 : Using CPU time instead of wallclock time. #
Total comments: 6
Patch Set 4 : Addressing feedback. #
Total comments: 2
Patch Set 5 : Renaming the metric logged for Clank to Event.TouchDragSelectionDuration. #Patch Set 6 : Formatting #
Total comments: 5
Patch Set 7 : Min duration: 1ms -> 1000ms #
Total comments: 5
Patch Set 8 : Updating histogram names, updating enum type. #Patch Set 9 : Changed duration's min to 500ms instead of 1000ms #
Messages
Total messages: 33 (7 generated)
mfomitchev@chromium.org changed reviewers: + jdduke@chromium.org, mohsen@chromium.org
jdduke@chromium.org: Please review changes in ui/touch_selection mohsen@chromium.org: Please review changes in ui/views/touchui
https://codereview.chromium.org/895903003/diff/20001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/20001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:455: base::TimeDelta duration = base::Time::Now() - selection_start_time_; Shouldn't we be using TimeTicks (CPU time), not Time (wallclock time)?
https://codereview.chromium.org/895903003/diff/20001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/20001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:455: base::TimeDelta duration = base::Time::Now() - selection_start_time_; Good point. Fixed.
@jdduke, @mohsen: *poke*
https://codereview.chromium.org/895903003/diff/40001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/40001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:454: void TouchSelectionController::LogSelectionEnd() { Hmm, on second thought, if this will need to change for when we unify, I'm fine with leaving it out. I guess I'm also curious what the duration of the selection means, and how actionable it is... What does the duration of the selection tell us? https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:432: UMA_HISTOGRAM_BOOLEAN("Event.TouchSelectionEndedWithAction", false); So the only time there's no action is when this is destroyed and no command has been executed? Is the class lifetime tied to a single selection instance? https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:606: UMA_HISTOGRAM_CUSTOM_TIMES("Event.TouchSelectionDuration", Will this be changing in the near future when we unify?
https://codereview.chromium.org/895903003/diff/40001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/40001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:454: void TouchSelectionController::LogSelectionEnd() { My hope is that getting this into M42 will help us understand if the new selection granularity strategy (which I plan to get into M43) improves the UX. Generally we want the duration to go down and the success rate to go up. Now, this is more tricky when duration is the only thing we are logging, because the avg duration may decrease b/c of accidental selections. Still, I think having some information here would be useful. E.g. if we implement nothing to affect the number of accidental selections, while implementing a new selection granularity strategy, and the duration goes down - we can hypothesize that with a high degree of likelihood that is because the new selection strategy is working well. And vice versa. https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:432: UMA_HISTOGRAM_BOOLEAN("Event.TouchSelectionEndedWithAction", false); Yes and yes. https://codereview.chromium.org/895903003/diff/40001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:606: UMA_HISTOGRAM_CUSTOM_TIMES("Event.TouchSelectionDuration", Yes. The logic for command execution in the unified design is in TouchSelectionControllerAura. And then when Android starts using the quick menu, that code will likely move to TouchSelectionController. So we'll have to write some new code to do the logging, but I'd like to keep the names of the UMA events the same.
ui/views/touchui LGTM
https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:465: if (selection_handle_dragged_) { I was thinking we'd only log the cumulative duration of the act of dragging? This now has different semantics than Aura's "Event.TouchSelectionDuration", so I'd rather just not make any changes to TouchSelectionController for now.
https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... ui/touch_selection/touch_selection_controller.cc:465: if (selection_handle_dragged_) { I don't like logging individual drags b/c that can make the UX look better than it actually is if the user needs to performs a sequence of drags instead of one. Semantics for Aura vs Android were already different - b/c Aura only logs time for 'successful' selections. If we don't make any changes, we won't have any data on how the selection granularity strategy change affects the UX on Android. There are practical reasons we'd like to have that information.. Perhaps we can call the Android metric something else instead if different semantics with Aura is a serious concern?
On 2015/02/09 19:39:26, mfomitchev wrote: > https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... > File ui/touch_selection/touch_selection_controller.cc (right): > > https://codereview.chromium.org/895903003/diff/60001/ui/touch_selection/touch... > ui/touch_selection/touch_selection_controller.cc:465: if > (selection_handle_dragged_) { > I don't like logging individual drags b/c that can make the UX look better than > it actually is if the user needs to performs a sequence of drags instead of one. > Semantics for Aura vs Android were already different - b/c Aura only logs time > for 'successful' selections. > If we don't make any changes, we won't have any data on how the selection > granularity strategy change affects the UX on Android. There are practical > reasons we'd like to have that information.. Perhaps we can call the Android > metric something else instead if different semantics with Aura is a serious > concern? Yeah, if they have different semantics lets call it something different, and ideally we'd track an identical metric on Aura.
New patchsets have been uploaded after l-g-t-m from mohsen@chromium.org
> Yeah, if they have different semantics lets call it something different, and > ideally we'd track an identical metric on Aura. Okay, Clank now logs Event.TouchDragSelectionDuration instead of Event.TouchSelectionDuration. I'd rather not log TouchDragSelectionDuration on Aura, since it is temporary on Clank, and I don't think we need it on Aura now. Also it feels odd to log the same value twice in some cases (under TouchDragSelectionDuration and under TouchSelectionDuration).
lgtm with a couple questions. Just know that dev/canary channels are so sparse that it can take a while to get actionable data, so I'm not 100% sold on the value if we're planning on unifying selection behavior soon. https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:65: TouchSelectionController::~TouchSelectionController() { I'm not sure we ought to count this case, as this will occur if the tab is closed, a cross-origin navigation occurs or the page crashes. I dunno. https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:468: 50); Is this enough buckets to get the granularity we want?
+rbyers@ as FYI, I can be a UMA cynic so I'm not always the best reviewer for these changes :)
New patchsets have been uploaded after l-g-t-m from jdduke@chromium.org
> Just know that dev/canary channels are so sparse > that it can take a while to get actionable data, so I'm not 100% sold on the > value if we're planning on unifying selection behavior soon. Sure, but at least we will have a data point before the granularity strategy changes. If those changes will go into M43 and we don't have UMA data for M42, we won't be able to use UMA to judge their effectiveness. https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:65: TouchSelectionController::~TouchSelectionController() { Sure. Got rid of this. https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:468: 50); That's a good point. The time scale is logarithmic by default, so I had a ton of buckets from 1ms to 100ms, which was a total waste. I changed the min to be 1000ms, which makes things a lot better: at 1s the granularity is 90ms, at 3s it is 300ms, at 10s it is 900ms, and the last bucket is ~5s. I think that's pretty reasonable.
mfomitchev@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in histograms.xml
https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... File ui/touch_selection/touch_selection_controller.cc (right): https://codereview.chromium.org/895903003/diff/100001/ui/touch_selection/touc... ui/touch_selection/touch_selection_controller.cc:468: 50); On 2015/02/10 19:05:23, mfomitchev wrote: > That's a good point. The time scale is logarithmic by default, so I had a ton of > buckets from 1ms to 100ms, which was a total waste. I changed the min to be > 1000ms, which makes things a lot better: at 1s the granularity is 90ms, at 3s it > is 300ms, at 10s it is 900ms, and the last bucket is ~5s. I think that's pretty > reasonable. Ah, I didn't realize it was logarithmic, thanks.
https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:7325: +<histogram name="Event.TouchSelectionEndedWithAction" enum="BooleanEnabled"> This doesn't sound like BooleanEnabled. Is there a more appropriate enum you can use? If not, add a custom one for this metric. https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... ui/views/touchui/touch_selection_controller_impl.cc:608: base::TimeDelta::FromMilliseconds(1000), Could you explain what kind of info you're hoping to get from these time metrics? Starting at 1s sounds like you will be missing a lot of details about the lower end - but maybe that's what you want? Or, perhaps it might be better to log the data in different units that milliseconds, like e.g seconds or 100ms?
https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... ui/views/touchui/touch_selection_controller_impl.cc:608: base::TimeDelta::FromMilliseconds(1000), On 2015/02/10 22:23:04, Alexei Svitkine wrote: > Could you explain what kind of info you're hoping to get from these time > metrics? > > Starting at 1s sounds like you will be missing a lot of details about the lower > end - but maybe that's what you want? > > Or, perhaps it might be better to log the data in different units that > milliseconds, like e.g seconds or 100ms? Yeah, I'm thinking we should at least go down to 500ms.
https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:7325: +<histogram name="Event.TouchSelectionEndedWithAction" enum="BooleanEnabled"> On 2015/02/10 22:23:04, Alexei Svitkine wrote: > This doesn't sound like BooleanEnabled. Is there a more appropriate enum you can > use? If not, add a custom one for this metric. Done. I also changed the names a bit. https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... ui/views/touchui/touch_selection_controller_impl.cc:608: base::TimeDelta::FromMilliseconds(1000), On 2015/02/10 22:29:00, jdduke wrote: > On 2015/02/10 22:23:04, Alexei Svitkine wrote: > > Could you explain what kind of info you're hoping to get from these time > > metrics? > > > > Starting at 1s sounds like you will be missing a lot of details about the > lower > > end - but maybe that's what you want? > > > > Or, perhaps it might be better to log the data in different units that > > milliseconds, like e.g seconds or 100ms? > > Yeah, I'm thinking we should at least go down to 500ms. Perhaps 1s is a bit low. I actually tried before I chose 1s. The problem is that with the logarithmic scale it takes something like 7 buckets for the value to double, and we don't really need 7 buckets between 500ms and 1s, and we also lose granularity at higher values unless we increase the total number of buckets. The way I thought about it - anything at 1s or less is pretty awesome UX for touch text selection. I don't know if we'd do anything differently if the distribution in 500ms - 1s range changes. That said, I don't mind changing the min to 500 and increasing the number of buckets to ~60 so that we can maintain a decent granularity in the 1s-~10s range, which is the range I am mostly interested in. @asvitkine I guess I could log in 100ms units instead of in milliseconds - is this a recommended practice in the cases where you don't need granularity higher than 100ms? (Seconds would be too coarse here.) Is that done elsewhere?
On 2015/02/10 23:22:54, mfomitchev wrote: > https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/895903003/diff/120001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:7325: +<histogram > name="Event.TouchSelectionEndedWithAction" enum="BooleanEnabled"> > On 2015/02/10 22:23:04, Alexei Svitkine wrote: > > This doesn't sound like BooleanEnabled. Is there a more appropriate enum you > can > > use? If not, add a custom one for this metric. > > Done. I also changed the names a bit. > > https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... > File ui/views/touchui/touch_selection_controller_impl.cc (right): > > https://codereview.chromium.org/895903003/diff/120001/ui/views/touchui/touch_... > ui/views/touchui/touch_selection_controller_impl.cc:608: > base::TimeDelta::FromMilliseconds(1000), > On 2015/02/10 22:29:00, jdduke wrote: > > On 2015/02/10 22:23:04, Alexei Svitkine wrote: > > > Could you explain what kind of info you're hoping to get from these time > > > metrics? > > > > > > Starting at 1s sounds like you will be missing a lot of details about the > > lower > > > end - but maybe that's what you want? > > > > > > Or, perhaps it might be better to log the data in different units that > > > milliseconds, like e.g seconds or 100ms? > > > > Yeah, I'm thinking we should at least go down to 500ms. > > Perhaps 1s is a bit low. I actually tried before I chose 1s. The problem is > that with the logarithmic scale it takes something like 7 buckets for the value > to double, and we don't really need 7 buckets between 500ms and 1s, and we also > lose granularity at higher values unless we increase the total number of > buckets. The way I thought about it - anything at 1s or less is pretty awesome > UX for touch text selection. I don't know if we'd do anything differently if the > distribution in 500ms - 1s range changes. That said, I don't mind changing the > min to 500 and increasing the number of buckets to ~60 so that we can maintain a > decent granularity in the 1s-~10s range, which is the range I am mostly > interested in. > @asvitkine I guess I could log in 100ms units instead of in milliseconds - is > this a recommended practice in the cases where you don't need granularity higher > than 100ms? (Seconds would be too coarse here.) Is that done elsewhere? "I actually tried before I chose 1s" -> "I actually tried 500ms before I chose 1s"
asvitkine: Can you please take another look?
lgtm
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895903003/160001
The CQ bit was unchecked by mfomitchev@chromium.org
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/895903003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7e58cd2ca21f020e349893fbd310ade83cf27f47 Cr-Commit-Position: refs/heads/master@{#315894} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
