|
|
Created:
3 years, 7 months ago by Tom (Use chromium acct) Modified:
3 years, 7 months ago CC:
chromium-reviews, Peter Beverloo, mlamouri+watch-notifications_chromium.org, awdf+watch_chromium.org, asvitkine+watch_chromium.org, Lei Zhang Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLinux native notifications: Add server capabilities metrics
This CL adds the following metrics:
* If the system has a dbus object at /org/Freedesktop/Notifications
with interface org.Freedesktop.Notifications
* Server capabilities as returned by GetCapabilities
* Capabilities are stored in |capabilities_|. It is not used now,
but will be in the future.
* Whether native notifications are actually used
BUG=676220
R=peter@chromium.org,mpearson@chromium.org
CC=thestig@chromium.org
Review-Url: https://codereview.chromium.org/2856753002
Cr-Commit-Position: refs/heads/master@{#469434}
Committed: https://chromium.googlesource.com/chromium/src/+/cb0550026886b2cdcd7bb9c975910dcb85056b4c
Patch Set 1 #
Total comments: 10
Patch Set 2 : address mpearson@'s comments #
Total comments: 10
Patch Set 3 : address peter@'s comments #
Total comments: 13
Patch Set 4 : Fix test failure #Patch Set 5 : git cl lint #
Total comments: 6
Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : Rebase #
Dependent Patchsets: Messages
Total messages: 29 (14 generated)
reviewers ptal peter: NPBL mpearson: histograms.xml
Description was changed from ========== Linux native notifications: Add server capabilities metrics This CL adds the following metrics: * If the system has a dbus object at /org/Freedesktop/Notifications with interface org.Freedesktop.Notifications * Server capabilities as returned by GetCapabilities * Whether native notifications are actually used BUG=676220 R=peter@chromium.org,mpearson@chromium.org CC=thestig@chromium.org ========== to ========== Linux native notifications: Add server capabilities metrics This CL adds the following metrics: * If the system has a dbus object at /org/Freedesktop/Notifications with interface org.Freedesktop.Notifications * Server capabilities as returned by GetCapabilities * Capabilities are stored in |capabilities_|. It is not used now, but will be in the future. * Whether native notifications are actually used BUG=676220 R=peter@chromium.org,mpearson@chromium.org CC=thestig@chromium.org ==========
https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:283: !!notification_proxy_); drive-by nit: I find "!!" hard to read. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25609: + center. (Linux only) Logged on each start up. This description is clear, but it doesn't sound much like the comment in the code where it is emitted... // Called once the connection has been set up (or not). |success| // indicates the connection is ready to use. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46202: +<histogram name="org.Freedesktop.Notifications" enum="BooleanSupported"> I'm reluctant to start a new group "org.". The meaning of this isn't intuitive. What about Freedesktop.? By the way, I think we encourage all histogram names to be camelcase. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46212: +<histogram name="org.Freedesktop.Notifications.Capabilities.action-icons" I'm not sure if the dashboard supports dashes in histogram names. Please avoid. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46216: + Whether the notification server has the action-icons capability. (Linux Please clarify here and below that these are only recorded if there is a notification server. They're not logged on each start up.
https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_platform_bridge_linux.cc:283: !!notification_proxy_); On 2017/05/01 22:07:01, Mark P wrote: > drive-by nit: I find "!!" hard to read. Done. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:25609: + center. (Linux only) Logged on each start up. On 2017/05/01 22:07:01, Mark P wrote: > This description is clear, but it doesn't sound much like the comment in the > code where it is emitted... > // Called once the connection has been set up (or not). |success| > // indicates the connection is ready to use. Yeah, logging this in notification_platform_bridge_linux.cc was a bit odd. Moved the code into native_notification_display_service.cc. I think it makes more sense there. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46202: +<histogram name="org.Freedesktop.Notifications" enum="BooleanSupported"> On 2017/05/01 22:07:01, Mark P wrote: > I'm reluctant to start a new group "org.". The meaning of this isn't intuitive. > What about Freedesktop.? > > By the way, I think we encourage all histogram names to be camelcase. Done. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46212: +<histogram name="org.Freedesktop.Notifications.Capabilities.action-icons" On 2017/05/01 22:07:01, Mark P wrote: > I'm not sure if the dashboard supports dashes in histogram names. Please avoid. Done. https://codereview.chromium.org/2856753002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:46216: + Whether the notification server has the action-icons capability. (Linux On 2017/05/01 22:07:01, Mark P wrote: > Please clarify here and below that these are only recorded if there is a > notification server. They're not logged on each start up. Done.
https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:72: // message center. Did `git cl format` do this? These should be indented by two spaces.. https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:74: UMA_HISTOGRAM_BOOLEAN("Linux.NativeNotifications.UsingNativeNotifications", What about using: Notifications.UsingNativeNotificationCenter Then we'd be able to re-use it for Windows as well. https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:283: notification_proxy_ != nullptr); The difference between this one and the UMA defined in the native_notification_platform_bridge.cc file is that this includes Linux platforms that (a) expose the Freedesktop service, but (b) cannot bind to the signals, or, in the future, (c) do not adhere to the required capabilities. Would it be sensible to create an enum-based UMA to distinguish between those four statuses? - No freedesktop - Freedesktop, but missing capabilities - Freedesktop, but no connected signals - Freedesktop w/ capabilities and signals https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:323: capabilities_.count("sound")); nit: maybe move this to another method so that we can separate metrics from functional code? https://codereview.chromium.org/2856753002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21325: + <owner>thomasanderson@chromium.org</owner> nit: it would be nicer if we can define one <histogram> and append the capabilities as suffices. See <histogram_suffixes>.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:72: // message center. On 2017/05/02 14:15:24, Peter Beverloo wrote: > Did `git cl format` do this? These should be indented by two spaces.. Done. https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/native_notification_display_service.cc:74: UMA_HISTOGRAM_BOOLEAN("Linux.NativeNotifications.UsingNativeNotifications", On 2017/05/02 14:15:24, Peter Beverloo wrote: > What about using: > Notifications.UsingNativeNotificationCenter > > Then we'd be able to re-use it for Windows as well. Done. https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:283: notification_proxy_ != nullptr); On 2017/05/02 14:15:24, Peter Beverloo wrote: > The difference between this one and the UMA defined in the > native_notification_platform_bridge.cc file is that this includes Linux > platforms that (a) expose the Freedesktop service, but (b) cannot bind to the > signals, or, in the future, (c) do not adhere to the required capabilities. > > Would it be sensible to create an enum-based UMA to distinguish between those > four statuses? > > - No freedesktop > - Freedesktop, but missing capabilities > - Freedesktop, but no connected signals > - Freedesktop w/ capabilities and signals Done. https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:323: capabilities_.count("sound")); On 2017/05/02 14:15:24, Peter Beverloo wrote: > nit: maybe move this to another method so that we can separate metrics from > functional code? Done. https://codereview.chromium.org/2856753002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21325: + <owner>thomasanderson@chromium.org</owner> On 2017/05/02 14:15:24, Peter Beverloo wrote: > nit: it would be nicer if we can define one <histogram> and append the > capabilities as suffices. See <histogram_suffixes>. Done.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping
https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:39: enum class ConnectionInitializationStatusCode { Please follow the practices here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:42: MISSING_REQUIRED_CAPABILITIES, This value doesn't seem to be used anywhere. https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:614: // callsite, so we can't roll the below into a nice loop. Thank you for the comment. https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21233: +<histogram name="Freedesktop.Notifications.Capabilities" I think you're supposed to have base="true" here for histograms that have suffixes. https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21234: + enum="BooleanNotificationServerCapabilitySupported"> nit: Don't bother adding a new enum. Just use BooleanSupported. (which by the way has 1==supported, which is what I think makes sense. This is the opposite of your enum.) https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21241: + successfully. optional nit: ... which happens at most once on startup https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43862: + center. Logged on each start up. Linux only?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:39: enum class ConnectionInitializationStatusCode { On 2017/05/03 19:20:40, Mark P wrote: > Please follow the practices here: > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... Done. https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_platform_bridge_linux.cc:42: MISSING_REQUIRED_CAPABILITIES, On 2017/05/03 19:20:40, Mark P wrote: > This value doesn't seem to be used anywhere. It will be used soon in the future https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21233: +<histogram name="Freedesktop.Notifications.Capabilities" On 2017/05/03 19:20:40, Mark P wrote: > I think you're supposed to have base="true" here for histograms that have > suffixes. Done. https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21234: + enum="BooleanNotificationServerCapabilitySupported"> On 2017/05/03 19:20:40, Mark P wrote: > nit: Don't bother adding a new enum. Just use BooleanSupported. > > (which by the way has 1==supported, which is what I think makes sense. This is > the opposite of your enum.) Done. https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:21241: + successfully. On 2017/05/03 19:20:40, Mark P wrote: > optional nit: ... which happens at most once on startup Done. https://codereview.chromium.org/2856753002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43862: + center. Logged on each start up. On 2017/05/03 19:20:40, Mark P wrote: > Linux only? Nope, all platforms now. This was changed from the first patch set.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % naming nits https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:74: // message center. nit: I don't think this comment adds a lot, the code is clear. https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:621: "Linux.NotificationPlatformBridge.InitializationStatus", nit: Notifications.Linux.BridgeInitializationStatus? It'd be good to keep everything related to notifications in the Notifications. namespace. https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:645: UMA_HISTOGRAM_BOOLEAN("Freedesktop.Notifications.Capabilities.ActionIcons", nit: dito, why not Notifications.Freedesktop.XX?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/native_notification_display_service.cc:74: // message center. On 2017/05/03 22:52:48, Peter Beverloo wrote: > nit: I don't think this comment adds a lot, the code is clear. Done. https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:621: "Linux.NotificationPlatformBridge.InitializationStatus", On 2017/05/03 22:52:48, Peter Beverloo wrote: > nit: Notifications.Linux.BridgeInitializationStatus? It'd be good to keep > everything related to notifications in the Notifications. namespace. Done. https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_linux.cc:645: UMA_HISTOGRAM_BOOLEAN("Freedesktop.Notifications.Capabilities.ActionIcons", On 2017/05/03 22:52:48, Peter Beverloo wrote: > nit: dito, why not Notifications.Freedesktop.XX? Done.
pinging mpearson for histograms
histograms.xml lgtm modulo comments --mark https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43805: + enum="BooleanSupported"> When you changed the enum from BooleanNotificationServerCapabilitySupported to BooleanSupported, I notice you didn't change the code. Yet, the first enum had supports = 0, not supported = 1, and BooleanSupported does the opposite. Can you confirm the current code and enum value is correct, i.e., the older version was wrong? This also remind me, have you tested this code using about:histograms? https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43861: + The status code for initializing NotificationPlatformBridgeLinux. Logged on Linux only, as the histogram name implies?
Just saw the note on chromium-dev about the histograms.xml refactoring. Just had to move the 2 added enums to enums.xml https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43805: + enum="BooleanSupported"> On 2017/05/04 18:27:00, Mark P wrote: > When you changed the enum from > BooleanNotificationServerCapabilitySupported > to > BooleanSupported, > I notice you didn't change the code. > > Yet, the first enum had supports = 0, not supported = 1, and BooleanSupported > does the opposite. > > Can you confirm the current code and enum value is correct, i.e., the older > version was wrong? > It was wrong before, and is correct now. > This also remind me, have you tested this code using about:histograms? Didn't know about that page, just tested/verified the histograms now! https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43861: + The status code for initializing NotificationPlatformBridgeLinux. Logged on On 2017/05/04 18:27:00, Mark P wrote: > Linux only, as the histogram name implies? Done.
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2856753002/#ps140001 (title: "Rebase")
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": 140001, "attempt_start_ts": 1493923294397180, "parent_rev": "27b0b2aa98566217498c58e9174da2c6789cd433", "commit_rev": "cb0550026886b2cdcd7bb9c975910dcb85056b4c"}
Message was sent while issue was closed.
Description was changed from ========== Linux native notifications: Add server capabilities metrics This CL adds the following metrics: * If the system has a dbus object at /org/Freedesktop/Notifications with interface org.Freedesktop.Notifications * Server capabilities as returned by GetCapabilities * Capabilities are stored in |capabilities_|. It is not used now, but will be in the future. * Whether native notifications are actually used BUG=676220 R=peter@chromium.org,mpearson@chromium.org CC=thestig@chromium.org ========== to ========== Linux native notifications: Add server capabilities metrics This CL adds the following metrics: * If the system has a dbus object at /org/Freedesktop/Notifications with interface org.Freedesktop.Notifications * Server capabilities as returned by GetCapabilities * Capabilities are stored in |capabilities_|. It is not used now, but will be in the future. * Whether native notifications are actually used BUG=676220 R=peter@chromium.org,mpearson@chromium.org CC=thestig@chromium.org Review-Url: https://codereview.chromium.org/2856753002 Cr-Commit-Position: refs/heads/master@{#469434} Committed: https://chromium.googlesource.com/chromium/src/+/cb0550026886b2cdcd7bb9c97591... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/cb0550026886b2cdcd7bb9c97591... |