[sync] Divorce DataTypeManager from NotificationService notifications by creating a new DataTypeMa…

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2012 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/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 35 matching lines @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/net/gaia/gaia_constants.h"
 #include "chrome/common/time_format.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "grit/generated_resources.h"
 #include "net/cookies/cookie_monster.h"
 #include "sync/api/sync_error.h"
 #include "sync/internal_api/public/configure_reason.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/internal_api/public/util/sync_string_conversions.h"
 #include "sync/js/js_arg_list.h"
 #include "sync/js/js_event_details.h"
@@ skipping 419 matching lines @@
   // change from a native model.  In that case, it will get applied to the sync
   // database (which doesn't get destroyed until we destroy the backend below)
   // as an unsynced change.  That will be persisted, and committed on restart.
   if (data_type_manager_.get()) {
     if (data_type_manager_->state() != DataTypeManager::STOPPED) {
       // When aborting as part of shutdown, we should expect an aborted sync
       // configure result, else we'll dcheck when we try to read the sync error.
       expect_sync_configuration_aborted_ = true;
       data_type_manager_->Stop();
     }
-
-    registrar_.Remove(
-        this,
-        chrome::NOTIFICATION_SYNC_CONFIGURE_START,
-        content::Source<DataTypeManager>(data_type_manager_.get()));
-    registrar_.Remove(
-        this,
-        chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
-        content::Source<DataTypeManager>(data_type_manager_.get()));
-    registrar_.Remove(
-        this,
-        chrome::NOTIFICATION_SYNC_CONFIGURE_BLOCKED,
-        content::Source<DataTypeManager>(data_type_manager_.get()));
+    if (data_type_manager_->HasObserver(this))

[tim (not reviewing), 2012/08/16 18:25:20]

I think if data_type_manager_.get(), then this if could be a DCHECK?

[Raghu Simha, 2012/08/17 01:56:42]

Moot. We no longer add / remove observers from here.

+      data_type_manager_->RemoveObserver(this);
     data_type_manager_.reset();
   }
 
   // Shutdown the migrator before the backend to ensure it doesn't pull a null
   // snapshot.
   migrator_.reset();
   sync_js_controller_.AttachJsBackend(WeakHandle<syncer::JsBackend>());
 
   // Move aside the backend so nobody else tries to use it while we are
   // shutting it down.
@@ skipping 475 matching lines @@
       break;
     case syncer::DISABLE_SYNC_ON_CLIENT:
       OnStopSyncingPermanently();
       break;
     default:
       NOTREACHED();
   }
   NotifyObservers();
 }
 
