Implement InstanceIDAndroid using InstanceIDWithSubtype.java

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

Index: components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
diff --git a/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
index 130afadc6ed8a1108f0bcc9db51bc4103687afbd..ca3cc8f90f062a870b54f97dc3b4720f12251794 100644
--- a/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
+++ b/components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
@@ -5,9 +5,14 @@
 package org.chromium.components.gcm_driver.instance_id;
 
 import android.content.Context;
+import android.os.AsyncTask;
+import android.os.Bundle;
 
 import com.google.android.gms.iid.InstanceID;
 
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
@@ -16,17 +21,22 @@ import java.util.Map;
  * InstanceID variant that allows multiple InstanceIDs to be created, depending
  * on the provided subtype. Only for platforms-within-platforms like browsers.
  */
+@JNINamespace("instance_id")

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

I would be very much in favor to split this up in two files. One having the
InstanceIDWithSubtype class that extends InstanceID, and one that adds the JNI
and does any asynchronous operation we may have to do ourselves.

It's rather convoluted this way, and the InstanceID base class has lots of
similar methods that, in some cases, behave *slightly* differently. It would
simplify the code a lot.

[johnme, 2016/04/13 18:46:51]

Done.

 public class InstanceIDWithSubtype extends InstanceID {
+    private long mNativeInstanceIDAndroid;
     private final String mSubtype;
 
     private static Map<String, InstanceIDWithSubtype> sSubtypeInstances = new HashMap<>();
 
-    private InstanceIDWithSubtype(Context context, String subtype) {
+    private InstanceIDWithSubtype(long nativeInstanceIDAndroid, Context context, String subtype) {
         super(context, subtype, null /* options */);
+        mNativeInstanceIDAndroid = nativeInstanceIDAndroid;
         mSubtype = subtype;
     }
 
-    public static InstanceIDWithSubtype getInstance(Context context, String subtype) {
+    @CalledByNative
+    public static InstanceIDWithSubtype getInstance(
+            long nativeInstanceIDAndroid, Context context, String subtype) {
         synchronized (InstanceID.class) {
             if (subtype == null || subtype == "") {
                 throw new IllegalArgumentException("subtype must not be empty");
@@ -41,7 +51,7 @@ public class InstanceIDWithSubtype extends InstanceID {
 
             InstanceIDWithSubtype existing = sSubtypeInstances.get(subtype);
             if (existing == null) {
-                existing = new InstanceIDWithSubtype(context, subtype);
+                existing = new InstanceIDWithSubtype(nativeInstanceIDAndroid, context, subtype);
                 sSubtypeInstances.put(subtype, existing);
             }
             return existing;
@@ -62,4 +72,85 @@ public class InstanceIDWithSubtype extends InstanceID {
     public String getSubtype() {
         return mSubtype;
     }
+
+    @Override
+    @CalledByNative
+    public String getId() {
+        return super.getId();

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

Both getId() and getCreationTime() may hit SharedPreferences on first use. (They
are lazily initialized.) I think that could trigger a StrictMode violation as
well— why don't these methods have to be asynchronous?

[johnme, 2016/04/13 11:42:12]

They can indeed trigger StrictMode violations; I have a follow-on patch that
makes them async as well.

+    }
+
+    @Override
+    @CalledByNative
+    public long getCreationTime() {
+        return super.getCreationTime();
+    }
+
+    @CalledByNative
+    private void getToken(final int requestId, final String authorizedEntity, final String scope,

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

These methods need documentation, specifically because you're doing asynchronous
operations and you're not mentioning why. It may be clear to us now, but it
won't be for the next reader.

[johnme, 2016/04/13 18:46:51]

Done.

+            String[] extrasStrings) {
+        final Bundle extras = new Bundle();
+        for (int i = 1; i < extrasStrings.length; i += 2) {
+            extras.putString(extrasStrings[i - 1], extrasStrings[i]);

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

nit: why start at 1 and then use [i - 1 / i] while you could start at 0 and use
[i / i + 1]? Seems clearer.

[johnme, 2016/04/13 11:42:12]

Done (it was so the loop condition can just be i < extraStrings.length, and in
the case of an odd number of strings it'd avoid triggering
ArrayIndexOutOfBoundsException. But on second thought it's probably better to
throw if the number of strings is odd which is an invalid input; I've added an
assert as documentation too).

+        }
+        new AsyncTask<Void, Void, String>() {
+            @Override
+            protected String doInBackground(Void... params) {
+                try {
+                    return getToken(authorizedEntity, scope, extras);
+                } catch (IOException ex) {
+                    return "";

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

Why is it OK to silently drop exceptions? Shouldn't we at least write them to a
log?

[johnme, 2016/04/13 11:42:12]

IOException usually just means the device was offline. Doesn't seem worth
spamming a log?

[Peter Beverloo, 2016/04/13 13:21:59]

The user engaged in an action (subscribing for push through a website) that now
cannot complete. I'm sure that we'll get a bug about "why doesn't it work" at
some point...

[johnme, 2016/04/13 18:46:51]

I don't understand what you want. Spamming logs whenever we're offline doesn't
seem a good idea. InstanceIDAndroid::DidGetToken will return an
InstanceID::UNKNOWN_ERROR which Push will eventually handle, though perhaps a
more specific error would be useful.

+                }
+            }
+            @Override
+            protected void onPostExecute(String token) {
+                nativeDidGetToken(mNativeInstanceIDAndroid, requestId, token);

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

What guarantees that mNativeInstanceIDAndroid is alive when calling this method?

[johnme, 2016/04/13 11:42:12]

Fixed.

+            }
+        }.execute();
+    }
+
+    @CalledByNative
+    private void deleteToken(
+            final int requestId, final String authorizedEntity, final String scope) {
+        new AsyncTask<Void, Void, Boolean>() {
+            @Override
+            protected Boolean doInBackground(Void... params) {
+                try {
+                    deleteToken(authorizedEntity, scope);
+                    return true;
+                } catch (IOException ex) {
+                    return false;
+                }
+            }
+            @Override
+            protected void onPostExecute(Boolean success) {
+                nativeDidDeleteToken(mNativeInstanceIDAndroid, requestId, success);
+            }
+        }.execute();
+    }
+
+    @CalledByNative
+    private void deleteInstanceID(final int requestId) {
+        new AsyncTask<Void, Void, Boolean>() {
+            @Override
+            protected Boolean doInBackground(Void... params) {
+                try {
+                    deleteInstanceID();
+                    return true;
+                } catch (IOException ex) {
+                    return false;
+                }
+            }
+            @Override
+            protected void onPostExecute(Boolean success) {
+                nativeDidDeleteID(mNativeInstanceIDAndroid, requestId, success);
+            }
+        }.execute();
+    }
+
+    private native void nativeDidGetToken(
+            long nativeInstanceIDAndroid, int requestId, String token);
+    private native void nativeDidDeleteToken(
+            long nativeInstanceIDAndroid, int requestId, boolean success);
+    private native void nativeDidDeleteID(
+            long nativeInstanceIDAndroid, int requestId, boolean success);
 }