Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130923002:
Added UMA metrics for display settings UI in the status area. (Closed)
|
Created:
4 years, 5 months ago by bruthig Modified:
4 years, 5 months ago CC:
chromium-reviews, kalyank, sadrul, asvitkine+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded UMA metrics for display settings UI in the status area.
Record user actions when:
- User clicks the display settings default view in the system menu
- User clicks a display notification
BUG=620508
TEST=manual
Committed: https://crrev.com/4501b27d39098f2c43089b88987f57adc93f9559
Cr-Commit-Position: refs/heads/master@{#404981}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Re-worked metrics and descriptions. #Patch Set 3 : Merge branch 'master' into uma_display #
Total comments: 14
Patch Set 4 : Addressed comments from patch set 3. #Patch Set 5 : Merge branch 'master' into uma_display #Patch Set 6 : Merge branch 'master' into uma_display #
Messages
Total messages: 45 (19 generated)
bruthig@chromium.org changed reviewers: + tdanderson@chromium.org
Terry, can you take a first look?
lgtm, one comment below, and ditto to my (optional) naming suggestions in https://chromiumcodereview.appspot.com/2125403002/ https://chromiumcodereview.appspot.com/2130923002/diff/1/ash/system/chromeos/... File ash/system/chromeos/tray_display.cc (right): https://chromiumcodereview.appspot.com/2130923002/diff/1/ash/system/chromeos/... ash/system/chromeos/tray_display.cc:116: void OpenSettings(UserMetricsAction action) { nit: Add a brief explanation to the documentation of OpenSettings() in the .h to explain the new parameter
bruthig@chromium.org changed reviewers: + jamescook@chromium.org, rkaplow@chromium.org
jamescook@, can you review changes in: - ash/system/chromeos/tray_display.cc rkaplow@, can you review changes in: - tools/metrics/actions/actions.xml - ash/metrics/user_metrics_recorder.cc tdanderson@, can you please take another look? I've reworked the actual metrics a bit. https://codereview.chromium.org/2130923002/diff/1/ash/system/chromeos/tray_di... File ash/system/chromeos/tray_display.cc (right): https://codereview.chromium.org/2130923002/diff/1/ash/system/chromeos/tray_di... ash/system/chromeos/tray_display.cc:116: void OpenSettings(UserMetricsAction action) { On 2016/07/07 21:41:39, tdanderson wrote: > nit: Add a brief explanation to the documentation of OpenSettings() in the .h to > explain the new parameter This got reworked a little bit.
lgtm
ash/common lgtm, but I had a comment about ash/metrics https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:203: // reduces the column count per line and allows for longer string literals. I don't think this comment is correct, and I would remove it. The regex eats newlines: https://cs-staging.chromium.org/chromium/src/tools/metrics/actions/extract_ac... And there are tests for metrics that span multiple lines: https://cs-staging.chromium.org/chromium/src/tools/metrics/actions/extract_ac... https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:204: using UserMetricsAction = base::UserMetricsAction; optional nit: I think just "using base::UserMetricsAction" is fine, and is what other code does. I'm glad that you did this, btw. It makes the code much easier to read.
https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:202: // UserMetricsActions must be on the same line as the string literal. This nit: UserMetricsAction -> UserMetricsAction nit: give a super-brief explanation why they must be on the same line https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13809: Counts the number of times the user selects the display row in the system Add something such as "...does not always open the display settings, e.g., if the row is selected from the lock screen." similar comment for 13836. https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13820: selecting the display row in the system menu. wording: "number of times the display settings are opened as a result of the user selecting the display row in the system menu" similar comment for 13850 https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13828: Counts the number of times a new display notification is created. nit: specify this is within the notifications center. https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13850: the system menu and not the general notification center. This is actually part of the notifications center (here and line 13838)
rkaplow@, can you please review the doc changes in: - base/metrics/user_metrics.h - base/metrics/user_metrics_action.h https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... File ash/metrics/user_metrics_recorder.cc (right): https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:202: // UserMetricsActions must be on the same line as the string literal. This On 2016/07/08 18:37:50, tdanderson wrote: > nit: UserMetricsAction -> UserMetricsAction > > nit: give a super-brief explanation why they must be on the same line Turns out the string literal doesn't need to be on the same line and the documentation is out of date. I will leave the using statements because it improves the readability however. https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:203: // reduces the column count per line and allows for longer string literals. On 2016/07/08 17:50:19, James Cook wrote: > I don't think this comment is correct, and I would remove it. > > The regex eats newlines: > https://cs-staging.chromium.org/chromium/src/tools/metrics/actions/extract_ac... > > And there are tests for metrics that span multiple lines: > https://cs-staging.chromium.org/chromium/src/tools/metrics/actions/extract_ac... Confirmed with rkaplow@ as well. Done. https://codereview.chromium.org/2130923002/diff/40001/ash/metrics/user_metric... ash/metrics/user_metrics_recorder.cc:204: using UserMetricsAction = base::UserMetricsAction; On 2016/07/08 17:50:19, James Cook wrote: > optional nit: I think just "using base::UserMetricsAction" is fine, and is what > other code does. > > I'm glad that you did this, btw. It makes the code much easier to read. Done. Also added using base::RecordAction; https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13809: Counts the number of times the user selects the display row in the system On 2016/07/08 18:37:50, tdanderson wrote: > Add something such as "...does not always open the display settings, e.g., if > the row is selected from the lock screen." > > similar comment for 13836. Done. https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13820: selecting the display row in the system menu. On 2016/07/08 18:37:50, tdanderson wrote: > wording: "number of times the display settings are opened as a result of the > user selecting the display row in the system menu" > > similar comment for 13850 Done. https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13828: Counts the number of times a new display notification is created. On 2016/07/08 18:37:50, tdanderson wrote: > nit: specify this is within the notifications center. Done. https://codereview.chromium.org/2130923002/diff/40001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:13850: the system menu and not the general notification center. On 2016/07/08 18:37:50, tdanderson wrote: > This is actually part of the notifications center (here and line 13838) Done.
lgtm
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2130923002/#ps80001 (title: "Merge branch 'master' into uma_display")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
bruthig@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in - base/metrics/*
lgtm
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, jamescook@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2130923002/#ps100001 (title: "Merge branch 'master' into uma_display")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Added UMA metrics for display settings UI in the status area. Record user actions when: - User clicks the display settings default view in the system menu - User clicks a display notification BUG=620508 TEST=manual ========== to ========== Added UMA metrics for display settings UI in the status area. Record user actions when: - User clicks the display settings default view in the system menu - User clicks a display notification BUG=620508 TEST=manual Committed: https://crrev.com/4501b27d39098f2c43089b88987f57adc93f9559 Cr-Commit-Position: refs/heads/master@{#404981} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4501b27d39098f2c43089b88987f57adc93f9559 Cr-Commit-Position: refs/heads/master@{#404981} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