+void ProfileSyncService::OnConfigureBlocked() {
+  DCHECK(DataTypeManager::CONFIGURE_BLOCKED ==
+         data_type_manager_->last_configure_result().status);
+  NotifyObservers();
+}
+
+void ProfileSyncService::OnConfigureDone() {
+  // We should have cleared our cached passphrase before we get here (in
+  // OnBackendInitialized()).
+  DCHECK(cached_passphrase_.empty());
+
+  // Update start / stop times and do UMA logging if required.
+  UpdateConfigureTimes();
+
+  // Notify listeners that configuration is done.
+  content::NotificationService::current()->Notify(

[tim (not reviewing), 2012/08/16 18:25:20]

I just realized, another nice part about this new design is that the fact that
the ProfileSyncService gets the notification _first_ is explicit, whereas before
it was based on registration order.

We ran into trouble with backend migrator here before, note that it has a
special "OnConfigureDone handler". I'd like to avoid having multiple ways to do
the same thing, and avoid other classes (even within sync) being tempted to add
themselves as an observer of the DTM and potentially throw the wheels out of
alignment.  I think we can do that by passing in a single
DataTypeManagerObserver on construction and removing all the observer_list
maintenance since there'd only ever be one.  I think, (even in tests?).

[Raghu Simha, 2012/08/17 01:56:42]

True. I realised that while debugging and liked the idea.
 
I like the idea. Very much. Done. Let me know what you think of patch set 2.

+      chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
+      content::Source<ProfileSyncService>(this),
+      content::NotificationService::NoDetails());
+
+  const DataTypeManager::ConfigureResult result =
+      data_type_manager_->last_configure_result();
+  configure_status_ = result.status;
+  DVLOG(1) << "PSS OnConfigureDone called with status: "
+            << configure_status_;
+  // The possible status values:
+  //    ABORT - Configuration was aborted. This is not an error, if
+  //            initiated by user.
+  //    OK - Everything succeeded.
+  //    PARTIAL_SUCCESS - Some datatypes failed to start.
+  //    Everything else is an UnrecoverableError. So treat it as such.
+
+  // First handle the abort case.
+  if (configure_status_ == DataTypeManager::ABORTED &&
+      expect_sync_configuration_aborted_) {
+    DVLOG(0) << "ProfileSyncService::Observe Sync Configure aborted";
+    expect_sync_configuration_aborted_ = false;
+    return;
+  }
+
+  // Handle unrecoverable error.
+  if (configure_status_ != DataTypeManager::OK &&
+      configure_status_ != DataTypeManager::PARTIAL_SUCCESS) {
+    // Something catastrophic had happened. We should only have one
+    // error representing it.
+    DCHECK_EQ(result.failed_data_types.size(),
+              static_cast<unsigned int>(1));
+    syncer::SyncError error = result.failed_data_types.front();
+    DCHECK(error.IsSet());
+    std::string message =
+        "Sync configuration failed with status " +
+        DataTypeManager::ConfigureStatusToString(configure_status_) +
+        " during " + syncer::ModelTypeToString(error.type()) +
+        ": " + error.message();
+    LOG(ERROR) << "ProfileSyncService error: "
+               << message;
+    OnInternalUnrecoverableError(error.location(),
+                                 message,
+                                 true,
+                                 ERROR_REASON_CONFIGURATION_FAILURE);
+    return;
+  }
+
+  // Now handle partial success and full success.
+  MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&ProfileSyncService::OnSyncConfigureDone,
+                 weak_factory_.GetWeakPtr(), result));
+
+  // We should never get in a state where we have no encrypted datatypes
+  // enabled, and yet we still think we require a passphrase for decryption.
+  DCHECK(!(IsPassphraseRequiredForDecryption() &&
+           !IsEncryptedDatatypeEnabled()));
+
+  // This must be done before we start syncing with the server to avoid
+  // sending unencrypted data up on a first time sync.
+  if (encryption_pending_)
+    backend_->EnableEncryptEverything();
+  NotifyObservers();
+
+  if (migrator_.get() &&
+      migrator_->state() != browser_sync::BackendMigrator::IDLE) {
+    // Migration in progress.  Let the migrator know we just finished
+    // configuring something.  It will be up to the migrator to call
+    // StartSyncingWithServer() if migration is now finished.
+    migrator_->OnConfigureDone(result);
+  } else {
+    StartSyncingWithServer();
+  }
+}
+
+void ProfileSyncService::OnConfigureRetry() {
+  // We should have cleared our cached passphrase before we get here (in
+  // OnBackendInitialized()).
+  DCHECK(cached_passphrase_.empty());
+
+  DCHECK(DataTypeManager::RETRY ==
+         data_type_manager_->last_configure_result().status);
+
+  // Update start / stop times and do UMA logging if required.
+  UpdateConfigureTimes();
+
+  OnSyncConfigureRetry();
+}
+
+void ProfileSyncService::OnConfigureStart() {
+  sync_configure_start_time_ = base::Time::Now();
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_SYNC_CONFIGURE_START,
+      content::Source<ProfileSyncService>(this),
+      content::NotificationService::NoDetails());
+  NotifyObservers();
+}
+
+void ProfileSyncService::UpdateConfigureTimes() {
+  const DataTypeManager::ConfigureResult result =
+      data_type_manager_->last_configure_result();
+  if (!sync_configure_start_time_.is_null()) {
+    if (result.status == DataTypeManager::OK ||
+        result.status == DataTypeManager::PARTIAL_SUCCESS) {
+      base::Time sync_configure_stop_time = base::Time::Now();
+      base::TimeDelta delta = sync_configure_stop_time -
+          sync_configure_start_time_;
+      if (is_first_time_sync_configure_) {
+        UMA_HISTOGRAM_LONG_TIMES("Sync.ServiceInitialConfigureTime", delta);
+      } else {
+        UMA_HISTOGRAM_LONG_TIMES("Sync.ServiceSubsequentConfigureTime",
+                                  delta);
+      }
+    }
+    sync_configure_start_time_ = base::Time();
+  }
+}
+
 std::string ProfileSyncService::QuerySyncStatusSummary() {
   if (HasUnrecoverableError()) {
     return "Unrecoverable error detected";
   } else if (!backend_.get()) {
     return "Syncing not enabled";
   } else if (backend_.get() && !HasSyncSetupCompleted()) {
     return "First time sync setup incomplete";
   } else if (backend_.get() && HasSyncSetupCompleted() &&
              data_type_manager_.get() &&
              data_type_manager_->state() != DataTypeManager::CONFIGURED) {
@@ skipping 213 matching lines @@
   return backend_.get();
 }
 
 void ProfileSyncService::ConfigureDataTypeManager() {
   bool restart = false;
   if (!data_type_manager_.get()) {
     restart = true;
     data_type_manager_.reset(
         factory_->CreateDataTypeManager(backend_.get(),
                                         &data_type_controllers_));
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_SYNC_CONFIGURE_START,
-                   content::Source<DataTypeManager>(data_type_manager_.get()));
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
-                   content::Source<DataTypeManager>(data_type_manager_.get()));
-    registrar_.Add(this,
-                   chrome::NOTIFICATION_SYNC_CONFIGURE_BLOCKED,
-                   content::Source<DataTypeManager>(data_type_manager_.get()));
+    if (!data_type_manager_->HasObserver(this))
+      data_type_manager_->AddObserver(this);
 
     // We create the migrator at the same time.
     migrator_.reset(
         new browser_sync::BackendMigrator(
             profile_->GetDebugName(), GetUserShare(),
             this, data_type_manager_.get(),
             base::Bind(&ProfileSyncService::StartSyncingWithServer,
                        base::Unretained(this))));
   }
 
