[Offline pages] Creating BackgroundJobScheduler, which uses JobScheduler

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 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;
 import com.google.android.gms.gcm.OneoffTask;
 import com.google.android.gms.gcm.Task;
 
 import org.chromium.chrome.browser.ChromeBackgroundService;
 
 /**
  * The background scheduler class is for setting GCM Network Manager tasks.
  */
 public class BackgroundScheduler {
     private static final long ONE_WEEK_IN_SECONDS = 60 * 60 * 24 * 7;
     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()
                                             ? Task.NETWORK_STATE_UNMETERED
                                             : Task.NETWORK_STATE_CONNECTED)
                             .setRequiresCharging(triggerConditions.requirePowerConnected())
                             .setExtras(taskExtras)
                             .build();
 
-        gcmNetworkManager.schedule(task);
+        // Schedule a task using GCM network manager.
+        GcmNetworkManager.getInstance(context).schedule(task);
     }
 
     /**
      * Get the latest power conditions from the android APIs.
      */
     public static boolean getPowerConditions(Context context) {
         return OfflinePageUtils.getPowerConditions(context);
     }
 
     /**
      * Get the latest battery conditions from the android APIs.
      */
     public static int getBatteryConditions(Context context) {
         return OfflinePageUtils.getBatteryConditions(context);
     }
 
     /**
      * Get the latest network conditions from the android APIs.
      */
     public static int getNetworkConditions(Context context) {
         return OfflinePageUtils.getNetworkConditions(context);
     }
 }