Separate invalidator and sync client ID (part 2/2)

In r180907, we started saving the sync ID to a pref.  By now, many
clients will have exercised that code path and copied their sync ID
into that preference.  Prior to this commit, we never read from that
preference.  Most pre-existing clients will now have two copies of
their sync ID: one in the sync DB, and one in their preferences.

Meanwhile, in r185721 (part 1 of this set of patches), we extended the
protocol to support separate notifier and sync client IDs.  Prior to
that patch, we always used a single ID for both the invalidation client
and sync client.  In that patch, we made sure that the value of both IDs
were identical.

This commit takes advantage of that prior work to create an invalidator
ID that is separate from the sync ID.  It's important that the
invalidator client ID never change once it has been initialized, so this
commit tries to use the old client ID (which is equal to the sync ID at
the time of this migration) on existing clients.  New clients have no
such requirement, so their sync and invalidator IDs will be different.

This is implemented by using the preference (which may have been
initialized earlier, thanks to r180907) as the source of the invalidator
ID.  If the invalidator ID was not previously set, the code that
initializes the invalidator will create a new one.  This process
is entirely independent from the sync code.

The syncer will continue to report the sync client ID as before, but it
will now be provided with the invalidator ID during its construction.
It no longer has any control over this ID.

As part of this change, the many invalidator implementations have been
modified to accept a 'notifier_client_id' parameter in their
constructor.  This API change better reflects the fact that the client
ID may not be changed at runtime, and is made possible by the fact that
the notifier client ID is now available before the notifier is
constructed.

Finally, this code removes the hacks that were used to help support this
migration.  This includes the logic to forward the invalidator ID to the
preferences store when it was initialized on the sync thread, and the
code that sets the invalidator ID to be equal to the sync ID.

BUG=124142


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

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

6 files changed, 8 insertions, 5 deletions

diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index acd5394..2d71466 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -292,6 +292,8 @@ class SYNC_EXPORT SyncManager {
   // |user_agent| is a 7-bit ASCII string suitable for use as the User-Agent
   // HTTP header. Used internally when collecting stats to classify clients.
   // |invalidator| is owned and used to listen for invalidations.
+  // |invalidator_client_id| is used to unqiuely identify this client to the
+  // invalidation notification server.
   // |restored_key_for_bootstrapping| is the key used to boostrap the
   // cryptographer
   // |keystore_encryption_enabled| determines whether we enable the keystore
@@ -312,6 +314,7 @@ class SYNC_EXPORT SyncManager {
       ChangeDelegate* change_delegate,
       const SyncCredentials& credentials,
       scoped_ptr<Invalidator> invalidator,
+      const std::string& invalidator_client_id,
       const std::string& restored_key_for_bootstrapping,
       const std::string& restored_keystore_key_for_bootstrapping,
       scoped_ptr<InternalComponentsFactory> internal_components_factory,
diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h
index 2d0f9de..c03b4f5 100644
--- a/sync/internal_api/public/test/fake_sync_manager.h
+++ b/sync/internal_api/public/test/fake_sync_manager.h
@@ -81,6 +81,7 @@ class FakeSyncManager : public SyncManager {
       ChangeDelegate* change_delegate,
       const SyncCredentials& credentials,
       scoped_ptr<Invalidator> invalidator,
+      const std::string& invalidator_client_id,
       const std::string& restored_key_for_bootstrapping,
       const std::string& restored_keystore_key_for_bootstrapping,
       scoped_ptr<InternalComponentsFactory> internal_components_factory,
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index bd93a3c..017fdf5 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -340,6 +340,7 @@ void SyncManagerImpl::Init(
     SyncManager::ChangeDelegate* change_delegate,
     const SyncCredentials& credentials,
     scoped_ptr<Invalidator> invalidator,
+    const std::string& invalidator_client_id,
     const std::string& restored_key_for_bootstrapping,
     const std::string& restored_keystore_key_for_bootstrapping,
     scoped_ptr<InternalComponentsFactory> internal_components_factory,
@@ -417,16 +418,11 @@ void SyncManagerImpl::Init(
 
   std::string sync_id = directory()->cache_guid();
 
-  // TODO(rlarocque): The invalidator client ID should be independent from the
-  // sync client ID.  See crbug.com/124142.
-  const std::string invalidator_client_id = sync_id;
-
   allstatus_.SetSyncId(sync_id);
   allstatus_.SetInvalidatorClientId(invalidator_client_id);
 
   DVLOG(1) << "Setting sync client ID: " << sync_id;
   DVLOG(1) << "Setting invalidator client ID: " << invalidator_client_id;
-  invalidator_->SetUniqueId(invalidator_client_id);
 
   // Build a SyncSessionContext and store the worker in it.
   DVLOG(1) << "Sync is bringing up SyncSessionContext.";
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index d58a1f2..c0373cf 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -75,6 +75,7 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl :
       SyncManager::ChangeDelegate* change_delegate,
       const SyncCredentials& credentials,
       scoped_ptr<Invalidator> invalidator,
+      const std::string& invalidator_client_id,
       const std::string& restored_key_for_bootstrapping,
       const std::string& restored_keystore_key_for_bootstrapping,
       scoped_ptr<InternalComponentsFactory> internal_components_factory,
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index 37416c0..5e4d0ae 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -815,6 +815,7 @@ class SyncManagerTest : public testing::Test,
                        workers, &extensions_activity_monitor_, this,
                        credentials,
                        scoped_ptr<Invalidator>(fake_invalidator_),
+                       "fake_invalidator_client_id",
                        "", "",  // bootstrap tokens
                        scoped_ptr<InternalComponentsFactory>(GetFactory()),
                        &encryptor_,
diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc
index 57c6c77..09ee239 100644
--- a/sync/internal_api/test/fake_sync_manager.cc
+++ b/sync/internal_api/test/fake_sync_manager.cc
@@ -97,6 +97,7 @@ void FakeSyncManager::Init(
     ChangeDelegate* change_delegate,
     const SyncCredentials& credentials,
     scoped_ptr<Invalidator> invalidator,
+    const std::string& invalidator_client_id,
     const std::string& restored_key_for_bootstrapping,
     const std::string& restored_keystore_key_for_bootstrapping,
     scoped_ptr<InternalComponentsFactory> internal_components_factory,