[Android O] Refactor channel initialization

[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
Review-Url: https://codereview.chromium.org/2808163002
Cr-Commit-Position: refs/heads/master@{#463688}
Committed: https://chromium.googlesource.com/chromium/src/+/7776b8449261779e9660d4b3b48eca20492e21d3

[awdf, 2017-04-10 17:57:18]

Description was changed from

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 logic ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
==========

to

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 logic ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
==========

[awdf, 2017-04-10 17:57:18]

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

[awdf, 2017-04-10 17:58:00]

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

[commit-bot: I haz the power, 2017-04-10 17:58:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[awdf, 2017-04-10 18:01:47]

Description was changed from

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 logic ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
==========

to

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
==========

[nyquist, 2017-04-10 20:11:54]

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

https://codereview.chromium.org/2808163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:34:
static {
We try to avoid static initializers to ensure it doesn't happen when we don't
need to.

Could you do this lazily?

See here for a very similar example:
https://chromium.googlesource.com/chromium/src/+/7f44763d0280f85ef331e31f2ea9...

https://codereview.chromium.org/2808163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:48:
static {
Lazily here too.

https://codereview.chromium.org/2808163002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java
(right):

https://codereview.chromium.org/2808163002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java:55:
mChannelsInitializer.ensureInitialized(ChannelsInitializer.CHANNEL_ID_SITES);
Should there be another test that calls ensureInitialized() more than once (with
different channels)? The rest of that test could be minimal of course, but just
the concept of being able to initialize multiple channels sounds relevant.

[commit-bot: I haz the power, 2017-04-10 20:28:04]

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

[commit-bot: I haz the power, 2017-04-10 20:28:05]

Dry run: This issue passed the CQ dry run.

[awdf, 2017-04-11 12:15:06]

Thanks for the review!
Please take a look at the new patchset.
(Fixes for comments + added a few more @ChannelId annotations)

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

https://codereview.chromium.org/2808163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:34:
static {
On 2017/04/10 20:11:54, nyquist wrote:
> We try to avoid static initializers to ensure it doesn't happen when we don't
> need to.
> 
> Could you do this lazily?
> 
> See here for a very similar example:
>
https://chromium.googlesource.com/chromium/src/+/7f44763d0280f85ef331e31f2ea9...

Ah good idea - I assume you mean this idiom:
https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom - Done.

https://codereview.chromium.org/2808163002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:48:
static {
On 2017/04/10 20:11:54, nyquist wrote:
> Lazily here too.

Done.

https://codereview.chromium.org/2808163002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java
(right):

https://codereview.chromium.org/2808163002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsInitializerTest.java:55:
mChannelsInitializer.ensureInitialized(ChannelsInitializer.CHANNEL_ID_SITES);
On 2017/04/10 20:11:54, nyquist wrote:
> Should there be another test that calls ensureInitialized() more than once
(with
> different channels)? The rest of that test could be minimal of course, but
just
> the concept of being able to initialize multiple channels sounds relevant.

Yeah sounds reasonable - Done.

[awdf, 2017-04-11 15:51:16]

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

[commit-bot: I haz the power, 2017-04-11 15:51:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nyquist, 2017-04-11 16:21:05]

lgtm

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

https://codereview.chromium.org/2808163002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:35:
// Lazy holder idiom
Nit: Missing period at end of sentence.

[awdf, 2017-04-11 16:44:49]

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

https://codereview.chromium.org/2808163002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:35:
// Lazy holder idiom
On 2017/04/11 16:21:05, nyquist wrote:
> Nit: Missing period at end of sentence.

Done.

[Peter Beverloo, 2017-04-11 16:47:18]

lgtm

[awdf, 2017-04-11 16:49:39]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-11 16:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2808163002/#ps80001
(title: "fix comments")

[commit-bot: I haz the power, 2017-04-11 16:50:11]

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-11 18:20:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491929379679080,
"parent_rev": "1a7ec9987baf1b60ca25531a438a45567aa521bd", "commit_rev":
"7776b8449261779e9660d4b3b48eca20492e21d3"}

[commit-bot: I haz the power, 2017-04-11 18:21:40]

Description was changed from

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211
==========

to

==========
[Android O] Refactor channel initialization

- All the notification channel properties are now defined in the
 ChannelsInitializer and only the channel ID needs to be
 passed to the ChromeNotificationBuilder.

- This avoids previous duplication and this logic can be reused
 when we start initializing channels on launch and upgrade.

BUG=707211

Review-Url: https://codereview.chromium.org/2808163002
Cr-Commit-Position: refs/heads/master@{#463688}
Committed:
https://chromium.googlesource.com/chromium/src/+/7776b8449261779e9660d4b3b48e...
==========

[commit-bot: I haz the power, 2017-04-11 18:21:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/7776b8449261779e9660d4b3b48e...