Record client id in ready(), not reissueRegs().

BUG=172390


Review URL: https://chromiumcodereview.appspot.com/12096011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@179454 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 39 insertions, 6 deletions

diff --git a/sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java b/sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
index 2ace93c..8adbd4b 100644
--- a/sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
+++ b/sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
@@ -170,6 +170,11 @@ public class InvalidationService extends AndroidListener {
 
     @Override
     public void ready(byte[] clientId) {
+        setClientId(clientId);
+
+        // We might have accumulated some registrations to do while we were waiting for the client
+        // to become ready.
+        reissueRegistrations(clientId);
     }
 
     @Override
@@ -178,10 +183,6 @@ public class InvalidationService extends AndroidListener {
         if (!desiredRegistrations.isEmpty()) {
             register(clientId, desiredRegistrations);
         }
-        // TODO(dsmyers): [misc] reissue registrations is not guaranteed to be called by spec,
-        // although it will happen in practice. This code should be changed not to rely on it.
-        // Bug: https://code.google.com/p/chromium/issues/detail?id=172390
-        setClientId(clientId);
     }
 
     @Override
@@ -316,8 +317,7 @@ public class InvalidationService extends AndroidListener {
         prefs.commit(editContext);
 
         // If we do not have a ready invalidation client, we cannot change its registrations, so
-        // return. Later, when the client is ready, we will get a reissueRegistrations upcall and
-        // will supply the new registrations then.
+        // return. Later, when the client is ready, we will supply the new registrations.
         if (sClientId == null) {
             return;
         }
@@ -431,6 +431,12 @@ public class InvalidationService extends AndroidListener {
         return sIsClientStarted;
     }
 
+    /** Returns the notification client id, for tests. */
+    @VisibleForTesting
+    @Nullable static byte[] getClientIdForTest() {
+        return sClientId;
+    }
+
     /** Returns the client name used for the notification client. */
     private static byte[] getClientName() {
         // TODO(dsmyers): we should use the same client name as the native sync code.
diff --git a/sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java b/sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
index cfb8c0c..a66f238 100644
--- a/sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
+++ b/sync/android/javatests/src/org/chromium/sync/notifier/InvalidationServiceTest.java
@@ -127,6 +127,33 @@ public class InvalidationServiceTest extends ServiceTestCase<TestableInvalidatio
 
     @SmallTest
     @Feature({"Sync"})
+    public void testReady() {
+       /**
+        * Test plan: call ready. Verify that the service sets the client id correctly and reissues
+        * pending registrations.
+        */
+
+        // Persist some registrations.
+        InvalidationPreferences invPrefs = new InvalidationPreferences(getContext());
+        EditContext editContext = invPrefs.edit();
+        invPrefs.setSyncTypes(editContext, Lists.newArrayList("BOOKMARK", "SESSION"));
+        assertTrue(invPrefs.commit(editContext));
+
+        // Issue ready.
+        getService().ready(CLIENT_ID);
+        assertTrue(Arrays.equals(CLIENT_ID, InvalidationService.getClientIdForTest()));
+        byte[] otherCid = "otherCid".getBytes();
+        getService().ready(otherCid);
+        assertTrue(Arrays.equals(otherCid, InvalidationService.getClientIdForTest()));
+
+        // Verify registrations issued.
+        assertEquals(
+                Sets.newHashSet(ModelType.BOOKMARK.toObjectId(), ModelType.SESSION.toObjectId()),
+                Sets.newHashSet(getService().mRegistrations.get(0)));
+    }
+
+    @SmallTest
+    @Feature({"Sync"})
     public void testReissueRegistrations() {
         /*
          * Test plan: call the reissueRegistrations method of the listener with both empty and