[Android O] Add SystemNotification.ChannelBlocked histogram

[Android O] Add SystemNotification.ChannelBlocked histogram

- Added a new histogram for notifications that were already being
logged via NotificationUmaTracker.

- Now, if notifications are enabled at the app level but blocked at the
channel level, they will be counted in this histogram, instead of the
SystemNotification.Shown histogram.

- If notifications are blocked at the app level they will still be
counted in SystemNotification.Blocked, regardless of channel status.

TBR=dtrainor@chromium.org

BUG=706813
Review-Url: https://codereview.chromium.org/2834633002
Cr-Commit-Position: refs/heads/master@{#466424}
Committed: https://chromium.googlesource.com/chromium/src/+/d0ba3b6e3f693d2a11992a48dc28508bbfc89731

[awdf, 2017-04-20 15:53:01]

awdf@chromium.org changed reviewers:
+ dtrainor@chromium.org, peter@chromium.org

[awdf, 2017-04-20 15:53:02]

Part 1 of 2.
peter@ & dtrainor@ - PTAL.

[Peter Beverloo, 2017-04-20 17:30:29]

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
(right):

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:75:
* @param channelId
nit: Id of the channel of the notification that was shown.

Lame but consistent.

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:85:
saveLastShownNotification(type);
Should this be in the branch of line 89? The last notification doesn't really
matter if it wasn't shown due to the channel being disabled - the intention of
that metric is to try and check what type of notification caused the user to
block something.

https://codereview.chromium.org/2834633002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2834633002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:30272: +    by type. Only applies to
devices on Android O and above.
nit: s/and could not be shown//? It's redundant with the first part of the
sentence, where you explain that they were blocked.

[awdf, 2017-04-21 13:18:33]

The CQ bit was checked by awdf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 13:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[awdf, 2017-04-21 13:36:28]

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
(right):

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:75:
* @param channelId
On 2017/04/20 17:30:29, Peter Beverloo wrote:
> nit: Id of the channel of the notification that was shown.
> 
> Lame but consistent.

Done.

https://codereview.chromium.org/2834633002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:85:
saveLastShownNotification(type);
On 2017/04/20 17:30:29, Peter Beverloo wrote:
> Should this be in the branch of line 89? The last notification doesn't really
> matter if it wasn't shown due to the channel being disabled - the intention of
> that metric is to try and check what type of notification caused the user to
> block something.

Good point. Fixed.

https://codereview.chromium.org/2834633002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2834633002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:30272: +    by type. Only applies to
devices on Android O and above.
On 2017/04/20 17:30:29, Peter Beverloo wrote:
> nit: s/and could not be shown//? It's redundant with the first part of the
> sentence, where you explain that they were blocked.

changed it a bit to remove the redundancy, and make it clearer it's logged
whenever we try to show a notification but it's blocked by channel.

[Peter Beverloo, 2017-04-21 14:26:09]

lgtm

[awdf, 2017-04-21 15:03:17]

awdf@chromium.org changed reviewers:
+ isherman@chromium.org

[awdf, 2017-04-21 15:03:18]

+ isherman@ for histograms - ptal

[awdf, 2017-04-21 15:03:19]

+ isherman@ for histograms - ptal

[nyquist, 2017-04-21 15:36:52]

nyquist@chromium.org changed reviewers:
+ nyquist@chromium.org

[nyquist, 2017-04-21 15:36:53]

lgtm, optionally also address comment.

https://codereview.chromium.org/2834633002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
(right):

https://codereview.chromium.org/2834633002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:100:
Method getNotificationChannel = notificationManager.getClass().getMethod(
Recently you've had this awesome trick of writing the result of the reflection
as a comment. I've found that incredibly helpful. Care to add that here as well?

[awdf, 2017-04-21 15:39:41]

Description was changed from

==========
[Android O] Add SystemNotification.ChannelBlocked histogram

- Added a new histogram for notifications that were already being
logged via NotificationUmaTracker.

- Now, if notifications are enabled at the app level but blocked at the
channel level, they will be counted in this histogram, instead of the
SystemNotification.Shown histogram.

- If notifications are blocked at the app level they will still be
counted in SystemNotification.Blocked, regardless of channel status.

BUG=706813
==========

to

==========
[Android O] Add SystemNotification.ChannelBlocked histogram

- Added a new histogram for notifications that were already being
logged via NotificationUmaTracker.

- Now, if notifications are enabled at the app level but blocked at the
channel level, they will be counted in this histogram, instead of the
SystemNotification.Shown histogram.

- If notifications are blocked at the app level they will still be
counted in SystemNotification.Blocked, regardless of channel status.

TBR=dtrainor@chromium.org

BUG=706813
==========

[awdf, 2017-04-21 15:40:23]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-21 15:40:32]

The CQ bit was unchecked by awdf@chromium.org

[awdf, 2017-04-21 15:56:30]

https://codereview.chromium.org/2834633002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
(right):

https://codereview.chromium.org/2834633002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java:100:
Method getNotificationChannel = notificationManager.getClass().getMethod(
On 2017/04/21 15:36:53, nyquist wrote:
> Recently you've had this awesome trick of writing the result of the reflection
> as a comment. I've found that incredibly helpful. Care to add that here as
well?

I got lazy! Will stop being lazy now I know someone else appreciates it though
:) Added.

[awdf, 2017-04-21 15:56:39]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-21 15:56:39]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2834633002/#ps60001
(title: "Add comment explaining reflection")

[commit-bot: I haz the power, 2017-04-21 15:57:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[awdf, 2017-04-21 16:00:19]

The CQ bit was unchecked by awdf@chromium.org

[awdf, 2017-04-21 16:04:16]

On 2017/04/21 15:57:13, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

(Cancelled since I realised I still need a metrics review.)

[Ilya Sherman, 2017-04-21 19:52:21]

Metrics LGTM, thanks.

[Ilya Sherman, 2017-04-21 19:52:44]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2017-04-21 19:53:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 19:59:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492804364332770,
"parent_rev": "8e9317d79912ed62f99c834d8f5048d5e7010130", "commit_rev":
"d0ba3b6e3f693d2a11992a48dc28508bbfc89731"}

[commit-bot: I haz the power, 2017-04-21 19:59:45]

Description was changed from

==========
[Android O] Add SystemNotification.ChannelBlocked histogram

- Added a new histogram for notifications that were already being
logged via NotificationUmaTracker.

- Now, if notifications are enabled at the app level but blocked at the
channel level, they will be counted in this histogram, instead of the
SystemNotification.Shown histogram.

- If notifications are blocked at the app level they will still be
counted in SystemNotification.Blocked, regardless of channel status.

TBR=dtrainor@chromium.org

BUG=706813
==========

to

==========
[Android O] Add SystemNotification.ChannelBlocked histogram

- Added a new histogram for notifications that were already being
logged via NotificationUmaTracker.

- Now, if notifications are enabled at the app level but blocked at the
channel level, they will be counted in this histogram, instead of the
SystemNotification.Shown histogram.

- If notifications are blocked at the app level they will still be
counted in SystemNotification.Blocked, regardless of channel status.

TBR=dtrainor@chromium.org

BUG=706813

Review-Url: https://codereview.chromium.org/2834633002
Cr-Commit-Position: refs/heads/master@{#466424}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0ba3b6e3f693d2a11992a48dc28...
==========

[commit-bot: I haz the power, 2017-04-21 19:59:46]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d0ba3b6e3f693d2a11992a48dc28...