Android notifications: set group for push notifications

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}
Committed: https://chromium.googlesource.com/chromium/src/+/e424e1f96c68e959d2980c2f50ac3cee6a50dde9

[awdf, 2017-01-11 17:19:20]

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

[awdf, 2017-01-11 17:19:20]

cc: peter@ for review

Thought we should split this out from the summary notification in the interest
of helping resolve the original bug where all Chrome notifications are grouped
together on N.

note to self: check how this looks on Wear before pushing

[Peter Beverloo, 2017-01-11 23:05:25]

OK. This is a good first step, thanks!

lgtm % push -> web everywhere

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

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:432:
static void setGroupOnBuilder(Notification.Builder builder, CharSequence origin)
{
nit: docs

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:433:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT_WATCH && origin != null)
{
nit: prefer early return, which avoids a level of indentation

if (!...) {
    return;
}

(Or on one line if it fits.)

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

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:71:
static final String GROUP_PUSH_PREFIX = "Push:";
Let's call them "Web Notifications", since they can be shown without using push
as well. Prefix "Web"?

[awdf, 2017-01-12 16:33:42]

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

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:432:
static void setGroupOnBuilder(Notification.Builder builder, CharSequence origin)
{
On 2017/01/11 23:05:25, Peter Beverloo wrote:
> nit: docs

Done.

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:433:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT_WATCH && origin != null)
{
On 2017/01/11 23:05:25, Peter Beverloo wrote:
> nit: prefer early return, which avoids a level of indentation
> 
> if (!...) {
>     return;
> }
> 
> (Or on one line if it fits.)

Done.

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

https://codereview.chromium.org/2628723003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java:71:
static final String GROUP_PUSH_PREFIX = "Push:";
On 2017/01/11 23:05:25, Peter Beverloo wrote:
> Let's call them "Web Notifications", since they can be shown without using
push
> as well. Prefix "Web"?

Done.

[awdf, 2017-01-12 16:33:48]

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

[commit-bot: I haz the power, 2017-01-12 16:34:13]

Dry run: 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-01-12 16:35:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 16:35:50]

Dry run: 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[awdf, 2017-01-12 16:40:58]

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

[commit-bot: I haz the power, 2017-01-12 16:41:15]

Dry run: 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-01-12 17:15:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:15:45]

Dry run: This issue passed the CQ dry run.

[awdf, 2017-01-12 17:47:27]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-01-12 17:47:28]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2628723003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-12 17:48:16]

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-01-12 17:55:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484243247778190,
"parent_rev": "690cb481986ae0d2fc6820098a74086753f90771", "commit_rev":
"e424e1f96c68e959d2980c2f50ac3cee6a50dde9"}

[commit-bot: I haz the power, 2017-01-12 17:56:03]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/e424e1f96c68e959d2980c2f50ac...
==========

[commit-bot: I haz the power, 2017-01-12 17:56:04]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e424e1f96c68e959d2980c2f50ac...