[android] Add JobScheduler-based BackgroundTaskScheduler.

[android] Add JobScheduler-based BackgroundTaskScheduler.

In Android M+ it is encouraged to use JobScheduler for all background jobs,
instead of using things like IntentService or polling using alarms. Using the
system API is beneficial as it has a full view of what goes on in the system and
can schedule jobs accordingly.

However, this functionality was introduced in Android L, and
the API has been very stable since Android M. This means that we also need a
similar framework for older versions of Android, which is provided by
Google Play services. We prefer system APIs, since they do not require including
external libraries which bloats the APK size of Chrome and adds unnecessary
complexity. The GcmNetworkManager isonly used when the system API is not
available.

The background_task_scheduler component provides a new framework for use
within chromium to schedule and execute background jobs using the frameworks
available on a given version of Android.
The public API of the framework is similar to that of the Android
JobScheduler, but it is backed by either the system JobScheduler API or by
GcmNetworkManager. What service is used to back the framework remains unknown to
callers of the API.

This first CL only adds support for the JobScheduler based backend.

BUG=680757
Review-Url: https://codereview.chromium.org/2714463002
Cr-Commit-Position: refs/heads/master@{#453049}
Committed: https://chromium.googlesource.com/chromium/src/+/8a9fdb7818cf338102a0c1c889d19da5f704399a

[nyquist, 2017-02-22 10:57:51]

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

[commit-bot: I haz the power, 2017-02-22 10:58:28]

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

[nyquist, 2017-02-22 10:59:10]

nyquist@chromium.org changed reviewers:
+ dtrainor@chromium.org

[nyquist, 2017-02-22 10:59:11]

dtrainor: Could you take an initial stab at this?

It's quite a lot of JavaDoc and markdown documentation, so it looks much bigger
than it really is...

I suggest starting with reading the markdown documentation. I have uploaded it
as an experimental chromium ref so you can see it in gitiles here:
https://chromium.googlesource.com/chromium/src/+/150c38ab5815a12709180c56cd76...

[nyquist, 2017-02-22 11:01:46]

Description was changed from

==========
[android] Add JobScheduler-based BackgroundTaskScheduler.

In Android N+ it is encouraged to use JobScheduler for all background jobs,
instead of using things like IntentService or polling using alarms. Using the
system API is beneficial as it has a full view of what goes on in the system and
can schedule jobs accordingly.

However, this functionality was introduced in Android L, and
the API has been very stable since Android M. This means that we also need a
similar framework for older versions of Android, which is provided by
Google Play services. We prefer system APIs, since they do not require including
external libraries which bloats the APK size of Chrome and adds unnecessary
complexity. The GcmNetworkManager isonly used when the system API is not
available.

The background_task_scheduler component provides a new framework for use
within chromium to schedule and execute background jobs using the frameworks
available on a given version of Android.
The public API of the framework is similar to that of the Android
JobScheduler, but it is backed by either the system JobScheduler API or by
GcmNetworkManager. What service is used to back the framework remains unknown to
callers of the API.

This first CL only adds support for the JobScheduler based backend.

BUG=680757
==========

to

==========
[android] Add JobScheduler-based BackgroundTaskScheduler.

In Android M+ it is encouraged to use JobScheduler for all background jobs,
instead of using things like IntentService or polling using alarms. Using the
system API is beneficial as it has a full view of what goes on in the system and
can schedule jobs accordingly.

However, this functionality was introduced in Android L, and
the API has been very stable since Android M. This means that we also need a
similar framework for older versions of Android, which is provided by
Google Play services. We prefer system APIs, since they do not require including
external libraries which bloats the APK size of Chrome and adds unnecessary
complexity. The GcmNetworkManager isonly used when the system API is not
available.

The background_task_scheduler component provides a new framework for use
within chromium to schedule and execute background jobs using the frameworks
available on a given version of Android.
The public API of the framework is similar to that of the Android
JobScheduler, but it is backed by either the system JobScheduler API or by
GcmNetworkManager. What service is used to back the framework remains unknown to
callers of the API.

This first CL only adds support for the JobScheduler based backend.

BUG=680757
==========

[nyquist, 2017-02-22 11:08:06]

Apparently the background section of the documentation missed my last edits.
Please use the following link to read the markdown documentation from patch set
2:

https://chromium.googlesource.com/chromium/src/+/a8931a3b6b63b90e6f72333518c3...

[nyquist, 2017-02-22 11:08:16]

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

[commit-bot: I haz the power, 2017-02-22 11:08:35]

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-02-22 13:00:55]

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

