Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1135283003:
Gesture Editing UMA stats (Closed)
DescriptionGesture Editing UMA stats
This is the Chromium side changes to record UMA stats for Gesture Editing on the Virtual Keyboard. A follow up CL will be written on Google3 to populate these stats.
BUG=487323
Committed: https://crrev.com/ab29c168c5d64e8e2b4dd220a31abcab3174356d
Cr-Commit-Position: refs/heads/master@{#329798}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address reviewer feedback. #
Total comments: 6
Patch Set 3 : Address reviewer feedback. #Messages
Total messages: 15 (4 generated)
rsadam@chromium.org changed reviewers: + stevet@chromium.org
Hey Steve, This CL adds the UMA stats we discussed. Please take a first look before I pass it off to the histograms team for OWNERS.
back to you https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12890: + tracker. This is recorded each time the gesture deletion tracker is hidden. It's actually called the deletion "track". https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12915: + deletion tracker. This is recorded each time the gesture deletion tracker is ditto tracker->track https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12920: +<histogram name="InputMethod.VirtualKeyboard.WordsSelectedPerSwipe"> I don't think we should refer to it as "selecting" words because we're not selecting words anymore. We only move the cursors. So maybe "MovesPerSwipe" might be a better name. Need to update the description to describe a "move" as moved past one word. ditto above with "deselect". BackwardsMovesPerSwipe instead? https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12924: + selection tracker. This is recorded each time the gesture selection tracker ditto tracker->track
Thanks for the feedback, back to you Steve! https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12890: + tracker. This is recorded each time the gesture deletion tracker is hidden. On 2015/05/12 19:11:15, SteveT wrote: > It's actually called the deletion "track". Done. https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12915: + deletion tracker. This is recorded each time the gesture deletion tracker is On 2015/05/12 19:11:15, SteveT wrote: > ditto > > tracker->track Done. https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12920: +<histogram name="InputMethod.VirtualKeyboard.WordsSelectedPerSwipe"> On 2015/05/12 19:11:15, SteveT wrote: > I don't think we should refer to it as "selecting" words because we're not > selecting words anymore. We only move the cursors. > > So maybe "MovesPerSwipe" might be a better name. Need to update the description > to describe a "move" as moved past one word. > > ditto above with "deselect". BackwardsMovesPerSwipe instead? Done. https://codereview.chromium.org/1135283003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:12924: + selection tracker. This is recorded each time the gesture selection tracker On 2015/05/12 19:11:15, SteveT wrote: > ditto > > tracker->track Done.
lgtm LGTM
rsadam@chromium.org changed reviewers: + mpearson@chromium.org
Thanks Steve! +mpearson@chromium.org for OWNERS
I reviewed the first histograms description. Many of the comments apply to the rest, so I'll wait for revisions of all them before reviewing again. --mark https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12833: +<histogram name="InputMethod.VirtualKeyboard.BackwardsMovesPerSwipe"> units=? https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12836: + Counts the number of times the cursor was moved to the previous word by This description is confusing. I can't tell if this is a histogram that gets a value of 1 submitted to it when the cursor moves to a previous word (and maybe this action implicitly causes the selection track to be hidden?) or this histogram gets a value of x submitted to it when the selection track is hidden, and that x is the number of times the cursor moved to previous words in between the selection track opening and it getting hidden. Maybe the units field would help. Regardless, I think something needs to be rewritten a bit here. https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12838: + selection track is hidden. Is this only recorded on Android? All mobile platforms? Something else?
https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12833: +<histogram name="InputMethod.VirtualKeyboard.BackwardsMovesPerSwipe"> On 2015/05/12 19:34:02, Mark P wrote: > units=? Done. https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12836: + Counts the number of times the cursor was moved to the previous word by On 2015/05/12 19:34:02, Mark P wrote: > This description is confusing. I can't tell if this is a histogram that gets a > value of 1 submitted to it when the cursor moves to a previous word (and maybe > this action implicitly causes the selection track to be hidden?) or this > histogram gets a value of x submitted to it when the selection track is hidden, > and that x is the number of times the cursor moved to previous words in between > the selection track opening and it getting hidden. Maybe the units field would > help. Regardless, I think something needs to be rewritten a bit here. The answer is the latter. I rewrote the description, hopefully it's a lot clearer. https://codereview.chromium.org/1135283003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:12838: + selection track is hidden. On 2015/05/12 19:34:02, Mark P wrote: > Is this only recorded on Android? All mobile platforms? Something else? ChromeOS only, added to the description.
histograms.xml lgtm Thanks for the much better descriptions. --mark
The CQ bit was checked by rsadam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevet@chromium.org Link to the patchset: https://codereview.chromium.org/1135283003/#ps40001 (title: "Address reviewer feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135283003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ab29c168c5d64e8e2b4dd220a31abcab3174356d Cr-Commit-Position: refs/heads/master@{#329798} |
