[Android O] Initialize channels on first launch/upgrade

[Android O] Initialize channels on first launch/upgrade

- Previously channels would only be created lazily, when a notification
 was posted to them.

- Now they are initialized on first launch (asynchronously) and on app
 upgrade, so they appear in OS Settings much sooner.

- Further work is still required to re-initialize channels on locale
 change, and maybe on boot to catch OS upgrade as soon as possible.

BUG=707211
Review-Url: https://codereview.chromium.org/2807213002
Cr-Commit-Position: refs/heads/master@{#464036}
Committed: https://chromium.googlesource.com/chromium/src/+/02dc9f2819cf895bc4ec0dd2a67a90435c58f1bc

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

Description was changed from

==========
[Android O] Initialize channels on first launch/upgrade

- Previously channels would only be created lazily, when a notification
 was posted to them.

- Now they are initialized on first launch (asynchronously) and on app
 upgrade, so they appear in OS Settings much sooner.

- Further work is still required to re-initialize channels on locale
 change, and maybe on boot to catch OS upgrade as soon as possible.

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

to

==========
[Android O] Initialize channels on first launch/upgrade

- Previously channels would only be created lazily, when a notification
 was posted to them.

- Now they are initialized on first launch (asynchronously) and on app
 upgrade, so they appear in OS Settings much sooner.

- Further work is still required to re-initialize channels on locale
 change, and maybe on boot to catch OS upgrade as soon as possible.

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

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

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

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

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:38]

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-04-10 19:05:56]

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