[commit-bot: I haz the power, 2017-02-22 13:00:55]

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

[nyquist, 2017-02-22 18:24:12]

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/BUILD.gn (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/BUILD.gn:38: "//base:base_java",
Self-review: Also add a new DEPS-file.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/README.md (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:66: TaskInfo.createOneOffTask(42,
Self-review: Change to use TaskIds.YOUR_FEATURE

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:44:
// FIXME what if this happens way later?
Self-review: Need to look into this. The thought is around what happens if the
callback happens much, much later. As in; what if a new task is scheduled before
this task has finished processing? Should we trust the underlying framework to
stop the previous task first?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java:12:
public static final int OMAHA_JOB_ID = 0x00011684;
Self-review: Remove this ID or make a special ID for test instead.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerJobServiceTest.java:51:
TaskInfo.createOneOffTask(42, TestBackgroundTask.class,
Self-review: Use an ID specified TaskIds for all these tests. Just create a new
one for tests.

[David Trainor- moved to gerrit, 2017-02-24 07:19:50]

lgtm!  awesome :)

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/BUILD.gn (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/BUILD.gn:44: testonly = true
blank line after each block for consistency?

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/README.md (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:15: complexity. The
GcmNetworkManager isonly used when the system API is not
isonly -> is only

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:45: **Any class implementing this
interface must have a public constructor which takes
Should we assert this at register time?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:13:
* this interface must have a public which takes no arguments.
the word constructor should be in here somewhere :)

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:26:
* @param needsReSchedule whether this task must be rescheduled.
ReSchedule -> Reschedule everywhere?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:60:
Log.w(TAG, "Failed to start job, because class does not exist.");
can't instantiate class?

https://codereview.chromium.org/2714463002/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/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:17:
* It is backed by system APIs ({@link android.app.job.JobScheduler} on newer
platforms
missing ) after }.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:21:
public class BackgroundTaskScheduler {
Should you mention that people should access this via the factory?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java:12:
* A factory for {@link BackgroundTaskScheduler} that ensures there is only ever
a single instance.
Do we really ensure this?  Should we make the task scheduler an Impl and package
protected and just expose an interface?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerJobService.java:45:
* Retrieves the {@link TaskParameters} from the {@link JobParameters}, which are
passed as
Mention that only persistable data is transmitted somewhere when setting extras?
 I forget if you had it in another comment.

[Peter Beverloo, 2017-02-24 18:07:08]

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

[Peter Beverloo, 2017-02-24 18:07:14]

lgtm from the peanut gallery, thanks for doing this!

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/README.md (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:117: myBundle.putLong("number",
1337L);
This is awesome, no more Bundle/PersistableBundle conversion duplication

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:151: typically happens on a
different `Thread`, `Handler` or using an `AsyncTask`.
What if a feature cannot invoke yet TaskFinishedCallback either, because it
doesn't decisively know when a task has been finished?

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:42:
* @param context the current context.
micro nit: A blank line between the documentation paragraph and the parameters
would help readability a lot.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:21:
private static final String TAG = "BkgrdTaskJS";
Why not use the class' name, which is way more common? None of the docs for Log
(Chromium or Android's) mention a length limit.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java:12:
* A factory for {@link BackgroundTaskScheduler} that ensures there is only ever
a single instance.
On 2017/02/24 07:19:49, David Trainor wrote:
> Do we really ensure this?  Should we make the task scheduler an Impl and
package
> protected and just expose an interface?

The BackgroundTaskScheduler's constructor already is package-protected, so I
think we're good?

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java:17:
private static BackgroundTaskSchedulerDelegate getSchedulerDelegate() {
Wild idea: should we mark this with @TargetApi on M to prevent code from
accidentally assuming it works for M? There may be potential users with poor
test coverage of the start-up path, as that's often non-trivial to test.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:25:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
nit: docs say API 21, annotation says 22. Is this because of the getBoolean()
changes?

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:37:
private Result(@NonNull PersistableBundle bundle, @NonNull Set<String>
failedKeys) {
nit: Up to the Java crowd, but personally I find it confusing when some
arguments are annotated with @Nullable, some are annotated with @NonNull, and
some have no annotations. I prefer the binary system of @Nullable/none.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskInfo.java:333:
* milliseconds from now.
micro nit: weird wrapping

[Peter Beverloo, 2017-02-24 19:43:49]

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:20:
class BackgroundTaskJobService extends JobService {
This must be a `public` class so that Android can instantiate it.

[Peter Beverloo, 2017-02-24 20:29:44]

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:102:
} else {
This needs to be able to handle NULL values. I suggest adding:

    } else if (obj == null) {
        persistableBundle.putString(key, null);
    }

[nyquist, 2017-02-24 23:41:10]

nyquist@chromium.org changed reviewers:
+ jam@chromium.org

[nyquist, 2017-02-24 23:41:12]

jam: PTAL. This is the CL we talked about offline.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/BUILD.gn (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/BUILD.gn:38: "//base:base_java",
On 2017/02/22 18:24:12, nyquist wrote:
> Self-review: Also add a new DEPS-file.

Turns out this is unnecessary. //base is always OK to include (in top-level
DEPS), and the rest is from //third_party.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/BUILD.gn:44: testonly = true
On 2017/02/24 07:19:46, David Trainor wrote:
> blank line after each block for consistency?

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
File components/background_task_scheduler/README.md (right):

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:15: complexity. The
GcmNetworkManager isonly used when the system API is not
On 2017/02/24 07:19:46, David Trainor wrote:
> isonly -> is only

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:45: **Any class implementing this
interface must have a public constructor which takes
On 2017/02/24 07:19:47, David Trainor wrote:
> Should we assert this at register time?

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:66: TaskInfo.createOneOffTask(42,
On 2017/02/22 18:24:12, nyquist wrote:
> Self-review: Change to use TaskIds.YOUR_FEATURE

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:117: myBundle.putLong("number",
1337L);
On 2017/02/24 18:07:09, Peter Beverloo wrote:
> This is awesome, no more Bundle/PersistableBundle conversion duplication

Yeah, at least we only have to write that horrible conversion code once. Added
tests for it too.

Also added support for the null-case you asked about.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/README.md:151: typically happens on a
different `Thread`, `Handler` or using an `AsyncTask`.
On 2017/02/24 18:07:08, Peter Beverloo wrote:
> What if a feature cannot invoke yet TaskFinishedCallback either, because it
> doesn't decisively know when a task has been finished?

That would be something we have to think about I believe. It's not supported by
any of the underlying frameworks yet. I'm not sure how to fix this other than in
the users of the APIs themselves :-/

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:13:
* this interface must have a public which takes no arguments.
On 2017/02/24 07:19:48, David Trainor wrote:
> the word constructor should be in here somewhere :)

:-p Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:26:
* @param needsReSchedule whether this task must be rescheduled.
On 2017/02/24 07:19:48, David Trainor wrote:
> ReSchedule -> Reschedule everywhere?

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTask.java:42:
* @param context the current context.
On 2017/02/24 18:07:09, Peter Beverloo wrote:
> micro nit: A blank line between the documentation paragraph and the parameters
> would help readability a lot.

Very much agreed. Followed through for everything in this CL.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:21:
private static final String TAG = "BkgrdTaskJS";
On 2017/02/24 18:07:09, Peter Beverloo wrote:
> Why not use the class' name, which is way more common? None of the docs for
Log
> (Chromium or Android's) mention a length limit.

Nope. It's hidden in //PRESUBMIT.py:_CheckAndroidCrLogUsage(...)

https://cs.chromium.org/chromium/src/PRESUBMIT.py?type=cs&q=_CheckAndroidCrLo...

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskJobService.java:60:
Log.w(TAG, "Failed to start job, because class does not exist.");
On 2017/02/24 07:19:48, David Trainor wrote:
> can't instantiate class?

Done.

https://codereview.chromium.org/2714463002/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/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:17:
* It is backed by system APIs ({@link android.app.job.JobScheduler} on newer
platforms
On 2017/02/24 07:19:49, David Trainor wrote:
> missing ) after }.

Done.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskScheduler.java:21:
public class BackgroundTaskScheduler {
On 2017/02/24 07:19:49, David Trainor wrote:
> Should you mention that people should access this via the factory?

Good idea! Done.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java:12:
* A factory for {@link BackgroundTaskScheduler} that ensures there is only ever
a single instance.
On 2017/02/24 18:07:09, Peter Beverloo wrote:
> On 2017/02/24 07:19:49, David Trainor wrote:
> > Do we really ensure this?  Should we make the task scheduler an Impl and
> package
> > protected and just expose an interface?
> 
> The BackgroundTaskScheduler's constructor already is package-protected, so I
> think we're good?

I guess since the constructor is package protected like Peter says, maybe we're
good for now? I've also added a comment like you suggested in the other class
referring to the getScheduler() method in this class.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerFactory.java:17:
private static BackgroundTaskSchedulerDelegate getSchedulerDelegate() {
On 2017/02/24 18:07:09, Peter Beverloo wrote:
> Wild idea: should we mark this with @TargetApi on M to prevent code from
> accidentally assuming it works for M? There may be potential users with poor
> test coverage of the start-up path, as that's often non-trivial to test.

Good call. I'll do that for now until we have GcmNetworkManager support!

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerJobService.java:45:
* Retrieves the {@link TaskParameters} from the {@link JobParameters}, which are
passed as
On 2017/02/24 07:19:49, David Trainor wrote:
> Mention that only persistable data is transmitted somewhere when setting
extras?
>  I forget if you had it in another comment.

Done.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:25:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
On 2017/02/24 18:07:10, Peter Beverloo wrote:
> nit: docs say API 21, annotation says 22. Is this because of the getBoolean()
> changes?

Yes :-( Updated comment.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:37:
private Result(@NonNull PersistableBundle bundle, @NonNull Set<String>
failedKeys) {
On 2017/02/24 18:07:12, Peter Beverloo wrote:
> nit: Up to the Java crowd, but personally I find it confusing when some
> arguments are annotated with @Nullable, some are annotated with @NonNull, and
> some have no annotations. I prefer the binary system of @Nullable/none.

You have a good point. I guess I was just uncertain here, because sets and
bundles are often things that are null, so I'm not sure if developers would
trust it never being null and figured the annotation was just as good as me
adding a comment.

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BundleToPersistableBundleConverter.java:102:
} else {
On 2017/02/24 20:29:44, Peter Beverloo wrote:
> This needs to be able to handle NULL values. I suggest adding:
> 
>     } else if (obj == null) {
>         persistableBundle.putString(key, null);
>     }

Done. Also added test for this.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskIds.java:12:
public static final int OMAHA_JOB_ID = 0x00011684;
On 2017/02/22 18:24:12, nyquist wrote:
> Self-review: Remove this ID or make a special ID for test instead.

Done.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/TaskInfo.java:333:
* milliseconds from now.
On 2017/02/24 18:07:12, Peter Beverloo wrote:
> micro nit: weird wrapping

Wow, yeah. That was weird. Done.

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

https://codereview.chromium.org/2714463002/diff/20001/components/background_t...
components/background_task_scheduler/android/junit/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerJobServiceTest.java:51:
TaskInfo.createOneOffTask(42, TestBackgroundTask.class,
On 2017/02/22 18:24:12, nyquist wrote:
> Self-review: Use an ID specified TaskIds for all these tests. Just create a
new
> one for tests.

Done.

[jam, 2017-02-24 23:46:10]

lgtm for new directory

[nyquist, 2017-02-25 00:14:05]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2017-02-25 00:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dtrainor@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2714463002/#ps80001
(title: "Make correct class public")

[commit-bot: I haz the power, 2017-02-25 00:14:34]

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-02-25 01:07:43]

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

[commit-bot: I haz the power, 2017-02-25 01:07:44]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[nyquist, 2017-02-25 01:19:47]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2017-02-25 01:20:03]

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

[nyquist, 2017-02-25 01:31:50]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2017-02-25 01:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dtrainor@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2714463002/#ps100001
(title: "FindBugs wants the real Pi, but I won't give it.")

[commit-bot: I haz the power, 2017-02-25 01:32: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-02-25 02:35:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487986310727940,
"parent_rev": "c61da3e4d5f324c493728c5cc2f56c8dc962817d", "commit_rev":
"8a9fdb7818cf338102a0c1c889d19da5f704399a"}

[commit-bot: I haz the power, 2017-02-25 02:36:01]

Description was changed from

==========
[android] Add JobScheduler-based BackgroundTaskScheduler.

In Android M+ it is encouraged to use JobScheduler for all background jobs,
instead of using things like IntentService or polling using alarms. Using the
system API is beneficial as it has a full view of what goes on in the system and
can schedule jobs accordingly.

However, this functionality was introduced in Android L, and
the API has been very stable since Android M. This means that we also need a
similar framework for older versions of Android, which is provided by
Google Play services. We prefer system APIs, since they do not require including
external libraries which bloats the APK size of Chrome and adds unnecessary
complexity. The GcmNetworkManager isonly used when the system API is not
available.

The background_task_scheduler component provides a new framework for use
within chromium to schedule and execute background jobs using the frameworks
available on a given version of Android.
The public API of the framework is similar to that of the Android
JobScheduler, but it is backed by either the system JobScheduler API or by
GcmNetworkManager. What service is used to back the framework remains unknown to
callers of the API.

This first CL only adds support for the JobScheduler based backend.

BUG=680757
==========

to

==========
[android] Add JobScheduler-based BackgroundTaskScheduler.

In Android M+ it is encouraged to use JobScheduler for all background jobs,
instead of using things like IntentService or polling using alarms. Using the
system API is beneficial as it has a full view of what goes on in the system and
can schedule jobs accordingly.

However, this functionality was introduced in Android L, and
the API has been very stable since Android M. This means that we also need a
similar framework for older versions of Android, which is provided by
Google Play services. We prefer system APIs, since they do not require including
external libraries which bloats the APK size of Chrome and adds unnecessary
complexity. The GcmNetworkManager isonly used when the system API is not
available.

The background_task_scheduler component provides a new framework for use
within chromium to schedule and execute background jobs using the frameworks
available on a given version of Android.
The public API of the framework is similar to that of the Android
JobScheduler, but it is backed by either the system JobScheduler API or by
GcmNetworkManager. What service is used to back the framework remains unknown to
callers of the API.

This first CL only adds support for the JobScheduler based backend.

BUG=680757

Review-Url: https://codereview.chromium.org/2714463002
Cr-Commit-Position: refs/heads/master@{#453049}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a9fdb7818cf338102a0c1c889d1...
==========

[commit-bot: I haz the power, 2017-02-25 02:36:03]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8a9fdb7818cf338102a0c1c889d1...