Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(308)

Issue 2856753002: Linux native notifications: Add server capabilities metrics (Closed)

Created:
3 years, 7 months ago by Tom (Use chromium acct)
Modified:
3 years, 7 months ago
Reviewers:
Peter Beverloo, Mark P
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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -8 lines) Patch
M chrome/browser/notifications/native_notification_display_service.cc View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_linux.cc View 1 2 3 4 5 6 chunks +72 lines, -8 lines 0 comments Download
M chrome/browser/notifications/notification_platform_bridge_linux_unittest.cc View 1 2 3 4 3 chunks +14 lines, -0 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 5 6 2 chunks +12 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 4 chunks +44 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 29 (14 generated)
Tom (Use chromium acct)
reviewers ptal peter: NPBL mpearson: histograms.xml
3 years, 7 months ago (2017-05-01 21:53:07 UTC) #1
Mark P
https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode283 chrome/browser/notifications/notification_platform_bridge_linux.cc:283: !!notification_proxy_); drive-by nit: I find "!!" hard to read. ...
3 years, 7 months ago (2017-05-01 22:07:01 UTC) #3
Tom (Use chromium acct)
https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/1/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode283 chrome/browser/notifications/notification_platform_bridge_linux.cc:283: !!notification_proxy_); On 2017/05/01 22:07:01, Mark P wrote: > drive-by ...
3 years, 7 months ago (2017-05-01 22:35:01 UTC) #4
Peter Beverloo
https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifications/native_notification_display_service.cc#newcode72 chrome/browser/notifications/native_notification_display_service.cc:72: // message center. Did `git cl format` do this? ...
3 years, 7 months ago (2017-05-02 14:15:24 UTC) #5
Tom (Use chromium acct)
https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/20001/chrome/browser/notifications/native_notification_display_service.cc#newcode72 chrome/browser/notifications/native_notification_display_service.cc:72: // message center. On 2017/05/02 14:15:24, Peter Beverloo wrote: ...
3 years, 7 months ago (2017-05-02 18:49:41 UTC) #7
Tom (Use chromium acct)
ping
3 years, 7 months ago (2017-05-03 18:26:36 UTC) #12
Mark P
https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode39 chrome/browser/notifications/notification_platform_bridge_linux.cc:39: enum class ConnectionInitializationStatusCode { Please follow the practices here: ...
3 years, 7 months ago (2017-05-03 19:20:40 UTC) #13
Tom (Use chromium acct)
https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc File chrome/browser/notifications/notification_platform_bridge_linux.cc (right): https://codereview.chromium.org/2856753002/diff/60001/chrome/browser/notifications/notification_platform_bridge_linux.cc#newcode39 chrome/browser/notifications/notification_platform_bridge_linux.cc:39: enum class ConnectionInitializationStatusCode { On 2017/05/03 19:20:40, Mark P ...
3 years, 7 months ago (2017-05-03 22:20:29 UTC) #15
Peter Beverloo
lgtm % naming nits https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notifications/native_notification_display_service.cc#newcode74 chrome/browser/notifications/native_notification_display_service.cc:74: // message center. nit: I ...
3 years, 7 months ago (2017-05-03 22:52:48 UTC) #17
Tom (Use chromium acct)
https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notifications/native_notification_display_service.cc File chrome/browser/notifications/native_notification_display_service.cc (right): https://codereview.chromium.org/2856753002/diff/100001/chrome/browser/notifications/native_notification_display_service.cc#newcode74 chrome/browser/notifications/native_notification_display_service.cc:74: // message center. On 2017/05/03 22:52:48, Peter Beverloo wrote: ...
3 years, 7 months ago (2017-05-04 00:07:38 UTC) #20
Tom (Use chromium acct)
pinging mpearson for histograms
3 years, 7 months ago (2017-05-04 18:17:10 UTC) #21
Mark P
histograms.xml lgtm modulo comments --mark https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2856753002/diff/120001/tools/metrics/histograms/histograms.xml#newcode43805 tools/metrics/histograms/histograms.xml:43805: + enum="BooleanSupported"> When you ...
3 years, 7 months ago (2017-05-04 18:27:00 UTC) #22
Tom (Use chromium acct)
Just saw the note on chromium-dev about the histograms.xml refactoring. Just had to move the ...
3 years, 7 months ago (2017-05-04 18:41:27 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2856753002/140001
3 years, 7 months ago (2017-05-04 18:42:10 UTC) #26
commit-bot: I haz the power
3 years, 7 months ago (2017-05-04 19:53:16 UTC) #29
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cb0550026886b2cdcd7bb9c97591...

Powered by Google App Engine
This is Rietveld 408576698