Add fake for InstanceIDWithSubtype.java, in order to re-use unit test

components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java

Index: components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
diff --git a/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
index 1f61bd564a7f0cf52a92ea63e8227cd33b2bf8f9..b0c6ec0e5d32bec7f596f06eade20ffccbc24479 100644
--- a/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
+++ b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
@@ -14,6 +14,7 @@ import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 
 import java.io.IOException;
+import java.util.concurrent.ExecutionException;
 
 /**
  * Wraps InstanceID and InstanceIDWithSubtype so they can be used over JNI.
@@ -23,7 +24,9 @@ import java.io.IOException;
 public class InstanceIDBridge {
     /** Underlying InstanceID. May be shared by multiple InstanceIDBridges. */
     private final InstanceID mInstanceID;
-    private final long mNativeInstanceIDAndroid;
+    private long mNativeInstanceIDAndroid;
+
+    private static boolean sBlockOnAsyncTasksForTesting = false;
 
     private InstanceIDBridge(
             long nativeInstanceIDAndroid, Context context, String subtype) {
@@ -51,6 +54,16 @@ public class InstanceIDBridge {
         mNativeInstanceIDAndroid = 0;
     }
 
+    /**
+     * Enable this in tests such as unit tests where C++ never returns to the Java looper and the
+     * NestedSystemMessageHandler is disabled. In such tests, the onPostExecute of AsyncTasks will
+     * never get executed, so instead we'll synchronously block until each task completes.

[Peter Beverloo, 2016/04/15 00:00:17]

Please avoid use of "we" in comments. (Also goes for :177.) Maybe phrase this
as:

     * Will block returning the control flow to C++ until the asynchronous Java
operation has
     * completed. To be used in tests where no nested message loop is available.
The caller is
     * expected to reset this to "false" while tearing down.

[johnme, 2016/04/15 16:00:42]

Done.

+     */
+    @CalledByNative
+    private static void setBlockOnAsyncTasksForTesting(boolean block) {
+        sBlockOnAsyncTasksForTesting = block;
+    }
+
     /** Wrapper for {@link InstanceID#getId}. */
     @CalledByNative
     public String getId() {
@@ -74,9 +87,9 @@ public class InstanceIDBridge {
         for (int i = 0; i < extrasStrings.length; i += 2) {
             extras.putString(extrasStrings[i], extrasStrings[i + 1]);
         }
-        new AsyncTask<Void, Void, String>() {
+        new BridgeAsyncTask<String>() {
             @Override
-            protected String doInBackground(Void... params) {
+            protected String doBackgroundWork() {
                 try {
                     return mInstanceID.getToken(authorizedEntity, scope, extras);
                 } catch (IOException ex) {
@@ -84,10 +97,8 @@ public class InstanceIDBridge {
                 }
             }
             @Override
-            protected void onPostExecute(String token) {
-                if (mNativeInstanceIDAndroid != 0) {
-                    nativeDidGetToken(mNativeInstanceIDAndroid, requestId, token);
-                }
+            protected void sendResultToNative(String token) {
+                nativeDidGetToken(mNativeInstanceIDAndroid, requestId, token);
             }
         }.execute();
     }
@@ -96,9 +107,9 @@ public class InstanceIDBridge {
     @CalledByNative
     private void deleteToken(
             final int requestId, final String authorizedEntity, final String scope) {
-        new AsyncTask<Void, Void, Boolean>() {
+        new BridgeAsyncTask<Boolean>() {
             @Override
-            protected Boolean doInBackground(Void... params) {
+            protected Boolean doBackgroundWork() {
                 try {
                     mInstanceID.deleteToken(authorizedEntity, scope);
                     return true;
@@ -107,10 +118,8 @@ public class InstanceIDBridge {
                 }
             }
             @Override
-            protected void onPostExecute(Boolean success) {
-                if (mNativeInstanceIDAndroid != 0) {
-                    nativeDidDeleteToken(mNativeInstanceIDAndroid, requestId, success);
-                }
+            protected void sendResultToNative(Boolean success) {
+                nativeDidDeleteToken(mNativeInstanceIDAndroid, requestId, success);
             }
         }.execute();
     }
@@ -118,9 +127,9 @@ public class InstanceIDBridge {
     /** Async wrapper for {@link InstanceID#deleteInstanceID}. */
     @CalledByNative
     private void deleteInstanceID(final int requestId) {
-        new AsyncTask<Void, Void, Boolean>() {
+        new BridgeAsyncTask<Boolean>() {
             @Override
-            protected Boolean doInBackground(Void... params) {
+            protected Boolean doBackgroundWork() {
                 try {
                     mInstanceID.deleteInstanceID();
                     return true;
@@ -129,10 +138,8 @@ public class InstanceIDBridge {
                 }
             }
             @Override
-            protected void onPostExecute(Boolean success) {
-                if (mNativeInstanceIDAndroid != 0) {
-                    nativeDidDeleteID(mNativeInstanceIDAndroid, requestId, success);
-                }
+            protected void sendResultToNative(Boolean success) {
+                nativeDidDeleteID(mNativeInstanceIDAndroid, requestId, success);
             }
         }.execute();
     }
@@ -143,4 +150,40 @@ public class InstanceIDBridge {
             long nativeInstanceIDAndroid, int requestId, boolean success);
     private native void nativeDidDeleteID(
             long nativeInstanceIDAndroid, int requestId, boolean success);
+
+    /** Custom AsyncTask wrapper to workaround onPostExecute not working in tests. */

[Peter Beverloo, 2016/04/15 00:00:17]

Thank you for generalizing this!

Please give this class a proper documentation block, and be more elaborate about
why it's necessary. Right now you kind of have to go back and forth between this
line, the definition and the comment on line 174+, let's unify that.

[johnme, 2016/04/15 16:00:42]

Done.

+    private abstract class BridgeAsyncTask<Result> {
+        protected abstract Result doBackgroundWork();
+
+        protected abstract void sendResultToNative(Result result);
+
+        public void execute() {
+            AsyncTask<Void, Void, Result> task = new AsyncTask<Void, Void, Result>() {
+                @Override
+                protected Result doInBackground(Void... params) {
+                    return doBackgroundWork();
+                }
+                @Override
+                protected void onPostExecute(Result result) {
+                    if (!sBlockOnAsyncTasksForTesting && mNativeInstanceIDAndroid != 0) {
+                        sendResultToNative(result);
+                    }
+                }
+            };
+            task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);

[Peter Beverloo, 2016/04/15 00:00:17]

What's wrong with .execute()? The documentation says we probably don't want to
use this.

[johnme, 2016/04/15 16:00:42]

The default SERIAL_EXECUTOR is global to the app's process. So all AsyncTasks in
Chrome using the default SERIAL_EXECUTOR would be blocked waiting for our
network requests to complete. Using the THREAD_POOL_EXECUTOR avoids that; it
does however mean that the tasks in this file can all execute in parallel, which
should be fine (both InstanceID and my wrapper are careful to synchronize on
shared state), though I grant that there's an increased possibility of subtle
bugs. A 3rd alternative would be for this class to have its own
Executor.newSingleThreadExecutor(), so these tasks are executed sequentially
with respect to each other, but in parallel to other Clank tasks; but I'd much
rather use an existing thread pool than spin up new threads just for IID.

+            // Normally onPostExecute will be run once doInBackground completes. But during tests
+            // (other than content_browsertests and layout tests) onPostExecute never runs because
+            // C++ is blocking the UI thread (and the Java message loop isn't nested). In such tests
+            // we use AsyncTask.get() to synchronously block until doInBackground completes.
+            if (sBlockOnAsyncTasksForTesting) {
+                Result result;
+                try {
+                    result = task.get();
+                } catch (InterruptedException | ExecutionException e) {
+                    throw new IllegalStateException(e); // Shouldn't happen in tests :-p

[Peter Beverloo, 2016/04/15 00:00:17]

No smileys in comments please :-)

[johnme, 2016/04/15 16:00:42]

Spoil sport :-p

+                }
+                sendResultToNative(result);
+            }
+        }
+    }
 }