@@ skipping 251 matching lines @@
              IsSyncTokenAvailable()) {
     // Previously-configured sync has been re-enabled, so start sync now.
     StartUp();
   }
 }
 
 void ProfileSyncService::Observe(int type,
                                  const content::NotificationSource& source,
                                  const content::NotificationDetails& details) {
   switch (type) {
-    case chrome::NOTIFICATION_SYNC_CONFIGURE_START:
-      sync_configure_start_time_ = base::Time::Now();
-      // no break
-    case chrome::NOTIFICATION_SYNC_CONFIGURE_BLOCKED:
-      NotifyObservers();
-      break;
-    case chrome::NOTIFICATION_SYNC_CONFIGURE_DONE: {
-      // We should have cleared our cached passphrase before we get here (in
-      // OnBackendInitialized()).
-      DCHECK(cached_passphrase_.empty());
-
-      DataTypeManager::ConfigureResult* result =
-          content::Details<DataTypeManager::ConfigureResult>(details).ptr();
-
-      if (!sync_configure_start_time_.is_null()) {
-        if (result->status == DataTypeManager::OK ||
-            result->status == DataTypeManager::PARTIAL_SUCCESS) {
-          base::Time sync_configure_stop_time = base::Time::Now();
-          base::TimeDelta delta = sync_configure_stop_time -
-              sync_configure_start_time_;
-          if (is_first_time_sync_configure_) {
-            UMA_HISTOGRAM_LONG_TIMES("Sync.ServiceInitialConfigureTime", delta);
-          } else {
-            UMA_HISTOGRAM_LONG_TIMES("Sync.ServiceSubsequentConfigureTime",
-                                     delta);
-          }
-        }
-
-        sync_configure_start_time_ = base::Time();
-      }
-
-      configure_status_ = result->status;
-      DVLOG(1) << "PSS SYNC_CONFIGURE_DONE called with status: "
-               << configure_status_;
-
-      // The possible status values:
-      //    ABORT - Configuration was aborted. This is not an error, if
-      //            initiated by user.
-      //    RETRY - Configure failed but we are retrying.
-      //    OK - Everything succeeded.
-      //    PARTIAL_SUCCESS - Some datatypes failed to start.
-      //    Everything else is an UnrecoverableError. So treat it as such.
-
-      // First handle the abort case.
-      if (configure_status_ == DataTypeManager::ABORTED &&
-          expect_sync_configuration_aborted_) {
-        DVLOG(0) << "ProfileSyncService::Observe Sync Configure aborted";
-        expect_sync_configuration_aborted_ = false;
-        return;
-      }
-
-      // Handle retry case.
-      if (configure_status_ == DataTypeManager::RETRY) {
-        OnSyncConfigureRetry();
-        return;
-      }
-
-      // Handle unrecoverable error.
-      if (configure_status_ != DataTypeManager::OK &&
-          configure_status_ != DataTypeManager::PARTIAL_SUCCESS) {
-        // Something catastrophic had happened. We should only have one
-        // error representing it.
-        DCHECK_EQ(result->failed_data_types.size(),
-                  static_cast<unsigned int>(1));
-        syncer::SyncError error = result->failed_data_types.front();
-        DCHECK(error.IsSet());
-        std::string message =
-          "Sync configuration failed with status " +
-          DataTypeManager::ConfigureStatusToString(configure_status_) +
-          " during " + syncer::ModelTypeToString(error.type()) +
-          ": " + error.message();
-        LOG(ERROR) << "ProfileSyncService error: "
-                   << message;
-        OnInternalUnrecoverableError(error.location(),
-                                     message,
-                                     true,
-                                     ERROR_REASON_CONFIGURATION_FAILURE);
-        return;
-      }
-
-      // Now handle partial success and full success.
-      MessageLoop::current()->PostTask(FROM_HERE,
-          base::Bind(&ProfileSyncService::OnSyncConfigureDone,
-                     weak_factory_.GetWeakPtr(), *result));
-
-      // We should never get in a state where we have no encrypted datatypes
-      // enabled, and yet we still think we require a passphrase for decryption.
-      DCHECK(!(IsPassphraseRequiredForDecryption() &&
-               !IsEncryptedDatatypeEnabled()));
-
-      // This must be done before we start syncing with the server to avoid
-      // sending unencrypted data up on a first time sync.
-      if (encryption_pending_)
-        backend_->EnableEncryptEverything();
-      NotifyObservers();
-
-      if (migrator_.get() &&
-          migrator_->state() != browser_sync::BackendMigrator::IDLE) {
-        // Migration in progress.  Let the migrator know we just finished
-        // configuring something.  It will be up to the migrator to call
-        // StartSyncingWithServer() if migration is now finished.
-        migrator_->OnConfigureDone(*result);
-      } else {
-        StartSyncingWithServer();
-      }
-
-      break;
-    }
     case chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL: {
       const GoogleServiceSigninSuccessDetails* successful =
           content::Details<const GoogleServiceSigninSuccessDetails>(
               details).ptr();
       DCHECK(!successful->password.empty());
       if (!sync_prefs_.IsStartSuppressed()) {
         cached_passphrase_ = successful->password;
         // Try to consume the passphrase we just cached. If the sync backend
         // is not running yet, the passphrase will remain cached until the
         // backend starts up.
@@ skipping 178 matching lines @@
   // See http://stackoverflow.com/questions/6224121/is-new-this-myclass-undefined-behaviour-after-directly-calling-the-destru.
   ProfileSyncService* old_this = this;
   this->~ProfileSyncService();
   new(old_this) ProfileSyncService(
       new ProfileSyncComponentsFactoryImpl(profile,
                                            CommandLine::ForCurrentProcess()),
       profile,
       signin,
       behavior);
 }