[Android] Refactor our notification channel definitions

[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377, 716503
Review-Url: https://codereview.chromium.org/2859233002
Cr-Commit-Position: refs/heads/master@{#471829}
Committed: https://chromium.googlesource.com/chromium/src/+/19c23fdc0f568042d316dba0c9bd461e9e4e685b

[awdf, 2017-05-04 15:18:39]

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

[commit-bot: I haz the power, 2017-05-04 15:18:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[awdf, 2017-05-05 11:35:45]

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

[commit-bot: I haz the power, 2017-05-05 11:35:54]

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-05-05 12:50:27]

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

[commit-bot: I haz the power, 2017-05-05 12:50:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[awdf, 2017-05-08 15:57:29]

Description was changed from

==========
[Android] Refactor our notification channel definitions

- This refactor means in future we can have two Channel implementations:
  1. (Exists) One contructed with a resId for predefined channels
  2. (TODO) One constructed with a string name for site channels

BUG=700377
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer access the
resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

[awdf, 2017-05-08 15:58:17]

Description was changed from

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer access the
resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

[awdf, 2017-05-08 16:00:55]

Description was changed from

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

[awdf, 2017-05-08 16:01:08]

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

[awdf, 2017-05-08 16:02:16]

Description was changed from

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

[awdf, 2017-05-08 16:07:03]

Description was changed from

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377,716503
==========

[awdf, 2017-05-08 17:01:05]

This is ready for review now

[Peter Beverloo, 2017-05-09 13:02:54]

lgtm

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:19:
*    
http://developer.android.com/reference/android/app/NotificationManager.html</a>
nit: use https. Also quotes around the attribute.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java:60:
mNotificationManager.createNotificationChannel(channel);
nit: channelObject?

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/Channel.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/Channel.java:23:
this.mGroupId = groupId;
nit: drop |this.|

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java:58:
mNotificationManagerProxy.deleteNotificationChannel(channelId);
Shouldn't we start with a clean state here, i.e. delete all channels?

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java:243:
if (channel.getId().equals("miscellaneous")) it.remove();
nit: change this to be a constant

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
(left):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java:103:
return new String[] {"OldChannel", "AnotherOldChannel"};
Food for thought: at some point it might be a nice simplification to drop the
concept of legacy channels altogether - everything that's in the BROWSER group
and does not have a PredefinedChannel should just not be there.

[awdf, 2017-05-15 16:35:21]

Fixed the nits and made ChannelsUpdaterTest test what it was testing before by
mocking resources.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:19:
*    
http://developer.android.com/reference/android/app/NotificationManager.html</a>
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> nit: use https. Also quotes around the attribute.

Done.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxyImpl.java:60:
mNotificationManager.createNotificationChannel(channel);
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> nit: channelObject?

fixed.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/Channel.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/Channel.java:23:
this.mGroupId = groupId;
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> nit: drop |this.|

Done.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java
(right):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java:58:
mNotificationManagerProxy.deleteNotificationChannel(channelId);
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> Shouldn't we start with a clean state here, i.e. delete all channels?

It's only really safe to delete our own known channels - Android throws an
exception if we attempt to delete the 'misc' channel for example.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializerTest.java:243:
if (channel.getId().equals("miscellaneous")) it.remove();
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> nit: change this to be a constant

Done.

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
(left):

https://codereview.chromium.org/2859233002/diff/140001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java:103:
return new String[] {"OldChannel", "AnotherOldChannel"};
On 2017/05/09 13:02:53, Peter Beverloo wrote:
> Food for thought: at some point it might be a nice simplification to drop the
> concept of legacy channels altogether - everything that's in the BROWSER group
> and does not have a PredefinedChannel should just not be there.

Agree that might be nicer than relying on people updating the 'legacy channel
ids' array, but still isn't entirely safe as it assumes channels will only be
added & deprecated from the browser (GENERAL) group (..and that no third party
lib or the framework might create a notification channel within the GENERAL
group on our behalf). At least with this surgical approach we know exactly what
we're deleting.

[Peter Beverloo, 2017-05-15 16:41:47]

still lgtm, thanks!

https://codereview.chromium.org/2859233002/diff/160001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java
(right):

https://codereview.chromium.org/2859233002/diff/160001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdaterTest.java:132:
for (Channel ch : channels) {
micro nit: channels, fine to fix in another CL :)

[awdf, 2017-05-15 16:42:14]

The CQ bit was checked by awdf@chromium.org

[commit-bot: I haz the power, 2017-05-15 16:43:03]

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-05-15 18:12:58]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494866534202250,
"parent_rev": "727b9633cf8e84b3d79be12f6284248a9e129227", "commit_rev":
"19c23fdc0f568042d316dba0c9bd461e9e4e685b"}

[commit-bot: I haz the power, 2017-05-15 18:13:07]

Description was changed from

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377,716503
==========

to

==========
[Android] Refactor our notification channel definitions

This refactor means we now have two kinds of channels:
  1. PredefinedChannel, with a resId defining channel name.
    - This is what was previously known as ChannelDefinitions.Channel.
    - Can be converted to a channel when required.
  2. Channel - a drop-in replacement for Android's NotificationChannel.
    - This is useful until we target O and can use NotificationChannel
      directly. 
    - The ability to create a channel without a resId is required
      to allow site channels to be created in future.

As part of this change it was necessary to convert the existing
ChannelsInitializerTest to an instrumented test, as we can no longer
access the resId now the code refers to Channels. Since the test is
now instrumented, it makes sense to test against the real
NotificationManager, especially to test the fragile code that uses
reflection.

Finally, I made the ChannelsDefinition class no longer instantiable,
 as it didn't make much sense.

BUG=700377,716503

Review-Url: https://codereview.chromium.org/2859233002
Cr-Commit-Position: refs/heads/master@{#471829}
Committed:
https://chromium.googlesource.com/chromium/src/+/19c23fdc0f568042d316dba0c9bd...
==========

[commit-bot: I haz the power, 2017-05-15 18:13:08]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/19c23fdc0f568042d316dba0c9bd...