[Android] BackgroundTaskScheduler reschedule implementation

[Android] BackgroundTaskScheduler reschedule implementation

This patch:
* Adds reschedule capability to BackgroundTask and BackgroundTaskScheduler
* Updates all implementations of BackgroundTask
* Adds capability to remove all stored tasks from shared preferences
* Adds a call to reschedule from BTSGcmTaskService.onInitializeTasks
* Adds appropriate tests

BUG=710630
R=nyquist@chromium.org

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

[fgorski, 2017-04-11 21:57:08]

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

[commit-bot: I haz the power, 2017-04-11 21:57:52]

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

[fgorski, 2017-04-11 22:19:31]

Description was changed from

==========
[Android] BackgroundTaskScheduler reschedule implementation

This patch:
* Adds reschedule capability to BackgroundTask and BackgroundTaskScheduler
* Updates all implementations of BackgroundTask
* Adds capability to remove all stored tasks from shared preferences
* Adds appropriate tests

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

to

==========
[Android] BackgroundTaskScheduler reschedule implementation

This patch:
* Adds reschedule capability to BackgroundTask and BackgroundTaskScheduler
* Updates all implementations of BackgroundTask
* Adds capability to remove all stored tasks from shared preferences
* Adds a call to reschedule from BTSGcmTaskService.onInitializeTasks
* Adds appropriate tests

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

[fgorski, 2017-04-11 22:19:47]

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

[commit-bot: I haz the power, 2017-04-11 22:20:13]

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-12 00:22:15]

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

[commit-bot: I haz the power, 2017-04-12 00:22:16]

Dry run: This issue passed the CQ dry run.

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

PTAL

[nyquist, 2017-04-18 22:33:03]

https://codereview.chromium.org/2814653003/diff/40001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2814653003/diff/40001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:808: android:exported="true">
Are there any security measures we should take with this since it's exported?
Can we verify the bind is from Google Play services?

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

https://codereview.chromium.org/2814653003/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:97:
BackgroundTaskSchedulerPrefs.removeAllTasks();
So, conceptually we could do this before or after all the tasks, or before/after
each individual class.
You chose the before-all version, which is fine, but could you add a short
comment as to why? For example mention that if any of the tasks make us crash,
we won't crash repeatedly, etc., or whatever you thought process was behind
this.

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

https://codereview.chromium.org/2814653003/diff/40001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java:57:
assertFalse(BackgroundTaskSchedulerPrefs.getScheduledTasks().contains(
It's unclear to me why this should be false. Don't we unconditionally add a task
to these prefs? I thought we only removed things from the set when we cancel a
task, or reschedule all of them?

[fgorski, 2017-04-20 19:09:58]

PTAL (Let's discuss some of that stuff)

https://codereview.chromium.org/2814653003/diff/40001/chrome/android/java/And...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2814653003/diff/40001/chrome/android/java/And...
chrome/android/java/AndroidManifest.xml:808: android:exported="true">
On 2017/04/18 22:33:02, nyquist wrote:
> Are there any security measures we should take with this since it's exported?
> Can we verify the bind is from Google Play services?

I am not sure how that would be done. This was taken straight from the
ChromeBackgroundService... I don't know that there is any check there either.

Let's sync about that.

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

https://codereview.chromium.org/2814653003/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:97:
BackgroundTaskSchedulerPrefs.removeAllTasks();
On 2017/04/18 22:33:03, nyquist wrote:
> So, conceptually we could do this before or after all the tasks, or
before/after
> each individual class.
> You chose the before-all version, which is fine, but could you add a short
> comment as to why? For example mention that if any of the tasks make us crash,
> we won't crash repeatedly, etc., or whatever you thought process was behind
> this.

I did it to avoid updating the prefs entry from both directions (schedule and
here).
I can do one of the following:
* remove-all as here (upside: we start fresh, downside: if early task reschedule
crashes us, we lose all tasks)
* remove one task before: (upside: we don't lose tasks by crash of some other
task... downside: we update the prefs a lot...)
* remove one task after: (won't work as we will remove entry for rescheduled
task)
* don't remove tasks and make sure that next scheduling does not write if
already there (wont work if the task decides not to reschedule itself... it will
be pending there)

WDYT?

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

https://codereview.chromium.org/2814653003/diff/40001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerTest.java:57:
assertFalse(BackgroundTaskSchedulerPrefs.getScheduledTasks().contains(
On 2017/04/18 22:33:03, nyquist wrote:
> It's unclear to me why this should be false. Don't we unconditionally add a
task
> to these prefs? I thought we only removed things from the set when we cancel a
> task, or reschedule all of them?

Good catch. This was a test bug.

I was thinking we shouldn't be unconditional thou, so I added an extra test.

[nyquist, 2017-04-25 20:44:05]

lgtm

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

https://codereview.chromium.org/2814653003/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:97:
BackgroundTaskSchedulerPrefs.removeAllTasks();
On 2017/04/20 19:09:58, fgorski wrote:
> On 2017/04/18 22:33:03, nyquist wrote:
> > So, conceptually we could do this before or after all the tasks, or
> before/after
> > each individual class.
> > You chose the before-all version, which is fine, but could you add a short
> > comment as to why? For example mention that if any of the tasks make us
crash,
> > we won't crash repeatedly, etc., or whatever you thought process was behind
> > this.
> 
> I did it to avoid updating the prefs entry from both directions (schedule and
> here).
> I can do one of the following:
> * remove-all as here (upside: we start fresh, downside: if early task
reschedule
> crashes us, we lose all tasks)
> * remove one task before: (upside: we don't lose tasks by crash of some other
> task... downside: we update the prefs a lot...)
> * remove one task after: (won't work as we will remove entry for rescheduled
> task)
> * don't remove tasks and make sure that next scheduling does not write if
> already there (wont work if the task decides not to reschedule itself... it
will
> be pending there)
> 
> WDYT?

I think what we have now is fine.

[fgorski, 2017-05-15 22:47:04]

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

[commit-bot: I haz the power, 2017-05-15 22:47:50]

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-16 00:43:30]

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

[commit-bot: I haz the power, 2017-05-16 00:43:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fgorski, 2017-05-16 17:30:20]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-05-16 17:30:20]

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/2814653003/#ps80001
(title: "Rebasing")

[commit-bot: I haz the power, 2017-05-16 17:32:55]

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-16 18:41:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494955820052560,
"parent_rev": "21bb521df1e2e311b05b07819b43e42a5db5c402", "commit_rev":
"df49c6da32ca2a0ed87e1d5f87f4032d9c4fdc12"}

[commit-bot: I haz the power, 2017-05-16 18:41:46]

Description was changed from

==========
[Android] BackgroundTaskScheduler reschedule implementation

This patch:
* Adds reschedule capability to BackgroundTask and BackgroundTaskScheduler
* Updates all implementations of BackgroundTask
* Adds capability to remove all stored tasks from shared preferences
* Adds a call to reschedule from BTSGcmTaskService.onInitializeTasks
* Adds appropriate tests

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

to

==========
[Android] BackgroundTaskScheduler reschedule implementation

This patch:
* Adds reschedule capability to BackgroundTask and BackgroundTaskScheduler
* Updates all implementations of BackgroundTask
* Adds capability to remove all stored tasks from shared preferences
* Adds a call to reschedule from BTSGcmTaskService.onInitializeTasks
* Adds appropriate tests

BUG=710630
R=nyquist@chromium.org

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

[commit-bot: I haz the power, 2017-05-16 18:41:47]

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