Bug 1260519 - Support unsubscribing from Push API messages from a site. r?jchen draft
authorNick Alexander <nalexander@mozilla.com>
Tue, 29 Mar 2016 11:59:52 -0700
changeset 345695 ce01aa738d583a7200e9dc93ffa38dea9663779c
parent 345553 50c2007be8f8208cfca61e511617e36f97512ace
child 517243 25ea021ec77b29d457df31c4e6294d8b4db5429b
push id14144
push usernalexander@mozilla.com
push dateWed, 30 Mar 2016 04:28:26 +0000
reviewersjchen
bugs1260519
milestone48.0a1
Bug 1260519 - Support unsubscribing from Push API messages from a site. r?jchen This was implemented, but never wired up. I thought long and hard about how to unit test this, and it's quite difficult. First, we'd have to chose a layer of testing. We could unit test: * the JS <-> Java message passing; * the permission prompts <-> JS interface; * some interactions with the Service Worker interface. The first is difficult because none of our current testing emulators have Google Play Services and GCM enabled, so we'd need to allow to mock or otherwise fake the GCM registration. Then we'd need to stand up a mock autopush server (using httpd.js or the Java-side equivalent), or mock out the autopush client as well. At this point, we're testing sendMessage. This could be done, but I'd rather slide this fix in before building out quite a bit of test infrastructure. (For the record, the Java Push Service state machine is thoroughly tested with Java unit tests, so I have confidence that the unsubscribe logic works.) The second is tested via the PushWebSocket tests, which are now running on Android. That is, if permissions and the PushService are interacting badly, we should see it with the existing test suite. Since PushServiceAndroidGCM is pretty much a pass-through, there's little value to be added here. Finally, the third is also tested via the PushWebSocket tests. There's absolutely nothing GCM specific about the Service Worker interface to the PushService. So I'm left manually testing this -- and now we can unsubscribe from Push messages from sites. MozReview-Commit-ID: HiRiqasHJ27
mobile/android/base/java/org/mozilla/gecko/push/PushService.java
--- a/mobile/android/base/java/org/mozilla/gecko/push/PushService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/push/PushService.java
@@ -208,25 +208,27 @@ public class PushService implements Bund
             Log.e(LOG_TAG, "callback must not be null in " + event);
             return;
         }
 
         try {
             if ("PushServiceAndroidGCM:Configure".equals(event)) {
                 final String endpoint = message.getString("endpoint");
                 if (endpoint == null) {
-                    Log.e(LOG_TAG, "endpoint must not be null in " + event);
+                    callback.sendError("endpoint must not be null in " + event);
                     return;
                 }
                 final boolean debug = message.getBoolean("debug", false);
                 pushManager.configure(geckoProfile.getName(), endpoint, debug, System.currentTimeMillis()); // For side effects.
                 callback.sendSuccess(null);
                 return;
             }
             if ("PushServiceAndroidGCM:DumpRegistration".equals(event)) {
+                // In the future, this might be used to interrogate the Java Push Manager
+                // registration state from JavaScript.
                 callback.sendError("Not yet implemented!");
                 return;
             }
             if ("PushServiceAndroidGCM:DumpSubscriptions".equals(event)) {
                 try {
                     final Map<String, PushSubscription> result = pushManager.allSubscriptionsForProfile(geckoProfile.getName());
 
                     final JSONObject json = new JSONObject();
@@ -245,16 +247,20 @@ public class PushService implements Bund
                     callback.sendSuccess(null);
                 } catch (PushManager.ProfileNeedsConfigurationException | AutopushClientException | PushClient.LocalException | IOException e) {
                     Log.e(LOG_TAG, "Got exception in " + event, e);
                     callback.sendError("Got exception handling message [" + event + "]: " + e.toString());
                 }
                 return;
             }
             if ("PushServiceAndroidGCM:UnregisterUserAgent".equals(event)) {
+                // In the future, this might be used to tell the Java Push Manager to unregister
+                // a User Agent entirely from JavaScript.  Right now, however, everything is
+                // subscription based; there's no concept of unregistering all subscriptions
+                // simultaneously.
                 callback.sendError("Not yet implemented!");
                 return;
             }
             if ("PushServiceAndroidGCM:SubscribeChannel".equals(event)) {
                 final String service = SERVICE_WEBPUSH;
                 final JSONObject serviceData;
                 try {
                     serviceData = new JSONObject();
@@ -283,17 +289,30 @@ public class PushService implements Bund
                     Log.e(LOG_TAG, "Got exception in " + event, e);
                     callback.sendError("Got exception handling message [" + event + "]: " + e.toString());
                     return;
                 }
                 callback.sendSuccess(json);
                 return;
             }
             if ("PushServiceAndroidGCM:UnsubscribeChannel".equals(event)) {
-                callback.sendError("Not yet implemented!");
+                final String channelID = message.getString("channelID");
+                if (channelID == null) {
+                    callback.sendError("channelID must not be null in " + event);
+                    return;
+                }
+
+                // Fire and forget.  See comments in the function itself.
+                final PushSubscription pushSubscription = pushManager.unsubscribeChannel(channelID);
+                if (pushSubscription != null) {
+                    callback.sendSuccess(null);
+                    return;
+                }
+
+                callback.sendError("Could not unsubscribe from channel: " + channelID);
                 return;
             }
         } catch (GcmTokenClient.NeedsGooglePlayServicesException e) {
             // TODO: improve this.  Can we find a point where the user is *definitely* interacting
             // with the WebPush?  Perhaps we can show a dialog when interacting with the Push
             // permissions, and then be more aggressive showing this notification when we have
             // registrations and subscriptions that can't be advanced.
             callback.sendError("To handle event [" + event + "], user interaction is needed to enable Google Play Services.");