New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 674927 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Aug 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Group push notifications on Android by origin

Project Member Reported by awdf@chromium.org, Dec 16 2016

Issue description

Push notifications should be grouped by origin, this will also ensure they don't get bundled with other Chrome notifications on Android N+.
 

Comment 1 by awdf@chromium.org, Dec 21 2016

Looks like setGroup only groups them on Wear so we will also need to provide a summary notification in order for notifications to be grouped together by origin on phones/tablets.

Comment 2 by awdf@chromium.org, Jan 3 2017

Creating a summary notification is less simple than it sounds - it needs to be updated with the full list every time a notification is added, and NotificationManager.getActiveNotifications won't always return recently added notifications. This is probably why the Android N autobundling summary is sometimes inconsistent when lots of notifications are added in quick succession. Despite this, I'd say it's still worth doing, at least when a certain number of notifications are reached, as having too many notifications that don't fit on the lockscreen or in the shade produces a bad user experience.

We also need to decide where tapping on the summary notification should link to - presumably the origin - and whether to fire a notificationclick event (and if so with what values for the notificationevent).

Comment 3 by awdf@chromium.org, Jan 3 2017

In addition, we should decide whether tapping on the summary notification should dismiss all the notifications in the group (right now the automatic summary in N does not), and whether to display an action button for site settings on the summary notification. 




Comment 4 by peter@chromium.org, Jan 4 2017

Cc: miguelg@chromium.org owe...@chromium.org
+owencm for his opinions
+miguelg for a functional issue (2a)

Those are great questions, thanks! In no particular order -

    (1) Having a summary notification that does *something* beats automatic grouping that opens Chrome instead of anything in particular. This is more important when we group by origin, but clicking on it invokes an action that has nothing to do with said origin.

    (2a) The race in getActiveNotifications() is bad. (Internally tracked as b/32642818.) This may mean that we delete a notification's data if the developer calls ServiceWorkerRegistration.getNotifications() immediately after posting one, as the framework will report it's not showing anymore. *Miguel*, should we disable this behaviour for Android for now..?

    (2b) The race also impacts the regained-connectivity case, which makes it entirely valid for an origin to show multiple notifications in a brief amount of time. How feasible would it be to work around this ourselves by storing the titles of recently (~1s) shown notifications?

    (3) Ideally I'd like clicking on the summary notification to ungroup the notifications. Is this something we can do? I think we should avoid firing `notificationclick` for another notification not shown by the developer.

    (4) I'd vote yes on the site settings button. They're still coming from the website.

Do we know how other apps are planning on dealing with this? For example if I receive three comments on separate Google+ posts, how will those be summarized and what happens on tap?

It seems to me we should follow what those products would do, as effectively we're a platform for those same developers :)

I'd be worried for example if we summarize those and tapping it just goes to plus.google.com, that sounds like a bad experience to me so I wonder how other apps will deal with it.

Is there canonical documentation internally or externally about how apps should manage grouping?
2a looks really bad, let's disable it for now indeed. will send a CL.

Comment 7 by awdf@chromium.org, Jan 10 2017

miguelg@ I think it was disabled already due to test crashes on android M (on which getActiveNotifications is even more broken so we definitely shouldn't use it on M - I got NPEs just trying the Android notifications sample app on M) - see  https://bugs.chromium.org/p/chromium/issues/detail?id=674335 
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91a4892f8e93dd9328e4079197404c06bcd401b7

commit 91a4892f8e93dd9328e4079197404c06bcd401b7
Author: peter <peter@chromium.org>
Date: Thu Jan 12 14:30:40 2017

Add a group to sync-related Android notifications

This will avoid the system from inappropriately coalescing them with
other kinds of notifications shown by Chrome.

BUG= 674927 

Review-Url: https://codereview.chromium.org/2627963003
Cr-Commit-Position: refs/heads/master@{#443223}

[modify] https://crrev.com/91a4892f8e93dd9328e4079197404c06bcd401b7/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
[modify] https://crrev.com/91a4892f8e93dd9328e4079197404c06bcd401b7/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java

Comment 9 by awdf@chromium.org, Jan 13 2017

Status: Started (was: Assigned)
The following commit should have been attached to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e424e1f96c68e959d2980c2f50ac3cee6a50dde9

commit e424e1f96c68e959d2980c2f50ac3cee6a50dde9
Author: awdf <awdf@chromium.org>
Date: Thu Jan 12 17:55:27 2017

Android notifications: set group for push notifications

- Push notifications are no longer grouped with all other Chrome
  notifications

- However, this patch makes them standalone, rather than grouped by
  origin, as we aren't yet setting a summary notification.

BUG= 674015 ,674335

Review-Url: https://codereview.chromium.org/2628723003
Cr-Commit-Position: refs/heads/master@{#443285}

[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java
[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java
[modify] https://crrev.com/e424e1f96c68e959d2980c2f50ac3cee6a50dde9/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java

Comment 10 by awdf@chromium.org, Jan 17 2017

Blocking: -674015
This issue is no longer blocking  crbug.com/674015  now that web notifications are standalone thanks to the above commit.

I have been experimenting with summary notifications on N and found we need to manually maintain visibility of the summary notification ourselves otherwise they outlive their group members - see https://code.google.com/p/android/issues/detail?id=215803#c10. (Sounds like this should also be fixed in O though).

Q

Comment 11 by awdf@chromium.org, Apr 12 2017

Cc: dtrainor@chromium.org
Labels: -Pri-2 Pri-3
Status: Assigned (was: Started)
dtrainor@ did you get summary notifications working reliably for grouped download notifications in the end? Is it worth it?

Comment 12 by awdf@chromium.org, Oct 27 2017

TODO: Look into whether we can avoid using getActiveN10ns altogether if we only add summaries for standard (non custom) notifications. Apparently Android only uses the title + icon of these summaries and autopopulates the rest for us, at least on recent versions of Android.
Owner: peter@chromium.org
Owner: ----
Status: WontFix (was: Assigned)

Sign in to add a comment