[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

In order to enable scheduling using JobScheduler, this patch:
* Refactors BackgroundScheduler to expose its functionality through
  instance methods.
* Adds BackgroundScheduler#getInstance method to get appropriate
  implementation of BackgroundScheduler (default uses GCM Network
  Manager)
* Introduces BackgroundJobScheduler, which implements basic
  scheduling using JobScheduler
* Updates TasksExtrasPacker to handle PersistableBundle, which
  is required by JobScheduler (handling BaseBudnle would work if
  min API level was 21).
* Updates/adds tests.

BUG=683802, 692151
R=petewil@chromium.org,nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2686203002
Cr-Commit-Position: refs/heads/master@{#451866}
Committed: https://chromium.googlesource.com/chromium/src/+/21a89764bf8243494e3cb6f19096444bb7b9d6ed

[fgorski, 2017-02-09 23:45:26]

PTAL

[fgorski, 2017-02-09 23:46:40]

https://codereview.chromium.org/2686203002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:29:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) {
I'll use <= N_MR1 here so that anything above is using JobScheduler

[Pete Williamson, 2017-02-10 02:10:43]

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:28:
// TODO(fgorski): Cancel the thing here.
We should implement cancel and overwrite before turning on the JobScheduler.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:25:
private Context mContext;
Why hold onto the context?

As I recall, we need a fresh context with each call, which is why we were
passing it in before.  It should be fine as long as we always create the object
right before we use it, but that adds an extra limitation on the class.  If we
do hold onto the context, let's add a comment saying that we need a fresh
instance for every call to Schedule().  Alternately, we could go back to passing
in the context.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
/** @return An instance of BackgroundScheduler for given context, relevant to
the API level. */
nit: relevant to the API level. -> for the current API level.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:33:
return new BackgroundJobScheduler(context);
I'm not sure we are ready to turn this on yet, a lot of the background job
scheduler seems unimplemented. (Cancel, overwrite, onStartJob, etc)

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:46:
scheduleImpl(getContext(), triggerConditions, NO_DELAY, ONE_WEEK_IN_SECONDS,
OVERWRITE);
It might be cleaner to move the GCM impl into its own file, parallel to the way
we do the JobScheduler impl, and leave BackgroundScheduler as a pure interface
class.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeviceConditions.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeviceConditions.java:35:
/** @return Network connection type as defined by
org.chroium.net.ConnectionType. */
Comment might be slightly misleading.  We actually return an int with the same
values as org.chromium.net.ConnectionType, but not an instance of
ConnectionType.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:37:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
On the day this change lands, we might have some tasks stored with Bundle
instead of PersistentBundle.  What will happen when we unpack them?

[fgorski, 2017-02-10 19:23:19]

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

[commit-bot: I haz the power, 2017-02-10 19:23:59]

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

[fgorski, 2017-02-10 19:26:00]

Addressed. PTAL

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:28:
// TODO(fgorski): Cancel the thing here.
On 2017/02/10 02:10:42, Pete Williamson wrote:
> We should implement cancel and overwrite before turning on the JobScheduler.

Done. I changed the code enabling it to not do it yet.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:25:
private Context mContext;
On 2017/02/10 02:10:42, Pete Williamson wrote:
> Why hold onto the context?
> 
> As I recall, we need a fresh context with each call, which is why we were
> passing it in before.  It should be fine as long as we always create the
object
> right before we use it, but that adds an extra limitation on the class.  If we
> do hold onto the context, let's add a comment saying that we need a fresh
> instance for every call to Schedule().  Alternately, we could go back to
passing
> in the context.

It makes sense to add it for 2 reasons:
* All methods take context and this cleans up the interface a little bit.
* we centralize the API Level check into a single method, producing the
appropriate scheduler. (getInstance)

The old way is almost as good, I'd say, and obviously work, but we need internal
layer that does the same thing.

I can change it back if there is a good reason I haven't considered.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
/** @return An instance of BackgroundScheduler for given context, relevant to
the API level. */
On 2017/02/10 02:10:42, Pete Williamson wrote:
> nit: relevant to the API level. -> for the current API level.

Done.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:33:
return new BackgroundJobScheduler(context);
On 2017/02/10 02:10:42, Pete Williamson wrote:
> I'm not sure we are ready to turn this on yet, a lot of the background job
> scheduler seems unimplemented. (Cancel, overwrite, onStartJob, etc)

We are not. I added a TODO to leave it for next patch.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:46:
scheduleImpl(getContext(), triggerConditions, NO_DELAY, ONE_WEEK_IN_SECONDS,
OVERWRITE);
On 2017/02/10 02:10:43, Pete Williamson wrote:
> It might be cleaner to move the GCM impl into its own file, parallel to the
way
> we do the JobScheduler impl, and leave BackgroundScheduler as a pure interface
> class.

Done.
I thought about that earlier, but I was trying to avoid making this patch
bigger.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeviceConditions.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeviceConditions.java:35:
/** @return Network connection type as defined by
org.chroium.net.ConnectionType. */
On 2017/02/10 02:10:43, Pete Williamson wrote:
> Comment might be slightly misleading.  We actually return an int with the same
> values as org.chromium.net.ConnectionType, but not an instance of
> ConnectionType.

Updated.

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:37:
@TargetApi(Build.VERSION_CODES.LOLLIPOP_MR1)
On 2017/02/10 02:10:43, Pete Williamson wrote:
> On the day this change lands, we might have some tasks stored with Bundle
> instead of PersistentBundle.  What will happen when we unpack them?

Corresponding methods for Bundle still work. GCMNetworkManager is invoked
instead.

Creation of the schedule backup will use new scheduler, and everyone should be
happy.

[commit-bot: I haz the power, 2017-02-10 20:34:03]

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

[commit-bot: I haz the power, 2017-02-10 20:34:05]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2017-02-10 22:45:23]

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/10012/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:25:
private Context mContext;
On 2017/02/10 19:26:00, fgorski wrote:
> On 2017/02/10 02:10:42, Pete Williamson wrote:
> > Why hold onto the context?
> > 
> > As I recall, we need a fresh context with each call, which is why we were
> > passing it in before.  It should be fine as long as we always create the
> object
> > right before we use it, but that adds an extra limitation on the class.  If
we
> > do hold onto the context, let's add a comment saying that we need a fresh
> > instance for every call to Schedule().  Alternately, we could go back to
> passing
> > in the context.
> 
> It makes sense to add it for 2 reasons:
> * All methods take context and this cleans up the interface a little bit.
> * we centralize the API Level check into a single method, producing the
> appropriate scheduler. (getInstance)
> 
> The old way is almost as good, I'd say, and obviously work, but we need
internal
> layer that does the same thing.
> 
> I can change it back if there is a good reason I haven't considered.

I'm OK with leaving the code as is, but I still think we should add a warning
not to cache the instance across calls to the bridge to ensure that the context
is fresh.  My potential (weak) concern is that someday a maintenance programmer
could decide to cache this, which might lead to problems.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundGcmScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundGcmScheduler.java:19:
public class BackgroundGcmScheduler extends BackgroundScheduler {
Thanks!  Much nicer.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:47:
.setMinimumLatency(delayStartSeconds * 1000) // milliseconds
Let's make 1000 a constant.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
public static BackgroundScheduler getInstance(Context context) {
Should we add a warning somewhere that we should get a fresh instance every time
instead of caching these?

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:53:
public abstract void cancel();
I have a slight preference for unschedule over cancel.  I normally think of
cancel() as stopping a current task in progress (it is used that way in the
request coordinator), and of this method as removing a scheduling request (which
is not really in progress, just waiting to start).

[fgorski, 2017-02-13 05:13:48]

Addressed.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundGcmScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundGcmScheduler.java:19:
public class BackgroundGcmScheduler extends BackgroundScheduler {
On 2017/02/10 22:45:23, Pete Williamson wrote:
> Thanks!  Much nicer.

Acknowledged.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:47:
.setMinimumLatency(delayStartSeconds * 1000) // milliseconds
On 2017/02/10 22:45:23, Pete Williamson wrote:
> Let's make 1000 a constant.

Done.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:27:
public static BackgroundScheduler getInstance(Context context) {
On 2017/02/10 22:45:23, Pete Williamson wrote:
> Should we add a warning somewhere that we should get a fresh instance every
time
> instead of caching these?

Done.

https://codereview.chromium.org/2686203002/diff/30001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:53:
public abstract void cancel();
On 2017/02/10 22:45:23, Pete Williamson wrote:
> I have a slight preference for unschedule over cancel.  I normally think of
> cancel() as stopping a current task in progress (it is used that way in the
> request coordinator), and of this method as removing a scheduling request
(which
> is not really in progress, just waiting to start).

Both JobScheduler and GcmNetworkManager, as well as original documentation of
this method use cancel, and per our discussion I think it is OK to use it here.

[fgorski, 2017-02-13 05:17:07]

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

[commit-bot: I haz the power, 2017-02-13 05:17:20]

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-13 07:07:31]

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

[commit-bot: I haz the power, 2017-02-13 07:07:32]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-02-13 17:25:51]

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

[fgorski, 2017-02-13 17:25:52]

Tommy, can you provide an extra pair of eyes on this.

This is just a scheduling portion. I have a separate patch for service
implementation still in the works.

[Pete Williamson, 2017-02-13 18:17:35]

lgtm

lgtm

[Pete Williamson, 2017-02-13 18:17:35]

lgtm

[fgorski, 2017-02-16 20:41:48]

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

[fgorski, 2017-02-16 20:42:56]

Description was changed from

==========
[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

In order to enable scheduling using JobScheduler, this patch:
* Refactors BackgroundScheduler to expose its functionality through
  instance methods.
* Adds BackgroundScheduler#getInstance method to get appropriate
  implementation of BackgroundScheduler (default uses GCM Network
  Manager)
* Introduces BackgroundJobScheduler, which implements basic
  scheduling using JobScheduler
* Updates TasksExtrasPacker to handle PersistableBundle, which
  is required by JobScheduler (handling BaseBudnle would work if
  min API level was 21).
* Updates/adds tests.

BUG=683802
R=petewil@chromium.org
==========

to

==========
[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

In order to enable scheduling using JobScheduler, this patch:
* Refactors BackgroundScheduler to expose its functionality through
  instance methods.
* Adds BackgroundScheduler#getInstance method to get appropriate
  implementation of BackgroundScheduler (default uses GCM Network
  Manager)
* Introduces BackgroundJobScheduler, which implements basic
  scheduling using JobScheduler
* Updates TasksExtrasPacker to handle PersistableBundle, which
  is required by JobScheduler (handling BaseBudnle would work if
  min API level was 21).
* Updates/adds tests.

BUG=683802,692151
R=petewil@chromium.org,nyquist@chromium.org
==========

[commit-bot: I haz the power, 2017-02-16 20:43:27]

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

[fgorski, 2017-02-16 20:43:30]

Pete, I added null checks per our discussion. (To both old and new code)

Tommy, PTAL ;)

[commit-bot: I haz the power, 2017-02-16 22:23:04]

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

[commit-bot: I haz the power, 2017-02-16 22:23:09]

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

[nyquist, 2017-02-16 23:49:32]

this looks reasonable to me.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:21:
private static final long MILLISECONDS_IN_SECOND = 1000;
TimeUnit.SECONDS.toMillis(1)?

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:30:
if (jobScheduler == null) return;
When will this be null? And if it is, would you want to fall back to using GCM
as a best effort?
And does this change over time, or could we do this in the constructor, storing
a final member with a reference to this service? Then we could have something
like a "isAvailable()" or something on the BackgroundScheduler class as an
abstract method.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:53:
.setMinimumLatency(delayStartSeconds * MILLISECONDS_IN_SECOND)
How about something like: TimeUnit.SECONDS.toMillis(delayStartSeconds)

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:55:
.setPersisted(true) // across device resets
Nit: Double space before // and period at end of sentence.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:13:
private static final long ONE_WEEK_IN_SECONDS = 60 * 60 * 24 * 7;
Nit: TimeUnit.DAYS.toSeconds(7) ?

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:31:
// TODO(fgorski): Enable JobScheduler for >= N_MR1 once service implemented.
If a user upgrades the OS, do we need to force-cancel the old GCM-based jobs
before scheduling new ones? You don't have to address that in this CL though,
but maybe food for thought.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:27:
public static void packHoldWakelock(Bundle bundle) {
SELF: Who reads this?

[fgorski, 2017-02-17 22:31:15]

Addressed. PTAL

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:21:
private static final long MILLISECONDS_IN_SECOND = 1000;
On 2017/02/16 23:49:32, nyquist wrote:
> TimeUnit.SECONDS.toMillis(1)?

Done.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:30:
if (jobScheduler == null) return;
On 2017/02/16 23:49:32, nyquist wrote:
> When will this be null? And if it is, would you want to fall back to using GCM
> as a best effort?
This piece of code was added, as I was trying to sort the exception, when
GcmNetworkManager was not there. I presume that OS will give me something always
and I don't have to worry. Hence removing the null check.
If I'm proven to be wrong I'll happily fix this again.

> And does this change over time, or could we do this in the constructor,
storing
> a final member with a reference to this service? Then we could have something
> like a "isAvailable()" or something on the BackgroundScheduler class as an
> abstract method.

This all depends on whether context changes. If it doesn't then it may make
sense to keep the scheduler as a member variable. If it can change, which I
presume is the case, we don't plan to keep this class around. (It is not called
that commonly so discarding it does not cost much).

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:53:
.setMinimumLatency(delayStartSeconds * MILLISECONDS_IN_SECOND)
On 2017/02/16 23:49:32, nyquist wrote:
> How about something like: TimeUnit.SECONDS.toMillis(delayStartSeconds)

Done.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundJobScheduler.java:55:
.setPersisted(true) // across device resets
On 2017/02/16 23:49:32, nyquist wrote:
> Nit: Double space before // and period at end of sentence.

Done.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:13:
private static final long ONE_WEEK_IN_SECONDS = 60 * 60 * 24 * 7;
On 2017/02/16 23:49:32, nyquist wrote:
> Nit: TimeUnit.DAYS.toSeconds(7) ?

Done.

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:31:
// TODO(fgorski): Enable JobScheduler for >= N_MR1 once service implemented.
On 2017/02/16 23:49:32, nyquist wrote:
> If a user upgrades the OS, do we need to force-cancel the old GCM-based jobs
> before scheduling new ones? You don't have to address that in this CL though,
> but maybe food for thought.

We thought about that:

The first job that comes will be from Gcm. The subsequent scheduling will use
JobScheduler. We can detect the switch and release the wake-lock immediately
without doing any work.
Will that work? Or do you think the first job may not even be invoked?

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:27:
public static void packHoldWakelock(Bundle bundle) {
On 2017/02/16 23:49:32, nyquist wrote:
> SELF: Who reads this?

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

It is a way to request a wakelock.

[fgorski, 2017-02-17 22:31:20]

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

[commit-bot: I haz the power, 2017-02-17 22:33:54]

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-18 00:38:24]

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

[commit-bot: I haz the power, 2017-02-18 00:38:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[nyquist, 2017-02-21 18:40:13]

lgtm!

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
(right):

https://codereview.chromium.org/2686203002/diff/70001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:31:
// TODO(fgorski): Enable JobScheduler for >= N_MR1 once service implemented.
On 2017/02/17 22:31:15, fgorski wrote:
> On 2017/02/16 23:49:32, nyquist wrote:
> > If a user upgrades the OS, do we need to force-cancel the old GCM-based jobs
> > before scheduling new ones? You don't have to address that in this CL
though,
> > but maybe food for thought.
> 
> We thought about that:
> 
> The first job that comes will be from Gcm. The subsequent scheduling will use
> JobScheduler. We can detect the switch and release the wake-lock immediately
> without doing any work.
> Will that work? Or do you think the first job may not even be invoked?

Yeah, I think that should work. And yeah; I do think the job should be invoked
by gcm. Doing the switchover at that time should be fine.

It should be even more better if it happened automatically, but that might not
be easy, given we don't have access to the original JobInfo / TaskInfo, so you
running your own thing immediately should work.

[fgorski, 2017-02-21 18:45:01]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-02-21 18:45:01]

The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2686203002/#ps110001
(title: "Addressing feedback")

[commit-bot: I haz the power, 2017-02-21 18:45:48]

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-21 20:48:08]

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

[commit-bot: I haz the power, 2017-02-21 20:48:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[fgorski, 2017-02-21 23:11:08]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:12:16]

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 01:16:50]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1487718668502600,
"parent_rev": "8dd7ebcfe2458163d1faa4d0de517affec380d90", "commit_rev":
"21a89764bf8243494e3cb6f19096444bb7b9d6ed"}

[commit-bot: I haz the power, 2017-02-22 01:17:53]

Description was changed from

==========
[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

In order to enable scheduling using JobScheduler, this patch:
* Refactors BackgroundScheduler to expose its functionality through
  instance methods.
* Adds BackgroundScheduler#getInstance method to get appropriate
  implementation of BackgroundScheduler (default uses GCM Network
  Manager)
* Introduces BackgroundJobScheduler, which implements basic
  scheduling using JobScheduler
* Updates TasksExtrasPacker to handle PersistableBundle, which
  is required by JobScheduler (handling BaseBudnle would work if
  min API level was 21).
* Updates/adds tests.

BUG=683802,692151
R=petewil@chromium.org,nyquist@chromium.org
==========

to

==========
[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

In order to enable scheduling using JobScheduler, this patch:
* Refactors BackgroundScheduler to expose its functionality through
  instance methods.
* Adds BackgroundScheduler#getInstance method to get appropriate
  implementation of BackgroundScheduler (default uses GCM Network
  Manager)
* Introduces BackgroundJobScheduler, which implements basic
  scheduling using JobScheduler
* Updates TasksExtrasPacker to handle PersistableBundle, which
  is required by JobScheduler (handling BaseBudnle would work if
  min API level was 21).
* Updates/adds tests.

BUG=683802,692151
R=petewil@chromium.org,nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2686203002
Cr-Commit-Position: refs/heads/master@{#451866}
Committed:
https://chromium.googlesource.com/chromium/src/+/21a89764bf8243494e3cb6f19096...
==========

[commit-bot: I haz the power, 2017-02-22 01:17:55]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/21a89764bf8243494e3cb6f19096...