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

Issue 710843 link

Starred by 1 user
Status: Fixed
Owner:
█ awdf@chromium.org
Last visit > 30 days ago
Closed: Apr 2017
Cc:
peter@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Iterate through channels to be updated instead of deleting & recreating from scratch

Project Member Reported by awdf@chromium.org, Apr 12 2017 
The ChannelsUpdater/ChannelsInitializer should iterate through channels to be updated instead of deleting & recreating from scratch, to be:
a) more efficient
b) safer in terms of third party changes - we don't know if the effect of deleting a channel might change in future / on different devices
c) safer in terms of Chrome changes - there's a risk of a race condition if we try to update from two places simultaneously and crash before one has finished recreating the channels. Right now this shouldn't happen because the async task to update channels is scheduled on a serial executor but this could change.

Comment 1 by awdf@chromium.org, Apr 18 2017 

Indeed, we get the following error if there is only a system default channel and we try to delete it (noticed from running instrumentation tests):

04-18 15:35:40.928 E/cr_NotifManagerProxy(30690): Error deleting notification channel:
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690): java.lang.reflect.InvocationTargetException
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at java.lang.reflect.Method.invoke(Native Method)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at org.chromium.chrome.browser.notifications.NotificationManagerProxyImpl.d
eleteNotificationChannel(NotificationManagerProxyImpl.java:156)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at org.chromium.chrome.browser.notifications.ChannelsInitializer.deleteAllC
hannels(ChannelsInitializer.java:107)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at org.chromium.chrome.browser.notifications.ChannelsUpdater.updateChannels
(ChannelsUpdater.java:61)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at org.chromium.chrome.browser.DeferredStartupHandler$8.doInBackground(Defe
rredStartupHandler.java:261)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at org.chromium.chrome.browser.DeferredStartupHandler$8.doInBackground(Defe
rredStartupHandler.java:258)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at android.os.AsyncTask$2.call(AsyncTask.java:305)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at java.util.concurrent.FutureTask.run(FutureTask.java:266)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at android.os.AsyncTask$SerialExecutor$1.run(AsyncTask.java:243)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav
a:1162)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja
va:636)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690):       at java.lang.Thread.run(Thread.java:764)
04-18 15:35:40.928 E/cr_NotifManagerProxy(30690): Caused by: java.lang.IllegalArgumentException: Cannot delete default channel

Comment 2 by awdf@chromium.org, Apr 19 2017

Labels: -Pri-2 M-59 Pri-1
Status: Started (was: Assigned)
I'll implement this with an empty list of legacy channels to delete to start with- once b700377 is done the sites channel will need to be added to this.

Comment 3 Deleted

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 21 2017 

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

commit e0bb8b3085974f05fcb8c2a8f65a8585d749264f
Author: awdf <awdf@chromium.org>
Date: Fri Apr 21 16:02:58 2017

[Android O] Only delete legacy notification channels on upgrade

- Now instead of deleting all channels on upgrade, we only delete
those we know to be legacy channels.

- This fixes a crash where we tried to delete the default channel
created by Android causing an IllegalArgumentException.

BUG= 710843 

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

[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelDefinitions.java
[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java
[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/java_sources.gni
[add] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelDefinitionsTest.java
[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java
[modify] https://crrev.com/e0bb8b3085974f05fcb8c2a8f65a8585d749264f/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java

Comment 5 by awdf@chromium.org, Apr 21 2017

Labels: Merge-Request-59
Status: Fixed (was: Started)
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 21 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by awdf@chromium.org, Apr 21 2017

Labels: -Hotlist-Merge-Approved -Merge-Approved-59 Merge-Request-59
Forgot to attach the following CL to this bug which the approved patchset depends on: 

[Android O] Refactor channel definitions into new class

- As discussed, we can give this a per-file OWNERS for added safety
in case of channels being updated without increasing the version number.

- In future there will also be a requirement to move deprecated channels
into a new map, and thus a further need for safety.

- This refactoring will also make testing easier as fake channel
definitions can now be easily supplied in tests.

Review-Url: https://codereview.chromium.org/2832433002
Cr-Commit-Position: refs/heads/master@{#466316}
Committed: https://chromium.googlesource.com/chromium/src/+/8aacc8f8d02e5b19f491b34f744085dd2c521167

Please can I get merge-approval for the above change too?
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 21 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 21 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8041f5964cd3de050c62498f25ec559373412161

commit 8041f5964cd3de050c62498f25ec559373412161
Author: Anita Woodruff <awdf@chromium.org>
Date: Fri Apr 21 16:55:01 2017

[Android O] Refactor channel definitions into new class

- As discussed, we can give this a per-file OWNERS for added safety
in case of channels being updated without increasing the version number.

- In future there will also be a requirement to move deprecated channels
into a new map, and thus a further need for safety.

- This refactoring will also make testing easier as fake channel
definitions can now be easily supplied in tests.

Review-Url: https://codereview.chromium.org/2832433002
Cr-Commit-Position: refs/heads/master@{#466316}
(cherry picked from commit 8aacc8f8d02e5b19f491b34f744085dd2c521167)

BUG= 710843 

Review-Url: https://codereview.chromium.org/2831383002 .
Cr-Commit-Position: refs/branch-heads/3071@{#120}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationService.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/incognito/IncognitoNotificationManager.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationService.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
[add] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelDefinitions.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderFactory.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/OWNERS
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncNotificationController.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/java_sources.gni
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java
[modify] https://crrev.com/8041f5964cd3de050c62498f25ec559373412161/chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 21 2017 

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

commit 5f662891d23246fb57e349185b57cfa663a9db6b
Author: Anita Woodruff <awdf@chromium.org>
Date: Fri Apr 21 17:00:52 2017

[Android O] Only delete legacy notification channels on upgrade

- Now instead of deleting all channels on upgrade, we only delete
those we know to be legacy channels.

- This fixes a crash where we tried to delete the default channel
created by Android causing an IllegalArgumentException.

BUG= 710843 

Review-Url: https://codereview.chromium.org/2832493002
Cr-Commit-Position: refs/heads/master@{#466352}
(cherry picked from commit e0bb8b3085974f05fcb8c2a8f65a8585d749264f)

Review-Url: https://codereview.chromium.org/2829263002 .
Cr-Commit-Position: refs/branch-heads/3071@{#121}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelDefinitions.java
[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java
[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/java_sources.gni
[add] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelDefinitionsTest.java
[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java
[modify] https://crrev.com/5f662891d23246fb57e349185b57cfa663a9db6b/chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java
►

Sign in to add a comment