[Android] Implements OS upgrade check and rescheduling

[Android] Implements OS upgrade check and rescheduling

* Adds an OS upgrade check to the BackgroundTaskScheduler
* Adds OS version tracking to BackgroundTaskSchedulerPrefs
* Ensures GCMNetworkManager based upgrades only happen on supported OSs
* Calls OS version check from DeferredStartupHandler

BUG=710630
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2819703002
Cr-Commit-Position: refs/heads/master@{#473957}
Committed: https://chromium.googlesource.com/chromium/src/+/b10fbe550ed4e5ec7da45c2a07fd744502aee8f1

[fgorski, 2017-04-17 22:28:53]

Description was changed from

==========
[Android] Implements OS upgrade check and rescheduling

* Adds an OS upgrade check to the BackgroundTaskScheduler
* Adds OS version tracking to BackgroundTaskSchedulerPrefs
* Ensures GCMNetworkManager based upgrades only happen on supporting OSs

BUG=710630
R=nyquist@chromium.org
==========

to

==========
[Android] Implements OS upgrade check and rescheduling

* Adds an OS upgrade check to the BackgroundTaskScheduler
* Adds OS version tracking to BackgroundTaskSchedulerPrefs
* Ensures GCMNetworkManager based upgrades only happen on supported OSs
* Calls OS version check from DeferredStartupHandler

BUG=710630
R=nyquist@chromium.org
==========

[fgorski, 2017-04-18 20:22:58]

PTAL

[nyquist, 2017-04-18 22:41:08]

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
(right):

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:112:
if ((oldSdkInt < Build.VERSION_CODES.M && newSdkInt < Build.VERSION_CODES.M)
Do we want to extract the conditional out to something like:

if (!osUpgradeChangesDelegateType(oldSdkInt, newSdkInt)) return;

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:113:
|| (oldSdkInt >= Build.VERSION_CODES.M && newSdkInt >= Build.VERSION_CODES.M)) {
Maybe stupid question, but if oldSdkInt was somehow written badly to disk and is
a very high number, should we force an upgrade? I.e. it would look like a
downgrade, which  I don't think we support, but in case of ... I don't know...
Fire? Maybe just accept that as a bad state so force reschedule?

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:117:
// Explicitly invoke create old delegate type to cancel all scheduled tasks.
Something seems to be off with this sentence: "invoke create old...".
Maybe choose either invoke or create or add a word to bind them together?
Maybe something like: "Explicity create and invoke old delegate type to ..."

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
File
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java
(right):

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java:122:
/** This scenario tests upgrade from M+ to M+ OS, which requires no
rescheduling. */
Could you update this comment to match the test?

[fgorski, 2017-04-20 22:32:18]

Patchset #3 (id:40001) has been deleted

[fgorski, 2017-04-20 22:36:00]

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

[fgorski, 2017-04-20 22:36:07]

PTAL

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
(right):

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:112:
if ((oldSdkInt < Build.VERSION_CODES.M && newSdkInt < Build.VERSION_CODES.M)
On 2017/04/18 22:41:08, nyquist wrote:
> Do we want to extract the conditional out to something like:
> 
> if (!osUpgradeChangesDelegateType(oldSdkInt, newSdkInt)) return;

Done.

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:113:
|| (oldSdkInt >= Build.VERSION_CODES.M && newSdkInt >= Build.VERSION_CODES.M)) {
On 2017/04/18 22:41:08, nyquist wrote:
> Maybe stupid question, but if oldSdkInt was somehow written badly to disk and
is
> a very high number, should we force an upgrade? I.e. it would look like a
> downgrade, which  I don't think we support, but in case of ... I don't know...
> Fire? Maybe just accept that as a bad state so force reschedule?

I proposed some changes that work, but I am not very thrilled about these.

This
1) makes us support OS downgrade,
2) is a bit complex to test. (I can probably do it by either mocking
jobScheduler, or adding hooks in the factory)

Let me know what you think.

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:117:
// Explicitly invoke create old delegate type to cancel all scheduled tasks.
On 2017/04/18 22:41:08, nyquist wrote:
> Something seems to be off with this sentence: "invoke create old...".
> Maybe choose either invoke or create or add a word to bind them together?
> Maybe something like: "Explicity create and invoke old delegate type to ..."

Done.

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
File
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java
(right):

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java:122:
/** This scenario tests upgrade from M+ to M+ OS, which requires no
rescheduling. */
On 2017/04/18 22:41:08, nyquist wrote:
> Could you update this comment to match the test?

Done.

[commit-bot: I haz the power, 2017-04-20 22:36:30]

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-20 22:39:33]

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

