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/FakeInstanceIDWithSubtype.java

Index: components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
diff --git a/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
new file mode 100644
index 0000000000000000000000000000000000000000..e5f89d18b7f92073071843f1d714048caafc96b8
--- /dev/null
+++ b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java
@@ -0,0 +1,100 @@
+// 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.components.gcm_driver.instance_id;
+
+import android.content.Context;
+import android.os.Bundle;
+import android.os.Looper;
+
+import com.google.android.gms.iid.InstanceID;
+
+import java.io.IOException;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+
+/**
+ * Fake for InstanceIDWithSubtype.

[Peter Beverloo, 2016/04/11 15:16:14]

One higher-level concern I have with this approach is that we're going to ship
the FakeInstanceIDWithSubtype with production builds. It would be much nicer if
we can store this file in //components/gcm_driver/instance_id/android/javatests/
instead (and thus have it be test-only) and perhaps give InstanceIDWithSubtype a
method to *set* the instance for a given subtype.

It would be more plumbing, but shipping this isn't great.

[johnme, 2016/04/14 18:32:42]

I've uploaded two patchsets - the first has the other review comments applied,
and the second moves FakeInstanceIDWithSubtype to javatests/ as suggested. I
agree that it's cleaner not to compile it in prod builds, but it adds 122 lines
of JNI boilerplate etc :-|

+ */
+public class FakeInstanceIDWithSubtype extends InstanceIDWithSubtype {
+    private String mId = null;
+    private long mCreationTime = 0;
+    private Map<String, String> mTokens = new HashMap<>();
+
+    public FakeInstanceIDWithSubtype(
+            long nativeInstanceIDAndroid, Context context, String subtype) {
+        super(nativeInstanceIDAndroid, context, subtype);
+    }
+
+    @Override
+    public String getId() {
+        if (mId == null) {
+            mCreationTime = System.currentTimeMillis();
+            mId = randomBase64(11 /* length */);

[Peter Beverloo, 2016/04/11 14:57:05]

This implicitly depends on getId() to be called before getCreationTime()? Is
that intentional? Can we initialize these values in the constructor instead?

[johnme, 2016/04/14 18:32:42]

The mCreationTime member is initialized to be 0 in the declaration, which is a
valid value meaning not-yet-created. In practice this gets executed at
construction time. What's the benefit of moving that into the constructor body?

+        }
+        return mId;
+    }
+
+    @Override
+    public long getCreationTime() {
+        return mCreationTime;
+    }
+
+    @Override
+    public String getToken(String authorizedEntity, String scope) throws IOException {

[Peter Beverloo, 2016/04/11 14:57:05]

When would this be called?

[johnme, 2016/04/14 18:32:42]

I faked the entire public API of InstanceID. This is the only method which
doesn't currently get exercised by InstanceIDBridge, and it's only a few lines,
so it seemed cleaner to leave it in, so any future Java code using the fake
doesn't get surprised by accidentally calling non-fake methods.

+        return getToken(authorizedEntity, scope, null);
+    }
+
+    @Override
+    public String getToken(String authorizedEntity, String scope, Bundle extras)
+            throws IOException {
+        if (Looper.getMainLooper() == Looper.myLooper())
+            throw new IOException(InstanceID.ERROR_MAIN_THREAD);

[Peter Beverloo, 2016/04/11 14:57:05]

The general flow of your code is now as follows:

  (1) InstanceIDDriverTest
  (2) InstanceIDAndroid
    (3) JNI -> InstanceIDWithSubtype.java
      (3a) JNI getId() -> :33
      (3b) JNI getCreationTime() -> :42
      (3c) JNI getToken() -> AsyncTask -> :52
      (3d) JNI deleteToken() -> AsyncTask -> :67
      (3e) JNI deleteInstanceID() -> AsyncTask -> :77

That means that all the complexity you're adding with the
NestedSystemMessageHandler effectively tests the AsyncTasks and the four lines
of logic we have in InstanceIDWithSubtype.getToken().

Since we know that the NestedSystemMessageHandler class is very much disliked
(albeit a necessary evil) and the gain is very minor, let's instead override the
JNI entry points in this FakeInstanceIDWithSubtype class and not bother with the
asynchronousity.

If you feel strongly about testing the extraStrings -> bundle conversion in
InstanceIDWithSubtype.getToken(), you can extract it to a helper method and
re-use it in the test as well.

[johnme, 2016/04/14 18:32:42]

I managed to remove the dependency on NestedSystemMessageHandler by replacing my
AsyncTasks with a new BridgeAsyncTask wrapper which uses AsyncTask.get() to
workaround the fact that onPostExecute doesn't get run without
NestedSystemMessageHandler.

+        String key = getSubtype() + ',' + authorizedEntity + ',' + scope;
+        String token = mTokens.get(key);
+        if (token == null && !"true".equals(extras.getString("fakeOffline"))) {
+            getId();
+            token = mId + ':' + randomBase64(140 /* length */);
+            mTokens.put(key, token);
+        }
+        return token;
+    }
+
+    @Override
+    public void deleteToken(String authorizedEntity, String scope) throws IOException {
+        if (Looper.getMainLooper() == Looper.myLooper())
+            throw new IOException(InstanceID.ERROR_MAIN_THREAD);
+        String key = getSubtype() + ',' + authorizedEntity + ',' + scope;
+        mTokens.remove(key);
+        // Calling deleteToken causes ID to be generated; can be observed though getCreationTime.
+        getId();
+    }
+
+    @Override
+    public void deleteInstanceID() throws IOException {
+        synchronized (InstanceID.class) {
+            getInstanceIDsForTesting().remove(getSubtype());
+            if (Looper.getMainLooper() == Looper.myLooper())
+                throw new IOException(InstanceID.ERROR_MAIN_THREAD);
+            mTokens.clear();
+            mCreationTime = 0;
+            mId = null;
+        }
+    }
+
+    private static final String BASE64URL_ALPHABET =
+            "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-_";
+    /** Returns a random (not necessarily valid) base64url encoded string. */
+    private static String randomBase64(int length) {
+        Random random = new Random();
+        StringBuilder sb = new StringBuilder(length);
+        for (int i = 0; i < length; i++) {
+            int index = random.nextInt(BASE64URL_ALPHABET.length());
+            sb.append(BASE64URL_ALPHABET.charAt(index));
+        }
+        return sb.toString();
+    }
+}