[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
index 28e96617a52b2cf40ab1f2454e20e75a424b0e47..1a151ea4a0446f761af86a990729439ee5c943a0 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java
@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.offlinepages;
 
 import android.content.Context;
+import android.os.Build;
 import android.os.Bundle;
 
 import com.google.android.gms.gcm.GcmNetworkManager;
@@ -21,50 +22,64 @@ public class BackgroundScheduler {
     private static final long NO_DELAY = 0;
     private static final boolean OVERWRITE = true;
 
-    /**
-     * For the given Triggering conditions, start a new GCM Network Manager request.
-     */
-    public static void schedule(Context context, TriggerConditions triggerConditions) {
-        schedule(context, triggerConditions, NO_DELAY, OVERWRITE);
+    private Context mContext;

[Pete Williamson, 2017/02/10 02:10:42]

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.

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

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.

[Pete Williamson, 2017/02/10 22:45:22]

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.

+
+    /** @return An instance of BackgroundScheduler for given context, relevant to the API level. */

[Pete Williamson, 2017/02/10 02:10:42]

nit: relevant to the API level. -> for the current API level.

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

Done.

+    public static BackgroundScheduler getInstance(Context context) {
+        if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.N) {
+            return new BackgroundScheduler(context);
+        }
+
+        return new BackgroundJobScheduler(context);

[Pete Williamson, 2017/02/10 02:10:42]

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)

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

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

+    }
+
+    protected BackgroundScheduler(Context context) {
+        mContext = context;
+    }
+
+    protected Context getContext() {
+        return mContext;
+    }
+
+    /** Schedules a GCM Network Manager task for provided triggering conditions. */
+    public void schedule(TriggerConditions triggerConditions) {
+        scheduleImpl(getContext(), triggerConditions, NO_DELAY, ONE_WEEK_IN_SECONDS, OVERWRITE);

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

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.

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

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

     }
 
     /**
      * If there is no currently scheduled task, then start a GCM Network Manager request
-     * for the given Triggering conditions but delayed to run after {@code delayStartSecs}.
+     * for the given Triggering conditions but delayed to run after {@code delayStartSeconds}.
      * Typically, the Request Coordinator will overwrite this task after task processing
      * and/or queue updates. This is a backup task in case processing is killed by the
      * system.
      */
-    public static void backupSchedule(
-            Context context, TriggerConditions triggerConditions, long delayStartSecs) {
-        schedule(context, triggerConditions, delayStartSecs, !OVERWRITE);
+    public void backupSchedule(TriggerConditions triggerConditions, long delayStartSeconds) {
+        scheduleImpl(getContext(), triggerConditions, delayStartSeconds, ONE_WEEK_IN_SECONDS,
+                !OVERWRITE);
     }
 
     /**
      * Cancel any outstanding GCM Network Manager requests.
      */
-    public static void unschedule(Context context) {
-        // Get the GCM Network Scheduler.
-        GcmNetworkManager gcmNetworkManager = GcmNetworkManager.getInstance(context);
-        gcmNetworkManager.cancelTask(OfflinePageUtils.TASK_TAG, ChromeBackgroundService.class);
+    public void unschedule() {
+        GcmNetworkManager.getInstance(getContext())
+                .cancelTask(OfflinePageUtils.TASK_TAG, ChromeBackgroundService.class);
     }
 
     /**
      * For the given Triggering conditions, start a new GCM Network Manager request allowed
      * to run after {@code delayStartSecs} seconds.
      */
-    private static void schedule(Context context, TriggerConditions triggerConditions,
-            long delayStartSecs, boolean overwrite) {
-        // Get the GCM Network Scheduler.
-        GcmNetworkManager gcmNetworkManager = GcmNetworkManager.getInstance(context);
-
+    protected void scheduleImpl(Context context, TriggerConditions triggerConditions,
+            long delayStartSeconds, long executionDeadlineSeconds, boolean overwrite) {
         Bundle taskExtras = new Bundle();
         TaskExtrasPacker.packTimeInBundle(taskExtras);
+        TaskExtrasPacker.packHoldWakelock(taskExtras);
         TaskExtrasPacker.packTriggerConditionsInBundle(taskExtras, triggerConditions);
 
         Task task = new OneoffTask.Builder()
                             .setService(ChromeBackgroundService.class)
-                            .setExecutionWindow(delayStartSecs, ONE_WEEK_IN_SECONDS)
+                            .setExecutionWindow(delayStartSeconds, executionDeadlineSeconds)
                             .setTag(OfflinePageUtils.TASK_TAG)
                             .setUpdateCurrent(overwrite)
                             .setRequiredNetwork(triggerConditions.requireUnmeteredNetwork()
@@ -74,7 +89,8 @@ public class BackgroundScheduler {
                             .setExtras(taskExtras)
                             .build();
 
-        gcmNetworkManager.schedule(task);
+        // Schedule a task using GCM network manager.
+        GcmNetworkManager.getInstance(context).schedule(task);
     }
 
     /**