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

Issue 726340 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Pass in Channel type to CustomNotificationBuilder

Project Member Reported by yfried...@chromium.org, May 25 2017

Issue description

As a follow-up to https://codereview.chromium.org/284119300 pass in the ChannelDefinition to CustomNotificationBuilder
 

Comment 1 by awdf@chromium.org, May 30 2017

Owner: awdf@chromium.org
Status: Started (was: Assigned)
Reassigning to myself since I'll fix it as part of  crbug.com/727178  - hope that's okay (planning on fixing it today since that's a M60 blocker)
Sure, np
Project Member

Comment 3 by bugdroid1@chromium.org, May 31 2017

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

commit 22291519e3fe40a961ef28dce4465e4111330a67
Author: Anita Woodruff <awdf@chromium.org>
Date: Wed May 31 17:03:22 2017

[Web apks] Stop setting channels on web apk notifications

- This fixes a crash when web apk notifications are displayed on
 recent builds of Android O.

- The crash occurred because the channel we were setting on web apk
 notifications was never created for the web apk - it was only
 created for the browser apk - while the notifications were posted by
 the web apk.

- This fixes things by not setting a channel on web apk notifications.
 Web apks do not target O yet so this is fine for now. When they do,
 web apks will need their own channel(s).

Bug:  727178 , 726340 
Change-Id: I4d200f1dc63c484c8728f0b21d993ef35b0cf90c
Reviewed-on: https://chromium-review.googlesource.com/519345
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475949}
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkDisclosureNotificationManager.java
[modify] https://crrev.com/22291519e3fe40a961ef28dce4465e4111330a67/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 1 2017

Labels: merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2fb2815a0c9b277427231ce47b9bc232f5ba65c6

commit 2fb2815a0c9b277427231ce47b9bc232f5ba65c6
Author: Anita Woodruff <awdf@chromium.org>
Date: Thu Jun 01 11:38:13 2017

[Web apks] Stop setting channels on web apk notifications

- This fixes a crash when web apk notifications are displayed on
 recent builds of Android O.

- The crash occurred because the channel we were setting on web apk
 notifications was never created for the web apk - it was only
 created for the browser apk - while the notifications were posted by
 the web apk.

- This fixes things by not setting a channel on web apk notifications.
 Web apks do not target O yet so this is fine for now. When they do,
 web apks will need their own channel(s).

R=peter@chromium.org

Bug:  727178 , 726340 
Change-Id: I4d200f1dc63c484c8728f0b21d993ef35b0cf90c
Reviewed-on: https://chromium-review.googlesource.com/519345
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#475949}
Review-Url: https://codereview.chromium.org/2922443002 .
Cr-Commit-Position: refs/branch-heads/3112@{#83}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationBuilderBaseTest.java
[modify] https://crrev.com/2fb2815a0c9b277427231ce47b9bc232f5ba65c6/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java

Comment 5 by awdf@chromium.org, Jun 1 2017

Status: Fixed (was: Started)

Sign in to add a comment