Ports XMPP Invalidation code from drive_file_sync_service in /sync_file_system...

Ports XMPP Invalidation code from drive_file_sync_service in /sync_file_system to /google_apis/drive_notification_manager so it can be shared between ChromeOS file manager and SyncFS.

Please note that to keep this patch streamlined I've only ported over the XMPP invalidation code for now and will port the polling logic in the next patch.

BUG=173339

Review URL: https://codereview.chromium.org/13866037

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

chrome/browser/google_apis/DEPS

@@ -7,11 +7,12 @@ include_rules = [
   "!chrome/browser/chromeos/login/user_manager.h",
   "!chrome/browser/profiles/profile.h",
   "!chrome/browser/profiles/profile_dependency_manager.h",
-  "!chrome/browser/profiles/profile_keyed_service.h",
   "!chrome/browser/profiles/profile_keyed_service_factory.h",
+  "!chrome/browser/profiles/profile_keyed_service.h",
   "!chrome/browser/signin/token_service_factory.h",
   "!chrome/browser/signin/token_service.h",
   "!chrome/browser/sync/profile_sync_service_factory.h",
+  "!chrome/browser/sync/profile_sync_service.h",
   "!chrome/common/chrome_notification_types.h",
   "!content/public/browser/browser_thread.h",
   "!content/public/browser/notification_details.h",

chrome/browser/google_apis/drive_notification_manager.cc

@@ -6,24 +6,65 @@
 
 #include "chrome/browser/google_apis/drive_notification_observer.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 {
 
-// TODO(calvinlo): Constants for polling and XMPP sync mechanisms go here.
+// The sync invalidation object ID for Google Drive.
+const char kDriveInvalidationObjectId[] = "CHANGELOG";
+
+// TODO(calvinlo): Constants for polling to go here.
 
 DriveNotificationManager::DriveNotificationManager(Profile* profile)
-    : profile_(profile) {
-  // TODO(calvinlo): Initialize member variables for polling and XMPP here.
+    : profile_(profile),
+      push_notification_registered_(false),
+      push_notification_enabled_(false) {
+  // TODO(calvinlo): Initialize member variables for polling here.
+  RegisterDriveNotifications();
 }
 
 DriveNotificationManager::~DriveNotificationManager() {}
 
 void DriveNotificationManager::Shutdown() {
-  // TODO(calvinlo): Unregister for Drive notifications goes here.
+  // Unregister for Drive notifications.
+  ProfileSyncService* profile_sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (!profile_sync_service || !push_notification_registered_) {
+    return;
+  }
+
+  profile_sync_service->UpdateRegisteredInvalidationIds(
+      this, syncer::ObjectIdSet());
+  profile_sync_service->UnregisterInvalidationHandler(this);
 }
 
-// TODO(calvinlo): Implementations for syncer::InvalidationHandler go here.
-// Implement OnInvalidatorStateChange() and OnIncomingInvalidation().
+void DriveNotificationManager::OnInvalidatorStateChange(
+    syncer::InvalidatorState state) {
+  SetPushNotificationEnabled(state);
+}
+
+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_);
+  CHECK(profile_sync_service);
+  profile_sync_service->AcknowledgeInvalidation(
+      invalidation_map.begin()->first,
+      invalidation_map.begin()->second.ack_handle);
+
+  NotifyObserversToUpdate();
+}
 
 void DriveNotificationManager::AddObserver(
     DriveNotificationObserver* observer) {
@@ -39,4 +80,52 @@ void DriveNotificationManager::NotifyObserversToUpdate() {
   FOR_EACH_OBSERVER(DriveNotificationObserver, observers_, CheckForUpdates());
 }
 
+// Register for Google Drive invalidation notifications through XMPP.
+void DriveNotificationManager::RegisterDriveNotifications() {
+  // Push notification registration might have already occurred if called from
+  // a different extension.
+  if (!IsDriveNotificationSupported() || push_notification_registered_)
+    return;
+
+  ProfileSyncService* profile_sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (!profile_sync_service)
+    return;
+
+  profile_sync_service->RegisterInvalidationHandler(this);
+  syncer::ObjectIdSet ids;
+  ids.insert(invalidation::ObjectId(
+      ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
+      kDriveInvalidationObjectId));
+  profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
+  push_notification_registered_ = true;
+  SetPushNotificationEnabled(profile_sync_service->GetInvalidatorState());
+}
+
+// TODO(calvinlo): Remove when all patches for http://crbug.com/173339 done.
+bool DriveNotificationManager::IsDriveNotificationSupported() {
+  // TODO(calvinlo): A invalidation ID can only be registered to one handler.
+  // Therefore ChromeOS and SyncFS cannot both use XMPP notifications until
+  // (http://crbug.com/173339) is completed.
+  // For now, disable XMPP notifications for SyncFC on ChromeOS to guarantee
+  // that ChromeOS's file manager can register itself to the invalidationID.
+
+#if defined(OS_CHROMEOS)
+  return false;
+#else
+  return true;
+#endif
+}
+
+void DriveNotificationManager::SetPushNotificationEnabled(
+    syncer::InvalidatorState state) {
+  DVLOG(1) << "SetPushNotificationEnabled() with state=" << state;
+  push_notification_enabled_ = (state == syncer::INVALIDATIONS_ENABLED);
+  if (!push_notification_enabled_)
+    return;
+
+  // Push notifications are enabled so reset polling timer.
+  //UpdatePollingDelay(kPollingDelaySecondsWithNotification);
+}
+
 }  // namespace google_apis

