Make use of InvalidationService

chrome/browser/google_apis/drive_notification_manager.cc

Index: chrome/browser/google_apis/drive_notification_manager.cc
diff --git a/chrome/browser/google_apis/drive_notification_manager.cc b/chrome/browser/google_apis/drive_notification_manager.cc
index b9744735591c69ba1b4754f9be81e08613844475..acc0691ebe144af2703b25fdb732dec11f72c973 100644
--- a/chrome/browser/google_apis/drive_notification_manager.cc
+++ b/chrome/browser/google_apis/drive_notification_manager.cc
@@ -6,9 +6,9 @@
 
 #include "base/metrics/histogram.h"
 #include "chrome/browser/google_apis/drive_notification_observer.h"
+#include "chrome/browser/invalidation/invalidation_service.h"
+#include "chrome/browser/invalidation/invalidation_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sync/profile_sync_service.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "google/cacheinvalidation/types.pb.h"
 
 namespace google_apis {
@@ -42,15 +42,13 @@ DriveNotificationManager::~DriveNotificationManager() {}
 
 void DriveNotificationManager::Shutdown() {
   // Unregister for Drive notifications.
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (!profile_sync_service || !push_notification_registered_) {
+  invalidation::InvalidationService* invalidation_service =
+      invalidation::InvalidationServiceFactory::GetForProfile(profile_);
+  if (!invalidation_service || !push_notification_registered_) {
     return;
   }
 
-  profile_sync_service->UpdateRegisteredInvalidationIds(

[rlarocque, 2013/05/21 20:51:36]

The API expects that clients will not unregister themselves unless their feature
is being permanently disabled (eg. user signs out).  It would be a misuse of the
API to unregister at every shutdown.  

I've taken the liberty of removing this unregistration request.  I can put it
back if there's a good reason to keep it.  

[satorux1, 2013/05/22 02:24:11]

I'm a bit confused. Call to UpdateRegisteredInvalidationIds() is removed, but
call to UnregisterInvalidationHandler() remains. Do we need to keep the latter?
If we don't need the latter, Shutdown() function will be empty, which is nice.

[rlarocque, 2013/05/22 23:46:51]

I agree, it does look strange.  There's a valid reason for it, though.  

The Invalidations server (Tango) wants clients to stay registered even after
they've gone offline.  The only case where we should unregister at the server is
when we have no need to receive those notifications for the foreseeable future. 
That might be the case if the user signs out of the browser, for example.  

The registrations are managed further down the stack, inside classes that we
can't access here.  They best way to communicate to them that we want to remain
registered even though we've shut down is to *not* update the registrations
here.  

We still want to call UnregisterInvalidationHandler.  This will unregister us
with the InvalidationService, but not unregister our IDs at the Tango server. 
The InvalidationService should not maintain a pointer to this object after it is
destroyed.  I think we also DCHECK() that all clients of the InvalidationService
have called UnregisterInvalidationHandler by the time we shut down.

[satorux1, 2013/05/23 00:54:21]

Thank you for the explanation. Could you leave a shorter version as a comment
here?

[rlarocque, 2013/05/23 21:43:27]

Done.

-      this, syncer::ObjectIdSet());
-  profile_sync_service->UnregisterInvalidationHandler(this);
+  invalidation_service->UnregisterInvalidationHandler(this);
 }
 
 void DriveNotificationManager::OnInvalidatorStateChange(
@@ -76,9 +74,10 @@ void DriveNotificationManager::OnIncomingInvalidation(
 
   // TODO(dcheng): Only acknowledge the invalidation once the fetch has
   // completed. http://crbug.com/156843
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  profile_sync_service->AcknowledgeInvalidation(
+  invalidation::InvalidationService* invalidation_service =
+      invalidation::InvalidationServiceFactory::GetForProfile(profile_);
+  CHECK(invalidation_service);

[satorux1, 2013/05/22 02:24:11]

DCHECK? I think we rarely use CHECK.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[rlarocque, 2013/05/22 23:46:51]

Done.

+  invalidation_service->AcknowledgeInvalidation(
       invalidation_map.begin()->first,
       invalidation_map.begin()->second.ack_handle);
 
@@ -128,19 +127,19 @@ void DriveNotificationManager::NotifyObserversToUpdate(
 void DriveNotificationManager::RegisterDriveNotifications() {
   DCHECK(!push_notification_enabled_);
 
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (!profile_sync_service)
+  invalidation::InvalidationService* invalidation_service =
+      invalidation::InvalidationServiceFactory::GetForProfile(profile_);
+  if (!invalidation_service)
     return;
 
-  profile_sync_service->RegisterInvalidationHandler(this);
+  invalidation_service->RegisterInvalidationHandler(this);
   syncer::ObjectIdSet ids;
   ids.insert(invalidation::ObjectId(
       ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
       kDriveInvalidationObjectId));
-  profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
+  invalidation_service->UpdateRegisteredInvalidationIds(this, ids);
   push_notification_registered_ = true;
-  OnInvalidatorStateChange(profile_sync_service->GetInvalidatorState());
+  OnInvalidatorStateChange(invalidation_service->GetInvalidatorState());
 
   UMA_HISTOGRAM_BOOLEAN("Drive.PushNotificationRegistered",
                         push_notification_registered_);