Implement GCMDriver.java using MultiplexingGcmListener

components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java

Index: components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
diff --git a/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java b/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
index b5b6f393b1a024f7ebd86dd5c96ffa7048ab97fd..966a783b4ddb64995fa412f8ffcc2156471406b9 100644
--- a/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
+++ b/components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
@@ -5,7 +5,11 @@
 package org.chromium.components.gcm_driver;
 
 import android.content.Context;
+import android.os.AsyncTask;
 import android.os.Bundle;
+import android.util.Log;
+
+import com.google.android.gcm.GCMRegistrar;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
@@ -15,11 +19,16 @@ import java.util.ArrayList;
 import java.util.List;
 
 /**
- * An implementation of GCMDriver using Android's Java GCM APIs.
+ * This class is the Java counterpart to the C++ GCMDriverAndroid class.
+ * It uses Android's Java GCM APIs to implements GCM registration etc, and
+ * sends back GCM messages over JNI.
+ *
  * Threading model: all calls to/from C++ happen on the UI thread.
  */
 @JNINamespace("gcm")
-public final class GCMDriver {
+public class GCMDriver {
+    private static final String TAG = "GCMDriver";
+
     // The instance of GCMDriver currently owned by a C++ GCMDriverAndroid, if any.
     private static GCMDriver sInstance = null;
 
@@ -58,32 +67,83 @@ public final class GCMDriver {
     }
 
     @CalledByNative
-    private void register(String appId, String[] senderIds) {
-        // TODO(johnme): Actually try to register.
-        nativeOnRegisterFinished(mNativeGCMDriverAndroid, appId, "", false);
+    private void register(final String appId, final String[] senderIds) {
+        new AsyncTask<Void, Void, String>() {
+            @Override
+            protected String doInBackground(Void... voids) {
+                try {
+                    GCMRegistrar.checkDevice(mContext);
+                } catch (UnsupportedOperationException ex) {
+                    return ""; // Indicates failure
+                }
+                // TODO(johnme): Move checkManifest call to a test instead.
+                GCMRegistrar.checkManifest(mContext);
+                String existingRegistrationId = GCMRegistrar.getRegistrationId(mContext);
+                if (existingRegistrationId.equals("")) {
+                    // TODO(johnme): Migrate from GCMRegistrar to GoogleCloudMessaging API, both
+                    // here and elsewhere in Chromium.
+                    // TODO(johnme): Pass appId to GCM.
+                    GCMRegistrar.register(mContext, senderIds);

[nyquist, 2014/06/11 07:52:13]

I was under the impression that the registration ID should be stored to a
preference together with the current app version, and that we must
force-re-register on app upgrade. See
http://developer.android.com/google/gcm/client.html
Or are we requiring all websites to remember to do this?

[johnme, 2014/06/11 13:53:12]

I talked with costin@, and he explained that the GCM docs recommend this in
order to work around a bug in Froyo (where GCM is in /system, so can't be
updated OTA), but that since Chrome only ships to ICS and above, we don't need
to do this. Phew :-)

+                    return null; // Indicates pending result
+                } else {
+                    Log.i(TAG, "Re-using existing registration ID");
+                    return existingRegistrationId;
+                }
+            }
+            @Override
+            protected void onPostExecute(String registrationId) {
+                if (registrationId == null)
+                    return; // Wait for onRegistered to be called.

[nyquist, 2014/06/11 07:52:13]

Nit: Shouldn't this be on the line above in Java? Also; {@link
#onRegisterFinished}

[johnme, 2014/06/11 13:53:12]

Added link. See below re line above.

+                nativeOnRegisterFinished(mNativeGCMDriverAndroid, appId, registrationId,
+                                         !registrationId.isEmpty());
+            }
+        }.execute();
     }
+
     @CalledByNative
-    private void unregister(String appId) {
-        // TODO(johnme): Actually try to unregister.
-        nativeOnUnregisterFinished(mNativeGCMDriverAndroid, appId, false);
+    private void unregister(final String appId) {
+        new AsyncTask<Void, Void, Boolean>() {
+            @Override
+            protected Boolean doInBackground(Void... voids) {
+                try {
+                    GCMRegistrar.checkDevice(mContext);
+                } catch (UnsupportedOperationException ex) {
+                    return false; // Indicates failure
+                }
+                if (!GCMRegistrar.isRegistered(mContext))
+                    return true; // Indicates success

[nyquist, 2014/06/11 07:52:13]

Nit: Shouldn't this be on the line above in Java?

[johnme, 2014/06/11 13:53:12]

The style guide seems happy with end-of-line comments, and even gives rules for
spaces before them[1] and aligning them[2]. In this case, it seems clearer to
tie the (short) explanation to the (also short) code?

[1]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.2-horiz...
[2]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.6.3-horiz...

I did however notice that you're supposed to use braces for conditionals etc
even if the body is a single statement[3], so I've added braces around these
conditionals.

[3]:
http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s4.1.1-brace...

+                // TODO(johnme): Pass appId to GCM.
+                GCMRegistrar.unregister(mContext);
+                return null; // Indicates pending result
+            }
+
+            @Override
+            protected void onPostExecute(Boolean success) {
+                if (success == null)
+                    return; // Wait for onUnregistered to be called.

[nyquist, 2014/06/11 07:52:13]

Nit: Shouldn't this be on the line above in Java?

[johnme, 2014/06/11 13:53:12]

Ditto.

+                nativeOnUnregisterFinished(mNativeGCMDriverAndroid, appId, success);
+            }
+        }.execute();
     }
 
-    public static void onRegistered(String appId, String registrationId) {
+    static void onRegisterFinished(String appId, String registrationId) {
         ThreadUtils.assertOnUiThread();
+        // TODO(johnme): If this gets called, did it definitely succeed?
         // TODO(johnme): Update registrations cache?
         if (sInstance != null)
             sInstance.nativeOnRegisterFinished(sInstance.mNativeGCMDriverAndroid, appId,
                                                registrationId, true);
     }
 
-    public static void onUnregistered(String appId) {
+    static void onUnregisterFinished(String appId) {
         ThreadUtils.assertOnUiThread();
+        // TODO(johnme): If this gets called, did it definitely succeed?
         // TODO(johnme): Update registrations cache?
         if (sInstance != null)
             sInstance.nativeOnUnregisterFinished(sInstance.mNativeGCMDriverAndroid, appId, true);
     }
 
-    public static void onMessageReceived(String appId, Bundle extras) {
+    static void onMessageReceived(final String appId, final Bundle extras) {
         // TODO(johnme): Store message and redeliver later if Chrome is killed before delivery.
         ThreadUtils.assertOnUiThread();
         launchNativeThen(new Runnable() {
@@ -110,7 +170,7 @@ public final class GCMDriver {
         });
     }
 
-    public static void onMessagesDeleted(String appId) {
+    static void onMessagesDeleted(final String appId) {
         // TODO(johnme): Store event and redeliver later if Chrome is killed before delivery.
         ThreadUtils.assertOnUiThread();
         launchNativeThen(new Runnable() {