Add a general purpose facility for letting background tasks wait until

chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
index f408bc18d9ad990c7c9cc6373e3e7a721f92ab38..84a21ca5a4d3da9562d82c23ed17036bc66ec940 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
@@ -25,18 +25,25 @@ import org.chromium.chrome.browser.offlinepages.BackgroundSchedulerProcessorImpl
 import org.chromium.chrome.browser.offlinepages.OfflinePageUtils;
 import org.chromium.chrome.browser.precache.PrecacheController;
 
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
 /**
  * {@link ChromeBackgroundService} is scheduled through the {@link GcmNetworkManager} when the
  * browser needs to be launched for scheduled tasks, or in response to changing network or power
  * conditions.
  */
-public class ChromeBackgroundService extends GcmTaskService {
+public class ChromeBackgroundService extends GcmTaskService
+        implements ChromeBackgroundServiceWaiter {
     private static final String TAG = "BackgroundService";
+    /** Bundle value to use to specify we should wait before returning */

[dougarnett, 2016/06/13 21:53:35]

Bundle key actually

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+    public static final String WAIT_NEEDED = "WaitNeeded";

[dougarnett, 2016/06/13 21:53:34]

I wonder about something like HOLD_WAKELOCK here as this is kind of the api to
the requester.

Think we need to be more clear with naming and/or documentation about the intent
with this - eg, applies to blocking the onRunTask thread while processing
proceed on main UI thread and is responsible for signalling when done.

[Pete Williamson, 2016/06/13 23:23:10]

Constant changed, I added lots of doc, PTAL.

 
     @Override
     @VisibleForTesting
     public int onRunTask(final TaskParams params) {
         Log.i(TAG, "[" + params.getTag() + "] Woken up at " + new java.util.Date().toString());
+        setWaitLatchIfNeeded(params.getExtras());
         final Context context = this;
         ThreadUtils.runOnUiThread(new Runnable() {
             @Override
@@ -71,6 +78,10 @@ public class ChromeBackgroundService extends GcmTaskService {
                 }
             }
         });
+        // The BackgroundOffliner needs to hold the thread until the operation is complete,

[dougarnett, 2016/06/13 21:53:35]

Comment not clear wrt waht holding the thread achieve (or what threads are
involved)? 

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+        // if we have a background offlining task, then wait for it.
+        waitForTaskIfNeeded(params.getExtras());
+
         return GcmNetworkManager.RESULT_SUCCESS;
     }
 
@@ -128,19 +139,48 @@ public class ChromeBackgroundService extends GcmTaskService {
     }
 
     @VisibleForTesting
-    protected void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) {
+    private void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) {

[dougarnett, 2016/06/13 21:53:35]

remove above annotation if private now

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+        Log.d(TAG, "handleOfflinePageBackgroundLoad");

[dougarnett, 2016/06/13 21:53:35]

remove before landing?

[Pete Williamson, 2016/06/13 23:23:10]

Done.

         if (!LibraryLoader.isInitialized()) {
             launchBrowser(context);
         }
 
         // Call BackgroundTask, provide context.
-        BackgroundOfflinerTask task =
-                new BackgroundOfflinerTask(new BackgroundSchedulerProcessorImpl());
-        task.startBackgroundRequests(context, bundle);
+        if (mBackgroundOfflinerTask == null) {
+            mBackgroundOfflinerTask =
+                    new BackgroundOfflinerTask(new BackgroundSchedulerProcessorImpl());
+        }
+        mBackgroundOfflinerTask.startBackgroundRequests(context, bundle, this);
         // TODO(petewil) if processBackgroundRequest returns false, return RESTART_RESCHEDULE
         // to the GcmNetworkManager
     }
 
+    private void setWaitLatchIfNeeded(Bundle bundle) {

[dougarnett, 2016/06/13 21:53:35]

actually this method and the next one having javadoc is maybe more important
than the call sites having comments

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+        // If wait_needed is set to anything other than 0 (not found sets it to 0), wait.
+        if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) {

[dougarnett, 2016/06/13 21:53:35]

why getLong()? Do you expect to plumb wait time value? If so, should be
documented. If not, suggest using putBoolean/getBoolean

[Pete Williamson, 2016/06/13 23:23:10]

Added doc.  There are no get/put Boolean methods for bundle.

[dougarnett, 2016/06/14 01:42:45]

yes, there is putBoolean and getBoolean (2 forms for getBoolean, one with
default value)

[Pete Williamson, 2016/06/15 00:25:19]

Done.  Huh, I didn't see it on the doc for Bundle. 

+            mLatch = new CountDownLatch(1);
+        }
+    }
+
+    private void waitForTaskIfNeeded(Bundle bundle) {
+        // If wait_needed is set to anything other than 0 (not found sets it to 0), wait.
+        if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) {
+            try {
+                boolean waitSucceeded =  mLatch.await(4, TimeUnit.MINUTES);

[dougarnett, 2016/06/13 21:53:35]

maybe define constant for the 4 value

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+                if (!waitSucceeded) {
+                    Log.d(TAG, "waiting for latch timed out");
+                }
+            } catch (InterruptedException e) {
+                Log.d(TAG, "ChromeBackgroundService interrupted while holding wake lock. " + e);
+            }
+        }
+    }
+
+    @Override
+    public void onWaitDone() {
+        mLatch.countDown();

[dougarnett, 2016/06/13 21:53:34]

// Release the onRunTask thread to return so that the wakelock being hed for it
is cleared. ?

[Pete Williamson, 2016/06/13 23:23:10]

Doc added.

+    }
+
     @VisibleForTesting
     @SuppressFBWarnings("DM_EXIT")
     protected void launchBrowser(Context context) {
@@ -160,4 +200,7 @@ public class ChromeBackgroundService extends GcmTaskService {
         BackgroundSyncLauncher.rescheduleTasksOnUpgrade(this);
         PrecacheController.rescheduleTasksOnUpgrade(this);
     }
+
+    private BackgroundOfflinerTask mBackgroundOfflinerTask;

[dougarnett, 2016/06/13 21:53:35]

members before methods for Java classes

[Pete Williamson, 2016/06/13 23:23:10]

Done.

+    private CountDownLatch mLatch;
 }