Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2900913002:
add UMA metrics for voice interaction entries. (Closed)
|
Created:
3 years, 7 months ago by Muyuan Modified:
3 years, 7 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, sadrul, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, oshima+watch_chromium.org, asvitkine+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, kalyank, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionadd UMA metrics for voice interaction entries.
BUG=b/38201455
TEST=manually test on device
Review-Url: https://codereview.chromium.org/2900913002
Cr-Commit-Position: refs/heads/master@{#474415}
Committed: https://chromium.googlesource.com/chromium/src/+/2614b90ab83cd2fc4512d7b28dd7ba72cc28d197
Patch Set 1 #Patch Set 2 : add UMA metrics for voice interaction entries. #
Total comments: 2
Patch Set 3 : address review comment #Patch Set 4 : add UMA metrics for voice interaction entries. #
Total comments: 4
Patch Set 5 : change to log user action instead. #
Total comments: 12
Patch Set 6 : address review comments #
Messages
Total messages: 22 (7 generated)
muyuanli@chromium.org changed reviewers: + isherman@chromium.org, lhchavez@chromium.org, skuhne@chromium.org
lhchavez: Please review changes in arc_voice_interaction_framework_service.cc. skuhne: Please review changes in app_list_button.cc isherman: Please review changes in metrics.
Rather than adding four separate histograms, with one bucket each, please either (a) add a single histogram with four buckets, or (b) record these as user actions. Option (a) allows you to easily compare the frequencies of these four events among each other. Option (b) allows you to view these actions in user action sequences, i.e. lets you get a sense for what users were doing before or after these actions.
chrome/browser/chromeos/arc lgtm with a nit https://codereview.chromium.org/2900913002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:137: if (accelerator.IsCmdDown() && accelerator.key_code() == ui::VKEY_SPACE) nit: you cannot elide braces since the body is not a single line.
app_listbutton.cc lgtm
On 2017/05/23 15:20:07, Ilya Sherman wrote: > Rather than adding four separate histograms, with one bucket each, please either > (a) add a single histogram with four buckets, or (b) record these as user > actions. Option (a) allows you to easily compare the frequencies of these four > events among each other. Option (b) allows you to view these actions in user > action sequences, i.e. lets you get a sense for what users were doing before or > after these actions. Uploaded a new patchset.
https://codereview.chromium.org/2900913002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:137: if (accelerator.IsCmdDown() && accelerator.key_code() == ui::VKEY_SPACE) On 2017/05/23 15:24:44, Luis Héctor Chávez wrote: > nit: you cannot elide braces since the body is not a single line. Done.
muyuanli@chromium.org changed reviewers: + derat@chromium.org
derat@chromium.org: Please review changes in ash/metrics/
https://codereview.chromium.org/2900913002/diff/60001/ash/metrics/voice_inter... File ash/metrics/voice_interaction_metrics.h (right): https://codereview.chromium.org/2900913002/diff/60001/ash/metrics/voice_inter... ash/metrics/voice_interaction_metrics.h:10: enum class VoiceInteractionMetaLayerEntry { i think it would make more sense to record these as user actions than histograms. this is what we use for other accelerators. see e.g. Accel_Focus_Search, Accel_Search_LWin, etc. https://codereview.chromium.org/2900913002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:132: ash::VoiceInteractionMetaLayerEntry::META_SHIFT_A, does "meta" here correspond to the search key? if so, please consider using e.g. SEARCH_SHIFT_A instead. i recommend staying away from the word "meta", since i think we either use search or "command" to refer to this key in the code and in user-facing strings.
https://codereview.chromium.org/2900913002/diff/60001/ash/metrics/voice_inter... File ash/metrics/voice_interaction_metrics.h (right): https://codereview.chromium.org/2900913002/diff/60001/ash/metrics/voice_inter... ash/metrics/voice_interaction_metrics.h:10: enum class VoiceInteractionMetaLayerEntry { On 2017/05/23 22:32:18, Daniel Erat wrote: > i think it would make more sense to record these as user actions than > histograms. this is what we use for other accelerators. see e.g. > Accel_Focus_Search, Accel_Search_LWin, etc. Acknowledged. Uploaded a patch set to use user action https://codereview.chromium.org/2900913002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:132: ash::VoiceInteractionMetaLayerEntry::META_SHIFT_A, On 2017/05/23 22:32:18, Daniel Erat wrote: > does "meta" here correspond to the search key? if so, please consider using e.g. > SEARCH_SHIFT_A instead. i recommend staying away from the word "meta", since i > think we either use search or "command" to refer to this key in the code and in > user-facing strings. Acknowledged.
Description was changed from ========== add UMA metrics for voice interaction entries. BUG=b/38201455 TEST=manually test on device and check chrome://histograms ========== to ========== add UMA metrics for voice interaction entries. BUG=b/38201455 TEST=manually test on device ==========
Metrics LGTM % nits: https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16846: <action name="VoiceInteraction.Entry.AppListButtonLongPress"> Optional nit: I'd choose "Started" or "Launched" or even "Entered" over "Entry". https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16849: User launches voice interaction session through long pressing app list nit: s/launches/launched (and ditto throughout)
https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:131: "VoiceInteraction.MetaLayerEntry.Search_Shift_A")); oh, i thought "meta" was referring to the meta key but i see now that there are other references to a "meta layer" in this file. it looks like it's capitalized as "Metalayer" (i.e. all one word) elsewhere. if that's correct, please change this to match, i.e. "VoiceInteraction.MetalayerEntry.Search_Shift_A". https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:141: base::UserMetricsAction("VoiceInteraction.Entry.Search_Space")); return here to match the rest of the method and to avoid bugs if another case is added below later https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16846: <action name="VoiceInteraction.Entry.AppListButtonLongPress"> On 2017/05/24 05:47:59, Ilya Sherman wrote: > Optional nit: I'd choose "Started" or "Launched" or even "Entered" over "Entry". agreed. "entry" is ambiguous about whether it refers to entering something vs. e.g. an entry in a list of items. https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16871: User launches meta layer through Search + Shift + A. "metalayer" here too if that's what's typically used to refer to it
https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc (right): https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:131: "VoiceInteraction.MetaLayerEntry.Search_Shift_A")); On 2017/05/24 13:07:55, Daniel Erat wrote: > oh, i thought "meta" was referring to the meta key but i see now that there are > other references to a "meta layer" in this file. > > it looks like it's capitalized as "Metalayer" (i.e. all one word) elsewhere. if > that's correct, please change this to match, i.e. > "VoiceInteraction.MetalayerEntry.Search_Shift_A". Done. https://codereview.chromium.org/2900913002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc:141: base::UserMetricsAction("VoiceInteraction.Entry.Search_Space")); On 2017/05/24 13:07:55, Daniel Erat wrote: > return here to match the rest of the method and to avoid bugs if another case is > added below later Done. https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16846: <action name="VoiceInteraction.Entry.AppListButtonLongPress"> On 2017/05/24 05:47:59, Ilya Sherman wrote: > Optional nit: I'd choose "Started" or "Launched" or even "Entered" over "Entry". Done. https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16846: <action name="VoiceInteraction.Entry.AppListButtonLongPress"> On 2017/05/24 13:07:55, Daniel Erat wrote: > On 2017/05/24 05:47:59, Ilya Sherman wrote: > > Optional nit: I'd choose "Started" or "Launched" or even "Entered" over > "Entry". > > agreed. "entry" is ambiguous about whether it refers to entering something vs. > e.g. an entry in a list of items. Done. https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16849: User launches voice interaction session through long pressing app list On 2017/05/24 05:47:59, Ilya Sherman wrote: > nit: s/launches/launched (and ditto throughout) Done. https://codereview.chromium.org/2900913002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:16871: User launches meta layer through Search + Shift + A. On 2017/05/24 13:07:55, Daniel Erat wrote: > "metalayer" here too if that's what's typically used to refer to it Done.
lgtm
The CQ bit was checked by muyuanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, skuhne@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2900913002/#ps100001 (title: "address review comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495657485023930,
"parent_rev": "9963f7ea173756bc0b8014ed0dcd9bd48a6d3598", "commit_rev":
"2614b90ab83cd2fc4512d7b28dd7ba72cc28d197"}
Message was sent while issue was closed.
Description was changed from ========== add UMA metrics for voice interaction entries. BUG=b/38201455 TEST=manually test on device ========== to ========== add UMA metrics for voice interaction entries. BUG=b/38201455 TEST=manually test on device Review-Url: https://codereview.chromium.org/2900913002 Cr-Commit-Position: refs/heads/master@{#474415} Committed: https://chromium.googlesource.com/chromium/src/+/2614b90ab83cd2fc4512d7b28dd7... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2614b90ab83cd2fc4512d7b28dd7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