[commit-bot: I haz the power, 2017-04-20 22:39:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nyquist, 2017-04-25 20:43:56]

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
(right):

https://codereview.chromium.org/2819703002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:113:
|| (oldSdkInt >= Build.VERSION_CODES.M && newSdkInt >= Build.VERSION_CODES.M)) {
On 2017/04/20 22:36:07, fgorski wrote:
> On 2017/04/18 22:41:08, nyquist wrote:
> > Maybe stupid question, but if oldSdkInt was somehow written badly to disk
and
> is
> > a very high number, should we force an upgrade? I.e. it would look like a
> > downgrade, which  I don't think we support, but in case of ... I don't
know...
> > Fire? Maybe just accept that as a bad state so force reschedule?
> 
> I proposed some changes that work, but I am not very thrilled about these.
> 
> This
> 1) makes us support OS downgrade,
> 2) is a bit complex to test. (I can probably do it by either mocking
> jobScheduler, or adding hooks in the factory)
> 
> Let me know what you think.

Yeah, I'm not sure if that is necessary. It feels weird :-/
Maybe just remove downgrade-support totally?
I mean, I don't even think it's officially supported anywhere.

[fgorski, 2017-05-22 21:27:43]

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

[commit-bot: I haz the power, 2017-05-22 21:28:24]

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-22 21:31:34]

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

[commit-bot: I haz the power, 2017-05-22 21:31:34]

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

[fgorski, 2017-05-22 21:32:15]

ptal. Cut out downgrade support

[nyquist, 2017-05-23 04:34:17]

lgtm

https://codereview.chromium.org/2819703002/diff/80001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
(right):

https://codereview.chromium.org/2819703002/diff/80001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:114:
// Check for OS upgrades or downgrades that would force delegate change or "just
in case"
Nit: This isn't checking for downgrades anymore.

[fgorski, 2017-05-23 16:29:59]

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

[commit-bot: I haz the power, 2017-05-23 16:30:18]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-05-23 16:39:06]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2017-05-23 16:39:07]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-05-23 16:39:08]

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/2819703002/#ps100001
(title: "Addressing final CR feedback")

[fgorski, 2017-05-23 16:39:20]

Thanks. Addressed.

https://codereview.chromium.org/2819703002/diff/80001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java
(right):

https://codereview.chromium.org/2819703002/diff/80001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:114:
// Check for OS upgrades or downgrades that would force delegate change or "just
in case"
On 2017/05/23 04:34:17, nyquist (nychthemeron ping) wrote:
> Nit: This isn't checking for downgrades anymore.

Done.

[commit-bot: I haz the power, 2017-05-23 16:39:33]

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-23 17:23:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495557547802760,
"parent_rev": "01a6ade045b8dc9c31e7f0e6e960099399dfe974", "commit_rev":
"b10fbe550ed4e5ec7da45c2a07fd744502aee8f1"}

[commit-bot: I haz the power, 2017-05-23 17:23:29]

Description was changed from

==========
[Android] Implements OS upgrade check and rescheduling

* Adds an OS upgrade check to the BackgroundTaskScheduler
* Adds OS version tracking to BackgroundTaskSchedulerPrefs
* Ensures GCMNetworkManager based upgrades only happen on supported OSs
* Calls OS version check from DeferredStartupHandler

BUG=710630
R=nyquist@chromium.org
==========

to

==========
[Android] Implements OS upgrade check and rescheduling

* Adds an OS upgrade check to the BackgroundTaskScheduler
* Adds OS version tracking to BackgroundTaskSchedulerPrefs
* Ensures GCMNetworkManager based upgrades only happen on supported OSs
* Calls OS version check from DeferredStartupHandler

BUG=710630
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2819703002
Cr-Commit-Position: refs/heads/master@{#473957}
Committed:
https://chromium.googlesource.com/chromium/src/+/b10fbe550ed4e5ec7da45c2a07fd...
==========

[commit-bot: I haz the power, 2017-05-23 17:23:31]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b10fbe550ed4e5ec7da45c2a07fd...