[Android O] Only delete legacy notification channels on upgrade

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

[awdf, 2017-04-19 14:41:33]

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

[Peter Beverloo, 2017-04-19 15:10:34]

lgtm

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

https://codereview.chromium.org/2832493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelDefinitions.java:91:
private static final String[] LEGACY_CHANNEL_IDS = {};
So suppose that we remove Incognito, we'd add an entry "incognito" in here?
Would it make sense to define that legacy channels only have to be in this array
for a certain number of releases?

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

https://codereview.chromium.org/2832493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:32:
* Cleans up any old channels that are no longer required from previous versions
of the app.
nit: would be good to detail that it's safe to delete channels multiple times

[awdf, 2017-04-19 16:49:42]

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

https://codereview.chromium.org/2832493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelDefinitions.java:91:
private static final String[] LEGACY_CHANNEL_IDS = {};
On 2017/04/19 15:10:34, Peter Beverloo wrote:
> So suppose that we remove Incognito, we'd add an entry "incognito" in here?

Correct. Could retain the named constant even, but I think we should have a
policy of removing them from the @ChannelId stringdef so they can't be passed by
accident to ensureInitialized.

> Would it make sense to define that legacy channels only have to be in this
array
> for a certain number of releases?

I don't think so - aside from the difficulties of enforcing such a rule, we have
no guarantees about what version users might upgrade from and thus what legacy
channels they might have hanging around.

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

https://codereview.chromium.org/2832493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:32:
* Cleans up any old channels that are no longer required from previous versions
of the app.
On 2017/04/19 15:10:34, Peter Beverloo wrote:
> nit: would be good to detail that it's safe to delete channels multiple times

Done.

[awdf, 2017-04-21 14:56:34]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-21 14:56:35]

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/2832493002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-21 14:56:57]

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 16:03:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492786594703580,
"parent_rev": "63c1e2603b28e1fb6a2ca120e793497ed3b37220", "commit_rev":
"e0bb8b3085974f05fcb8c2a8f65a8585d749264f"}

[commit-bot: I haz the power, 2017-04-21 16:03:13]

Description was changed from

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

to

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

[commit-bot: I haz the power, 2017-04-21 16:03:14]

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