Make use of InvalidationService

chrome/browser/google_apis/drive_notification_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/google_apis/drive_notification_manager.h"
 
 #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 {
 
 namespace {
 
 // The polling interval time is used when XMPP is disabled.
 const int kFastPollingIntervalInSecs = 60;
 
 // The polling interval time is used when XMPP is enabled.  Theoretically
@@ skipping 13 matching lines @@
       polling_timer_(true /* retain_user_task */, false /* is_repeating */),
       weak_ptr_factory_(this) {
   RegisterDriveNotifications();
   RestartPollingTimer();
 }
 
 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(
     syncer::InvalidatorState state) {
   push_notification_enabled_ = (state == syncer::INVALIDATIONS_ENABLED);
   if (push_notification_enabled_) {
     DVLOG(1) << "XMPP Notifications enabled";
   } else {
     DVLOG(1) << "XMPP Notifications disabled (state=" << state << ")";
   }
   FOR_EACH_OBSERVER(DriveNotificationObserver, observers_,
                     OnPushNotificationEnabled(push_notification_enabled_));
 }
 
 void DriveNotificationManager::OnIncomingInvalidation(
     const syncer::ObjectIdInvalidationMap& invalidation_map) {
   DVLOG(2) << "XMPP Drive Notification Received";
   DCHECK_EQ(1U, invalidation_map.size());
   const invalidation::ObjectId object_id(
       ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
       kDriveInvalidationObjectId);
   DCHECK_EQ(1U, invalidation_map.count(object_id));
 
   // 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);
 
   NotifyObserversToUpdate(NOTIFICATION_XMPP);
 }
 
 void DriveNotificationManager::AddObserver(
     DriveNotificationObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ skipping 29 matching lines @@
 
   // Note that polling_timer_ is not a repeating timer. Restarting manually
   // here is better as XMPP may be received right before the polling timer is
   // fired (i.e. we don't notify observers twice in a row).
   RestartPollingTimer();
 }
 
 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_);
 }
 
 // static
 std::string DriveNotificationManager::NotificationSourceToString(
     NotificationSource source) {
   switch (source) {
     case NOTIFICATION_XMPP:
       return "NOTIFICATION_XMPP";
     case NOTIFICATION_POLLING:
       return "NOTIFICATION_POLLING";
   }
 
   NOTREACHED();
   return "";
 }
 
 }  // namespace google_apis