[commit-bot: I haz the power, 2017-04-10 19:05:57]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-04-10 21:04:30]

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:266:
.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
Nit: This indent looks weird.

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:20:
* Version number identifying the current set of channels. This should be
incremented whenever
s/should/must/

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:33:
public static void updateChannels(Context context) {
This seems to be an important part of our future lives... Do we want to make
this class testable? That would probably include the private method taking an
int, etc., and also probably doing some tricks like a LazyHolder to keep the
public API similar. And maybe mock the ChannelsInitializer or something along
those lines?

I don't feel strongly though, it's quite trivial code, but it feels like it
should be tested.

(see here for LazyHolder: 
https://chromium.googlesource.com/chromium/src/+/b87f01f54ba47ace1ddf35476a91...
)

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:32:
if (ChannelsUpdater.shouldUpdateChannels()) {
Do we want ChannelsUpdate.updateChannelsIfNecessary() which could contain this
logic?

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:33:
ChannelsUpdater.updateChannels(context);
I think this might end up writing to disk?
Do we want to goAsync() here to give ourselves some more time?
See https://codereview.chromium.org/2805533007/ as an example.

It'll ensure you won't get ANRs or StrictMode violations, etc. So I think it's
the right thing to do. See
https://developer.android.com/reference/android/content/BroadcastReceiver.htm...
for details.

[awdf, 2017-04-11 14:19:22]

Thanks for the review Tommy!

Please take another look.

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:266:
.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
On 2017/04/10 21:04:30, nyquist wrote:
> Nit: This indent looks weird.

This is enforced by 'git cl format' and occurs elsewhere in this file. But I
agree it looks weird, filed
https://bugs.chromium.org/p/chromium/issues/detail?id=710441

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:20:
* Version number identifying the current set of channels. This should be
incremented whenever
On 2017/04/10 21:04:30, nyquist wrote:
> s/should/must/

Done.

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:24:
private static final int CHANNELS_VERSION = 1;
I usually count from 0, set to 1 from manual testing. will revert.

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:33:
public static void updateChannels(Context context) {
On 2017/04/10 21:04:30, nyquist wrote:
> This seems to be an important part of our future lives... Do we want to make
> this class testable? That would probably include the private method taking an
> int, etc., and also probably doing some tricks like a LazyHolder to keep the
> public API similar. And maybe mock the ChannelsInitializer or something along
> those lines?
> 
> I don't feel strongly though, it's quite trivial code, but it feels like it
> should be tested.
> 
> (see here for LazyHolder: 
>
https://chromium.googlesource.com/chromium/src/+/b87f01f54ba47ace1ddf35476a91...
> )

The only method that is really testable here is updateChannels, due to the
static calls required in the other two (to BuildInfo & ContextUtils). So I
*could* make a test for updateChannels that checks a mocked channels initializer
has deleteAllChannels & initializeStartupChannels called on it, but given
there's no logic there it feels a bit pointless.

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:32:
if (ChannelsUpdater.shouldUpdateChannels()) {
On 2017/04/10 21:04:30, nyquist wrote:
> Do we want ChannelsUpdate.updateChannelsIfNecessary() which could contain this
> logic?

This isn't applicable now that I've added the async task within the check (with
goAsync which is broadcast-receiver specific).

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:33:
ChannelsUpdater.updateChannels(context);
On 2017/04/10 21:04:30, nyquist wrote:
> I think this might end up writing to disk?
> Do we want to goAsync() here to give ourselves some more time?
> See https://codereview.chromium.org/2805533007/ as an example.
> 
> It'll ensure you won't get ANRs or StrictMode violations, etc. So I think it's
> the right thing to do. See
>
https://developer.android.com/reference/android/content/BroadcastReceiver.htm...
> for details.

Ah cool I didn't know about that one - sounds like exactly what we need here.
Done.

[nyquist, 2017-04-11 16:26:01]

lgtm!

https://codereview.chromium.org/2807213002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:39:
if (ChannelsUpdater.shouldUpdateChannels()) {
Nit: Could you flip this and return early to remove indent?

[awdf, 2017-04-11 16:37:03]

oops forgot to publish this comment

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java
(right):

https://codereview.chromium.org/2807213002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:33:
public static void updateChannels(Context context) {
On 2017/04/11 14:19:21, awdf wrote:
> On 2017/04/10 21:04:30, nyquist wrote:
> > This seems to be an important part of our future lives... Do we want to make
> > this class testable? That would probably include the private method taking
an
> > int, etc., and also probably doing some tricks like a LazyHolder to keep the
> > public API similar. And maybe mock the ChannelsInitializer or something
along
> > those lines?
> > 
> > I don't feel strongly though, it's quite trivial code, but it feels like it
> > should be tested.
> > 
> > (see here for LazyHolder: 
> >
>
https://chromium.googlesource.com/chromium/src/+/b87f01f54ba47ace1ddf35476a91...
> > )
> 
> The only method that is really testable here is updateChannels, due to the
> static calls required in the other two (to BuildInfo & ContextUtils). So I
> *could* make a test for updateChannels that checks a mocked channels
initializer
> has deleteAllChannels & initializeStartupChannels called on it, but given
> there's no logic there it feels a bit pointless.

On second thoughts, if I just inject the NotificationManagerProxy so we use a
real ChannelsInitializer in the test & check mock notification manager channels
after update are as we expect, I could have a slightly more useful test to check
that ChannelsUpdater + ChannelsInitializer work together correctly.

Think I'll do that.

(Inspired by Peter's unrelated rant about how mocking frameworks ain't as good
as using the real thing)

[awdf, 2017-04-11 18:45:18]

Thanks for the lgtm Tommy :)

I've now added tests for ChannelsUpdater as discussed - would appreciate if you
could take another quick look.

https://codereview.chromium.org/2807213002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:39:
if (ChannelsUpdater.shouldUpdateChannels()) {
On 2017/04/11 16:26:00, nyquist wrote:
> Nit: Could you flip this and return early to remove indent?

Done.

[nyquist, 2017-04-11 19:03:49]

lgtm

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

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:27:
private final ChannelsInitializer mChannelsInitializer;
Optional: Extra newline before the non-static members?

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:29:
private boolean mIsAtLeastO;
Nit: final here and mChannelsVersion

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:39:
if (!ChannelsUpdater.getInstance().shouldUpdateChannels()) return;
Optional: For clarity, a newline after early return is probably nice for the
reader.

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java
(right):

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java:90:
@SuppressWarnings("WrongConstant") // for constructing the old channel with
invalid parameters
Nit: Capitalize For and add period.

[Peter Beverloo, 2017-04-11 19:08:29]

lgtm

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

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:40:
* {@link ChannelsUpdater#CHANNELS_VERSION} must be incremented every time an
entry is
Maybe consider making this a static member of ChannelsInitializer? This file has
the list of things that have to change when adding or removing a channel, so
it'd localize the needed changes.

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

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:62:
mChannelsInitializer.initializeStartupChannels();
This assumes that removing and recreating channels is fine, which may not
necessarily be the case in OEM adoptions of O. Maybe we should add a TODO for
only creating or deleting the channels that need to be modified?

Fine to wait until we can drop the reflection, I don't expect this to be an
issue in M59 in either case.

[awdf, 2017-04-12 13:58:15]

Thanks for the lgtms & nits! Fixed them all and added a couple more comments.

Also, made a small change in the ChannelsUpdater.LazyHolder to initialize with
nulls for app prefs and notification manager if build version is < O. Peter
over-the-shouldered it so I assume his lgtm still stands?

Tommy I'll probably land this without another lgtm from you but let me know if
you're not ok with that part and I will change it back.

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

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsInitializer.java:40:
* {@link ChannelsUpdater#CHANNELS_VERSION} must be incremented every time an
entry is
On 2017/04/11 19:08:29, Peter Beverloo wrote:
> Maybe consider making this a static member of ChannelsInitializer? This file
has
> the list of things that have to change when adding or removing a channel, so
> it'd localize the needed changes.

Done.

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

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:27:
private final ChannelsInitializer mChannelsInitializer;
On 2017/04/11 19:03:49, nyquist wrote:
> Optional: Extra newline before the non-static members?

Done.

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:29:
private boolean mIsAtLeastO;
On 2017/04/11 19:03:49, nyquist wrote:
> Nit: final here and mChannelsVersion

Done.

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/ChannelsUpdater.java:62:
mChannelsInitializer.initializeStartupChannels();
On 2017/04/11 19:08:29, Peter Beverloo wrote:
> This assumes that removing and recreating channels is fine, which may not
> necessarily be the case in OEM adoptions of O. Maybe we should add a TODO for
> only creating or deleting the channels that need to be modified?
> 
> Fine to wait until we can drop the reflection, I don't expect this to be an
> issue in M59 in either case.

Added a TODO. There's also the risk of a race condition right now, so agree we
should do this soon.

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java
(right):

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/upgrade/PackageReplacedBroadcastReceiver.java:39:
if (!ChannelsUpdater.getInstance().shouldUpdateChannels()) return;
On 2017/04/11 19:03:49, nyquist wrote:
> Optional: For clarity, a newline after early return is probably nice for the
> reader.

Done.

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java
(right):

https://codereview.chromium.org/2807213002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/notifications/ChannelsUpdaterTest.java:90:
@SuppressWarnings("WrongConstant") // for constructing the old channel with
invalid parameters
On 2017/04/11 19:03:49, nyquist wrote:
> Nit: Capitalize For and add period.

Went one better and turned it into an actual sentence.

[awdf, 2017-04-12 14:37:28]

The CQ bit was checked by awdf@chromium.org

[awdf, 2017-04-12 14:37:28]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2807213002/#ps120001
(title: "Some more minor things")

[commit-bot: I haz the power, 2017-04-12 14:37:52]

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-12 15:38:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492007848137350,
"parent_rev": "a72556982dcf08c91239167f2a5cd0e1b7f50ea8", "commit_rev":
"02dc9f2819cf895bc4ec0dd2a67a90435c58f1bc"}

[commit-bot: I haz the power, 2017-04-12 15:39:39]

Description was changed from

==========
[Android O] Initialize channels on first launch/upgrade

- Previously channels would only be created lazily, when a notification
 was posted to them.

- Now they are initialized on first launch (asynchronously) and on app
 upgrade, so they appear in OS Settings much sooner.

- Further work is still required to re-initialize channels on locale
 change, and maybe on boot to catch OS upgrade as soon as possible.

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

to

==========
[Android O] Initialize channels on first launch/upgrade

- Previously channels would only be created lazily, when a notification
 was posted to them.

- Now they are initialized on first launch (asynchronously) and on app
 upgrade, so they appear in OS Settings much sooner.

- Further work is still required to re-initialize channels on locale
 change, and maybe on boot to catch OS upgrade as soon as possible.

BUG=707211

Review-Url: https://codereview.chromium.org/2807213002
Cr-Commit-Position: refs/heads/master@{#464036}
Committed:
https://chromium.googlesource.com/chromium/src/+/02dc9f2819cf895bc4ec0dd2a67a...
==========

[commit-bot: I haz the power, 2017-04-12 15:39:39]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/02dc9f2819cf895bc4ec0dd2a67a...

[nyquist, 2017-04-12 15:47:51]

This still looks OK to me