chrome/browser/google_apis/drive_notification_manager.h

@@ -8,6 +8,7 @@
 #include "base/observer_list.h"
 #include "chrome/browser/google_apis/drive_notification_observer.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
+#include "sync/notifier/invalidation_handler.h"
 
 class Profile;
 
@@ -20,7 +21,8 @@ namespace google_apis {
 // 2. Polling timer counts down.
 //    TODO(calvinlo): Also add in backup timer.
 class DriveNotificationManager
-    : public ProfileKeyedService {
+    : public ProfileKeyedService,
+      public syncer::InvalidationHandler {
  public:
   explicit DriveNotificationManager(Profile* profile);
   virtual ~DriveNotificationManager();
@@ -28,7 +30,11 @@ class DriveNotificationManager
   // ProfileKeyedService override.
   virtual void Shutdown() OVERRIDE;
 
-  // TODO(calvinlo): OVERRIDES for syncer::InvalidationHandler go here.
+  // syncer::InvalidationHandler implementation.
+  virtual void OnInvalidatorStateChange(
+      syncer::InvalidatorState state) OVERRIDE;
+  virtual void OnIncomingInvalidation(
+      const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
 
   void AddObserver(DriveNotificationObserver* observer);
   void RemoveObserver(DriveNotificationObserver* observer);
@@ -36,9 +42,22 @@ class DriveNotificationManager
  private:
   void NotifyObserversToUpdate();
 
+  // XMPP notification related methods.
+  void RegisterDriveNotifications();
+  bool IsDriveNotificationSupported();
+  void SetPushNotificationEnabled(syncer::InvalidatorState state);
+
   Profile* profile_;
   ObserverList<DriveNotificationObserver> observers_;
 
+  // XMPP notification related variables.
+  // True when Drive File Sync Service is registered for Drive notifications.
+  bool push_notification_registered_;
+  // True once the first drive notification is received with OK state.
+  bool push_notification_enabled_;
+
+  // TODO(calvinlo): Polling variables to go here.
+
   DISALLOW_COPY_AND_ASSIGN(DriveNotificationManager);
 };
 

chrome/browser/sync_file_system/drive_file_sync_service.cc

@@ -18,9 +18,9 @@
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/google_apis/drive_notification_manager.h"
+#include "chrome/browser/google_apis/drive_notification_manager_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 "chrome/browser/sync_file_system/conflict_resolution_policy.h"
 #include "chrome/browser/sync_file_system/drive_file_sync_client.h"
 #include "chrome/browser/sync_file_system/drive_file_sync_util.h"
@@ -31,7 +31,6 @@
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/common/constants.h"
-#include "google/cacheinvalidation/types.pb.h"
 #include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/syncable/sync_file_metadata.h"
 #include "webkit/fileapi/syncable/sync_file_type.h"
@@ -47,9 +46,6 @@ const base::FilePath::CharType kTempDirName[] = FILE_PATH_LITERAL("tmp");
 const base::FilePath::CharType kSyncFileSystemDir[] =
     FILE_PATH_LITERAL("Sync FileSystem");
 
-// The sync invalidation object ID for Google Drive.
-const char kDriveInvalidationObjectId[] = "CHANGELOG";
-
 // Incremental sync polling interval.
 // TODO(calvinlo): Improve polling algorithm dependent on whether push
 // notifications are on or off.
@@ -155,31 +151,6 @@ class DriveFileSyncService::TaskToken {
   DISALLOW_COPY_AND_ASSIGN(TaskToken);
 };
 
-void DriveFileSyncService::OnInvalidatorStateChange(
-    syncer::InvalidatorState state) {
-  SetPushNotificationEnabled(state);
-}
-
-void DriveFileSyncService::OnIncomingInvalidation(
-    const syncer::ObjectIdInvalidationMap& invalidation_map) {
-  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_);
-  CHECK(profile_sync_service);
-  profile_sync_service->AcknowledgeInvalidation(
-      invalidation_map.begin()->first,
-      invalidation_map.begin()->second.ack_handle);
-
-  may_have_unfetched_changes_ = true;
-  MaybeStartFetchChanges();
-}
-
 struct DriveFileSyncService::ProcessRemoteChangeParam {
   scoped_ptr<TaskToken> token;
   RemoteChange remote_change;
@@ -304,8 +275,6 @@ DriveFileSyncService::DriveFileSyncService(Profile* profile)
       state_(REMOTE_SERVICE_OK),
       sync_enabled_(true),
       largest_fetched_changestamp_(0),
-      push_notification_registered_(false),
-      push_notification_enabled_(false),
       polling_delay_seconds_(kMinimumPollingDelaySeconds),
       may_have_unfetched_changes_(false),
       remote_change_processor_(NULL),
@@ -338,22 +307,10 @@ DriveFileSyncService::~DriveFileSyncService() {
     sync_client_->RemoveObserver(this);
   token_.reset();
 
-  // Unregister for Drive notifications.
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (!profile_sync_service || !push_notification_registered_) {
-    return;
-  }
-
-  // TODO(calvinlo): Revisit this later in Consolidate Drive XMPP Notification
-  // and Polling Backup into one Class patch. http://crbug/173339.
-  // Original comment from Kochi about the order this is done in:
-  // Once DriveSystemService gets started / stopped at runtime, this ID needs to
-  // be unregistered *before* the handler is unregistered
-  // as ID persists across browser restarts.
-  profile_sync_service->UpdateRegisteredInvalidationIds(
-      this, syncer::ObjectIdSet());
-  profile_sync_service->UnregisterInvalidationHandler(this);
+  google_apis::DriveNotificationManager* drive_notification_manager =
+      google_apis::DriveNotificationManagerFactory::GetForProfile(profile_);
+  DCHECK(drive_notification_manager);
+  drive_notification_manager->RemoveObserver(this);
 }
 
 // static
@@ -719,8 +676,6 @@ DriveFileSyncService::DriveFileSyncService(
       state_(REMOTE_SERVICE_OK),
       sync_enabled_(true),
       largest_fetched_changestamp_(0),
-      push_notification_registered_(false),
-      push_notification_enabled_(false),
       polling_delay_seconds_(-1),
       may_have_unfetched_changes_(false),
       remote_change_processor_(NULL),
@@ -901,8 +856,12 @@ void DriveFileSyncService::DidInitializeMetadataStore(
     sync_client_->EnsureSyncRootIsNotInMyDrive(sync_root_resource_id());
 
   NotifyTaskDone(status, token.Pass());
-  RegisterDriveNotifications();
   may_have_unfetched_changes_ = true;
+
+  google_apis::DriveNotificationManager* drive_notification_manager =
+      google_apis::DriveNotificationManagerFactory::GetForProfile(profile_);
+  DCHECK(drive_notification_manager);
+  drive_notification_manager->AddObserver(this);
 }
 
 void DriveFileSyncService::UpdateRegisteredOrigins() {
@@ -1065,14 +1024,7 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
     metadata_store_->MoveBatchSyncOriginToIncremental(origin);
   }
 
-  // If this was the last batch sync origin and push_notification is enabled
-  // (indicates that we may have longer polling cycle), trigger the first
-  // incremental sync on next task cycle.
-  if (pending_batch_sync_origins_.empty() &&
-      push_notification_enabled_) {
-    may_have_unfetched_changes_ = true;
-  }
-
+  CheckForUpdates();
   NotifyTaskDone(SYNC_STATUS_OK, token.Pass());
 }
 
@@ -2154,6 +2106,8 @@ void DriveFileSyncService::MaybeStartFetchChanges() {
 }
 
 void DriveFileSyncService::CheckForUpdates() {
+  // TODO(calvinlo): Try to eliminate may_have_unfetched_changes_ variable.
+  may_have_unfetched_changes_ = true;
   MaybeStartFetchChanges();
 }
 
@@ -2315,11 +2269,6 @@ void DriveFileSyncService::UpdatePollingDelay(int64 new_delay_sec) {
     return;
   }
 
-  if (push_notification_enabled_) {
-    polling_delay_seconds_ = kPollingDelaySecondsWithNotification;
-    return;
-  }
-
   int64 old_delay = polling_delay_seconds_;
 
   // Push notifications off.
@@ -2329,52 +2278,6 @@ void DriveFileSyncService::UpdatePollingDelay(int64 new_delay_sec) {
     polling_timer_.Stop();
 }
 
-bool DriveFileSyncService::IsDriveNotificationSupported() {
-  // TODO(calvinlo): A invalidation ID can only be registered to one handler.
-  // Therefore ChromeOS and SyncFS cannot both use XMPP notifications until
-  // (http://crbug.com/173339) is completed.
-  // For now, disable XMPP notifications for SyncFC on ChromeOS to guarantee
-  // that ChromeOS's file manager can register itself to the invalidationID.
-
-#if defined(OS_CHROMEOS)
-  return false;
-#else
-  return true;
-#endif
-}
-
-// Register for Google Drive invalidation notifications through XMPP.
-void DriveFileSyncService::RegisterDriveNotifications() {
-  // Push notification registration might have already occurred if called from
-  // a different extension.
-  if (!IsDriveNotificationSupported() || push_notification_registered_)
-    return;
-
-  ProfileSyncService* profile_sync_service =
-      ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (!profile_sync_service)
-    return;
-
-  profile_sync_service->RegisterInvalidationHandler(this);
-  syncer::ObjectIdSet ids;
-  ids.insert(invalidation::ObjectId(
-      ipc::invalidation::ObjectSource::COSMO_CHANGELOG,
-      kDriveInvalidationObjectId));
-  profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
-  push_notification_registered_ = true;
-  SetPushNotificationEnabled(profile_sync_service->GetInvalidatorState());
-}
-
-void DriveFileSyncService::SetPushNotificationEnabled(
-    syncer::InvalidatorState state) {
-  push_notification_enabled_ = (state == syncer::INVALIDATIONS_ENABLED);
-  if (!push_notification_enabled_)
-    return;
-
-  // Push notifications are enabled so reset polling timer.
-  UpdatePollingDelay(kPollingDelaySecondsWithNotification);
-}
-
 void DriveFileSyncService::NotifyObserversFileStatusChanged(
     const FileSystemURL& url,
     SyncFileStatus sync_status,

chrome/browser/sync_file_system/drive_file_sync_service.h

@@ -22,7 +22,6 @@
 #include "chrome/browser/sync_file_system/drive_metadata_store.h"
 #include "chrome/browser/sync_file_system/local_change_processor.h"
 #include "chrome/browser/sync_file_system/remote_file_sync_service.h"
-#include "sync/notifier/invalidation_handler.h"
 #include "webkit/fileapi/syncable/file_change.h"
 #include "webkit/fileapi/syncable/sync_action.h"
 #include "webkit/fileapi/syncable/sync_callbacks.h"
@@ -48,7 +47,6 @@ class DriveFileSyncService
       public LocalChangeProcessor,
       public DriveFileSyncClientObserver,
       public base::NonThreadSafe,
-      public syncer::InvalidationHandler,
       public google_apis::DriveNotificationObserver {
  public:
   static const char kServiceName[];
@@ -116,12 +114,6 @@ class DriveFileSyncService
   virtual void OnAuthenticated() OVERRIDE;
   virtual void OnNetworkConnected() OVERRIDE;
 
-  // syncer::InvalidationHandler implementation.
-  virtual void OnInvalidatorStateChange(
-      syncer::InvalidatorState state) OVERRIDE;
-  virtual void OnIncomingInvalidation(
-      const syncer::ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
-
   // google_apis::DriveNotificationObserver implementation.
   virtual void CheckForUpdates() OVERRIDE;
 
@@ -439,9 +431,6 @@ class DriveFileSyncService
   void SchedulePolling();
   void OnPollingTimerFired();
   void UpdatePollingDelay(int64 new_delay_sec);
-  void RegisterDriveNotifications();
-  bool IsDriveNotificationSupported();
-  void SetPushNotificationEnabled(syncer::InvalidatorState state);
   void NotifyObserversFileStatusChanged(const fileapi::FileSystemURL& url,
                                         SyncFileStatus sync_status,
                                         SyncAction action_taken,
@@ -503,10 +492,6 @@ class DriveFileSyncService
   // NotifyTaskDone when the task finished.
   scoped_ptr<TaskToken> token_;
 
-  // True when Drive File Sync Service is registered for Drive notifications.
-  bool push_notification_registered_;
-  // True once the first drive notification is received with OK state.
-  bool push_notification_enabled_;
   // Timer to trigger fetching changes for incremental sync.
   base::OneShotTimer<DriveFileSyncService> polling_timer_;
   // If polling_delay_seconds_ is negative (<0) the timer won't start.

chrome/browser/sync_file_system/drive_file_sync_service_mock_unittest.cc

@@ -204,6 +204,14 @@ ACTION(InvokeDidApplyRemoteChange) {
       FROM_HERE, base::Bind(arg3, SYNC_STATUS_OK));
 }
 
+ACTION(InvokeGetChangeListWithEmptyChange) {
+  scoped_ptr<google_apis::ResourceList> resource_list(
+      new google_apis::ResourceList());
+  base::MessageLoopProxy::current()->PostTask(
+      FROM_HERE, base::Bind(arg1, google_apis::HTTP_SUCCESS,
+                            base::Passed(&resource_list)));
+}
+
 }  // namespace
 
 class MockRemoteServiceObserver : public RemoteFileSyncService::Observer {
@@ -626,6 +634,13 @@ class DriveFileSyncServiceMockTest : public testing::Test {
         .RetiresOnSaturation();
   }
 
+  void SetUpDriveServiceExpectCallsForEmptyRemoteChange() {
+    EXPECT_CALL(*mock_drive_service(),
+                GetChangeList(_, _))
+        .WillOnce(InvokeGetChangeListWithEmptyChange())
+        .RetiresOnSaturation();
+  }
+
   // End of mock setup helpers -----------------------------------------------
 
  private:
@@ -746,6 +761,8 @@ TEST_F(DriveFileSyncServiceMockTest, RegisterNewOrigin) {
       "chromeos/sync_file_system/listing_files_in_empty_directory.json",
       kDirectoryResourceId);
 
+  SetUpDriveServiceExpectCallsForEmptyRemoteChange();
+
   SetUpDriveSyncService(true);
   bool done = false;
   sync_service()->RegisterOriginForTrackingChanges(