[Sync] Move change-related methods out of SyncManager::Observer

chrome/browser/sync/internal_api/sync_manager.cc

Index: chrome/browser/sync/internal_api/sync_manager.cc
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index f8c6a69b131b080c69ca63124d3cc1922ce261fa..e2f1c2f14d348d9d3c83d67dbb58bab19f3c91f4 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -8,7 +8,11 @@
 
 #include "base/base64.h"
 #include "base/command_line.h"
+#include "base/compiler_specific.h"
 #include "base/json/json_writer.h"
+#include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/string_number_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/sync/engine/all_status.h"
@@ -72,7 +76,7 @@ using browser_sync::Syncer;
 using browser_sync::WeakHandle;
 using browser_sync::sessions::SyncSessionContext;
 using syncable::DirectoryManager;
-using syncable::EntryKernelMutationMap;
+using syncable::ImmutableWriteTransactionInfo;
 using syncable::ModelType;
 using syncable::ModelTypeBitSet;
 using syncable::SPECIFICS;
@@ -125,7 +129,10 @@ class SyncManager::SyncInternal
   explicit SyncInternal(const std::string& name)
       : name_(name),
         weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+        change_observers_(
+            new ObserverListThreadSafe<SyncManager::ChangeObserver>()),
         registrar_(NULL),
+        change_delegate_(NULL),
         initialized_(false),
         setup_for_test_mode_(false),
         observing_ip_address_changes_(false) {
@@ -171,6 +178,7 @@ class SyncManager::SyncInternal
             bool use_ssl,
             HttpPostProviderFactory* post_factory,
             ModelSafeWorkerRegistrar* model_safe_worker_registrar,
+            ChangeDelegate* change_delegate,
             const std::string& user_agent,
             const SyncCredentials& credentials,
             sync_notifier::SyncNotifier* sync_notifier,
@@ -224,15 +232,16 @@ class SyncManager::SyncInternal
   // builds the list of sync-engine initiated changes that will be forwarded to
   // the SyncManager's Observers.
   virtual void HandleTransactionCompleteChangeEvent(
-      const ModelTypeBitSet& models_with_changes);
+      const ModelTypeBitSet& models_with_changes) OVERRIDE;
   virtual ModelTypeBitSet HandleTransactionEndingChangeEvent(
-      syncable::BaseTransaction* trans);
+      const ImmutableWriteTransactionInfo& write_transaction_info,
+      syncable::BaseTransaction* trans) OVERRIDE;
   virtual void HandleCalculateChangesChangeEventFromSyncApi(
-      const EntryKernelMutationMap& mutations,
-      syncable::BaseTransaction* trans);
+      const ImmutableWriteTransactionInfo& write_transaction_info,
+      syncable::BaseTransaction* trans) OVERRIDE;
   virtual void HandleCalculateChangesChangeEventFromSyncer(
-      const EntryKernelMutationMap& mutations,
-      syncable::BaseTransaction* trans);
+      const ImmutableWriteTransactionInfo& write_transaction_info,
+      syncable::BaseTransaction* trans) OVERRIDE;
 
   // Listens for notifications from the ServerConnectionManager
   void HandleServerConnectionEvent(const ServerConnectionEvent& event);
@@ -242,16 +251,16 @@ class SyncManager::SyncInternal
 
   // SyncNotifierObserver implementation.
   virtual void OnNotificationStateChange(
-      bool notifications_enabled);
+      bool notifications_enabled) OVERRIDE;
 
   virtual void OnIncomingNotification(
-      const syncable::ModelTypePayloadMap& type_payloads);
+      const syncable::ModelTypePayloadMap& type_payloads) OVERRIDE;
+
+  virtual void StoreState(const std::string& cookie) OVERRIDE;
 
-  virtual void StoreState(const std::string& cookie);
+  void AddChangeObserver(SyncManager::ChangeObserver* observer);
+  void RemoveChangeObserver(SyncManager::ChangeObserver* observer);
 
-  // Thread-safe observers_ accessors.
-  void CopyObservers(ObserverList<SyncManager::Observer>* observers_copy);
-  bool HaveObservers() const;
   void AddObserver(SyncManager::Observer* observer);
   void RemoveObserver(SyncManager::Observer* observer);
 
@@ -297,7 +306,7 @@ class SyncManager::SyncInternal
                                 bool exists_now);
 
   // Called only by our NetworkChangeNotifier.
-  virtual void OnIPAddressChanged();
+  virtual void OnIPAddressChanged() OVERRIDE;
 
   bool InitialSyncEndedForAllEnabledTypes() {
     syncable::ModelTypeSet types;
@@ -312,10 +321,11 @@ class SyncManager::SyncInternal
   }
 
   // SyncEngineEventListener implementation.
-  virtual void OnSyncEngineEvent(const SyncEngineEvent& event);
+  virtual void OnSyncEngineEvent(const SyncEngineEvent& event) OVERRIDE;
 
   // ServerConnectionEventListener implementation.
-  virtual void OnServerConnectionEvent(const ServerConnectionEvent& event);
+  virtual void OnServerConnectionEvent(
+      const ServerConnectionEvent& event) OVERRIDE;
 
   // JsBackend implementation.
   virtual void SetJsEventHandler(
@@ -484,9 +494,12 @@ class SyncManager::SyncInternal
   // constructing any transaction type.
   UserShare share_;
 
-  // We have to lock around every observers_ access because it can get accessed
-  // from any thread and added to/removed from on the core thread.
-  mutable base::Lock observers_lock_;
+  // Even though observers are always added/removed from the sync
+  // thread, we still need to use a thread-safe observer list as we
+  // can notify from any thread.
+  scoped_refptr<ObserverListThreadSafe<SyncManager::ChangeObserver> >
+      change_observers_;
+
   ObserverList<SyncManager::Observer> observers_;
 
   // The ServerConnectionManager used to abstract communication between the
@@ -516,6 +529,8 @@ class SyncManager::SyncInternal
   // The instance is shared between the SyncManager and the Syncer.
   ModelSafeWorkerRegistrar* registrar_;
 
+  SyncManager::ChangeDelegate* change_delegate_;
+
   // Set to true once Init has been called.
   bool initialized_;
 
@@ -539,6 +554,10 @@ class SyncManager::SyncInternal
 const int SyncManager::SyncInternal::kDefaultNudgeDelayMilliseconds = 200;
 const int SyncManager::SyncInternal::kPreferencesNudgeDelayMilliseconds = 2000;
 
+SyncManager::ChangeDelegate::~ChangeDelegate() {}
+
+SyncManager::ChangeObserver::~ChangeObserver() {}
+
 SyncManager::Observer::~Observer() {}
 
 SyncManager::SyncManager(const std::string& name)
@@ -584,11 +603,13 @@ bool SyncManager::Init(
     bool use_ssl,
     HttpPostProviderFactory* post_factory,
     ModelSafeWorkerRegistrar* registrar,
+    ChangeDelegate* change_delegate,
     const std::string& user_agent,
     const SyncCredentials& credentials,
     sync_notifier::SyncNotifier* sync_notifier,
     const std::string& restored_key_for_bootstrapping,
     bool setup_for_test_mode) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(post_factory);
   VLOG(1) << "SyncManager starting Init...";
   string server_string(sync_server_and_path);
@@ -599,6 +620,7 @@ bool SyncManager::Init(
                      use_ssl,
                      post_factory,
                      registrar,
+                     change_delegate,
                      user_agent,
                      credentials,
                      sync_notifier,
@@ -607,15 +629,18 @@ bool SyncManager::Init(
 }
 
 void SyncManager::UpdateCredentials(const SyncCredentials& credentials) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->UpdateCredentials(credentials);
 }
 
 void SyncManager::UpdateEnabledTypes() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->UpdateEnabledTypes();
 }
 
 void SyncManager::MaybeSetSyncTabsInNigoriNode(
     const syncable::ModelTypeSet enabled_types) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->MaybeSetSyncTabsInNigoriNode(enabled_types);
 }
 
@@ -624,15 +649,18 @@ bool SyncManager::InitialSyncEndedForAllEnabledTypes() {
 }
 
 void SyncManager::StartSyncingNormally() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->StartSyncingNormally();
 }
 
 void SyncManager::SetPassphrase(const std::string& passphrase,
      bool is_explicit) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->SetPassphrase(passphrase, is_explicit);
 }
 
 void SyncManager::EnableEncryptEverything() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   {
     // Update the cryptographer to know we're now encrypting everything.
     WriteTransaction trans(FROM_HERE, GetUserShare());
@@ -659,17 +687,20 @@ bool SyncManager::IsUsingExplicitPassphrase() {
 }
 
 void SyncManager::RequestCleanupDisabledTypes() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (data_->scheduler())
     data_->scheduler()->ScheduleCleanupDisabledTypes();
 }
 
 void SyncManager::RequestClearServerData() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (data_->scheduler())
     data_->scheduler()->ScheduleClearUserData();
 }
 
 void SyncManager::RequestConfig(const syncable::ModelTypeBitSet& types,
     ConfigureReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!data_->scheduler()) {
     LOG(INFO)
         << "SyncManager::RequestConfig: bailing out because scheduler is "
@@ -681,6 +712,7 @@ void SyncManager::RequestConfig(const syncable::ModelTypeBitSet& types,
 }
 
 void SyncManager::StartConfigurationMode(ModeChangeCallback* callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!data_->scheduler()) {
     LOG(INFO)
         << "SyncManager::StartConfigurationMode: could not start "
@@ -704,6 +736,7 @@ bool SyncManager::SyncInternal::Init(
     bool use_ssl,
     HttpPostProviderFactory* post_factory,
     ModelSafeWorkerRegistrar* model_safe_worker_registrar,
+    ChangeDelegate* change_delegate,
     const std::string& user_agent,
     const SyncCredentials& credentials,
     sync_notifier::SyncNotifier* sync_notifier,
@@ -718,10 +751,12 @@ bool SyncManager::SyncInternal::Init(
   weak_handle_this_ = MakeWeakHandle(weak_ptr_factory_.GetWeakPtr());
 
   registrar_ = model_safe_worker_registrar;
+  change_delegate_ = change_delegate;
   setup_for_test_mode_ = setup_for_test_mode;
 
   sync_notifier_.reset(sync_notifier);
 
+  AddChangeObserver(&js_sync_manager_observer_);
   AddObserver(&js_sync_manager_observer_);
   SetJsEventHandler(event_handler);
 
@@ -775,9 +810,7 @@ bool SyncManager::SyncInternal::Init(
   // post a task to shutdown sync. But if this function posts any other tasks
   // on the UI thread and if shutdown wins then that tasks would execute on
   // a freed pointer. This is because UI thread is not shut down.
-  ObserverList<SyncManager::Observer> temp_obs_list;
-  CopyObservers(&temp_obs_list);
-  FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         WeakHandle<JsBackend>(weak_ptr_factory_.GetWeakPtr()),
                         signed_in));
@@ -822,9 +855,7 @@ bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
   sync_pb::NigoriSpecifics nigori(node.GetNigoriSpecifics());
   Cryptographer::UpdateResult result = cryptographer->Update(nigori);
   if (result == Cryptographer::NEEDS_PASSPHRASE) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnPassphraseRequired(sync_api::REASON_DECRYPTION));
   }
 
@@ -961,9 +992,7 @@ void SyncManager::SyncInternal::MaybeSetSyncTabsInNigoriNode(
 }
 
 void SyncManager::SyncInternal::RaiseAuthNeededEvent() {
-  ObserverList<SyncManager::Observer> temp_obs_list;
-  CopyObservers(&temp_obs_list);
-  FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
       OnAuthError(AuthError(AuthError::INVALID_GAIA_CREDENTIALS)));
 }
 
@@ -972,9 +1001,7 @@ void SyncManager::SyncInternal::SetPassphrase(
   // We do not accept empty passphrases.
   if (passphrase.empty()) {
     VLOG(1) << "Rejecting empty passphrase.";
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
         OnPassphraseRequired(sync_api::REASON_SET_PASSPHRASE_FAILED));
     return;
   }
@@ -1011,9 +1038,7 @@ void SyncManager::SyncInternal::SetPassphrase(
     }
 
     if (!succeeded) {
-      ObserverList<SyncManager::Observer> temp_obs_list;
-      CopyObservers(&temp_obs_list);
-      FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+      FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
           OnPassphraseRequired(sync_api::REASON_SET_PASSPHRASE_FAILED));
       return;
     }
@@ -1049,9 +1074,7 @@ void SyncManager::SyncInternal::SetPassphrase(
   VLOG(1) << "Passphrase accepted, bootstrapping encryption.";
   std::string bootstrap_token;
   cryptographer->GetBootstrapToken(&bootstrap_token);
-  ObserverList<SyncManager::Observer> temp_obs_list;
-  CopyObservers(&temp_obs_list);
-  FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnPassphraseAccepted(bootstrap_token));
 }
 
@@ -1086,11 +1109,9 @@ void SyncManager::SyncInternal::EncryptDataTypes(
   if (!cryptographer->is_ready()) {
     VLOG(1) << "Attempting to encrypt datatypes when cryptographer not "
             << "initialized, prompting for passphrase.";
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
     // TODO(zea): this isn't really decryption, but that's the only way we have
     // to prompt the user for a passsphrase. See http://crbug.com/91379.
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnPassphraseRequired(sync_api::REASON_DECRYPTION));
     return;
   }
@@ -1182,21 +1203,32 @@ void SyncManager::SyncInternal::ReEncryptEverything(WriteTransaction* trans) {
     }
   }
 
-  ObserverList<SyncManager::Observer> temp_obs_list;
-  CopyObservers(&temp_obs_list);
-  FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnEncryptionComplete(encrypted_types));
 }
 
 SyncManager::~SyncManager() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   delete data_;
 }
 
+void SyncManager::AddChangeObserver(ChangeObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  data_->AddChangeObserver(observer);
+}
+
+void SyncManager::RemoveChangeObserver(ChangeObserver* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  data_->RemoveChangeObserver(observer);
+}
+
 void SyncManager::AddObserver(Observer* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->AddObserver(observer);
 }
 
 void SyncManager::RemoveObserver(Observer* observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->RemoveObserver(observer);
 }
 
@@ -1215,6 +1247,7 @@ void SyncManager::SyncInternal::RequestEarlyExit() {
 }
 
 void SyncManager::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->Shutdown();
 }
 
@@ -1229,6 +1262,7 @@ void SyncManager::SyncInternal::Shutdown() {
   scheduler_.reset();
 
   SetJsEventHandler(WeakHandle<JsEventHandler>());
+  RemoveChangeObserver(&js_sync_manager_observer_);
   RemoveObserver(&js_sync_manager_observer_);
 
   if (sync_notifier_.get()) {
@@ -1260,6 +1294,7 @@ void SyncManager::SyncInternal::Shutdown() {
   share_.dir_manager.reset();
 
   setup_for_test_mode_ = false;
+  change_delegate_ = NULL;
   registrar_ = NULL;
 
   initialized_ = false;
@@ -1301,25 +1336,19 @@ void SyncManager::SyncInternal::OnServerConnectionEvent(
   allstatus_.HandleServerConnectionEvent(event);
   if (event.connection_code ==
       browser_sync::HttpResponse::SERVER_CONNECTION_OK) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnAuthError(AuthError::None()));
   }
 
   if (event.connection_code == browser_sync::HttpResponse::SYNC_AUTH_ERROR) {
     observing_ip_address_changes_ = false;
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
         OnAuthError(AuthError(AuthError::INVALID_GAIA_CREDENTIALS)));
   }
 
   if (event.connection_code ==
       browser_sync::HttpResponse::SYNC_SERVER_ERROR) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
         OnAuthError(AuthError(AuthError::CONNECTION_FAILED)));
   }
 }
@@ -1329,26 +1358,27 @@ void SyncManager::SyncInternal::HandleTransactionCompleteChangeEvent(
   // This notification happens immediately after the transaction mutex is
   // released. This allows work to be performed without blocking other threads
   // from acquiring a transaction.
-  if (!HaveObservers())
+  if (!change_delegate_)
     return;
 
   // Call commit.
   for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) {
-    if (models_with_changes.test(i)) {
-      ObserverList<SyncManager::Observer> temp_obs_list;
-      CopyObservers(&temp_obs_list);
-      FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                        OnChangesComplete(syncable::ModelTypeFromInt(i)));
+    const syncable::ModelType type = syncable::ModelTypeFromInt(i);
+    if (models_with_changes.test(type)) {
+      change_delegate_->OnChangesComplete(type);
+      change_observers_->Notify(
+          &SyncManager::ChangeObserver::OnChangesComplete, type);
     }
   }
 }
 
 ModelTypeBitSet SyncManager::SyncInternal::HandleTransactionEndingChangeEvent(
+    const ImmutableWriteTransactionInfo& write_transaction_info,
     syncable::BaseTransaction* trans) {
   // This notification happens immediately before a syncable WriteTransaction
   // falls out of scope. It happens while the channel mutex is still held,
   // and while the transaction mutex is held, so it cannot be re-entrant.
-  if (!HaveObservers() || ChangeBuffersAreEmpty())
+  if (!change_delegate_ || ChangeBuffersAreEmpty())
     return ModelTypeBitSet();
 
   // This will continue the WriteTransaction using a read only wrapper.
@@ -1358,18 +1388,20 @@ ModelTypeBitSet SyncManager::SyncInternal::HandleTransactionEndingChangeEvent(
   ReadTransaction read_trans(GetUserShare(), trans);
 
   syncable::ModelTypeBitSet models_with_changes;
-  for (int i = 0; i < syncable::MODEL_TYPE_COUNT; ++i) {
-    if (change_buffers_[i].IsEmpty())
+  for (int i = syncable::FIRST_REAL_MODEL_TYPE;
+       i < syncable::MODEL_TYPE_COUNT; ++i) {
+    const syncable::ModelType type = syncable::ModelTypeFromInt(i);
+    if (change_buffers_[type].IsEmpty())
       continue;
 
     ImmutableChangeRecordList ordered_changes =
-        change_buffers_[i].GetAllChangesInTreeOrder(&read_trans);
+        change_buffers_[type].GetAllChangesInTreeOrder(&read_trans);
     if (!ordered_changes.Get().empty()) {
-      ObserverList<SyncManager::Observer> temp_obs_list;
-      CopyObservers(&temp_obs_list);
-      FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                        OnChangesApplied(syncable::ModelTypeFromInt(i),
-                                         &read_trans, ordered_changes));
+      change_delegate_->
+          OnChangesApplied(type, &read_trans, ordered_changes);
+      change_observers_->Notify(
+          &SyncManager::ChangeObserver::OnChangesApplied,
+          type, write_transaction_info.Get().id, ordered_changes);
       models_with_changes.set(i, true);
     }
     change_buffers_[i].Clear();
@@ -1378,7 +1410,7 @@ ModelTypeBitSet SyncManager::SyncInternal::HandleTransactionEndingChangeEvent(
 }
 
 void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi(
-    const EntryKernelMutationMap& mutations,
+    const ImmutableWriteTransactionInfo& write_transaction_info,
     syncable::BaseTransaction* trans) {
   if (!scheduler()) {
     return;
@@ -1393,8 +1425,10 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncApi(
 
   // Find the first real mutation.  We assume that only a single model
   // type is mutated per transaction.
+  const syncable::ImmutableEntryKernelMutationMap& mutations =
+      write_transaction_info.Get().mutations;
   for (syncable::EntryKernelMutationMap::const_iterator it =
-           mutations.begin(); it != mutations.end(); ++it) {
+           mutations.Get().begin(); it != mutations.Get().end(); ++it) {
     if (!it->second.mutated.ref(syncable::IS_UNSYNCED)) {
       continue;
     }
@@ -1458,7 +1492,7 @@ void SyncManager::SyncInternal::SetExtraChangeRecordData(int64 id,
 }
 
 void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncer(
-    const EntryKernelMutationMap& mutations,
+    const ImmutableWriteTransactionInfo& write_transaction_info,
     syncable::BaseTransaction* trans) {
   // We only expect one notification per sync step, so change_buffers_ should
   // contain no pending entries.
@@ -1466,8 +1500,10 @@ void SyncManager::SyncInternal::HandleCalculateChangesChangeEventFromSyncer(
       "CALCULATE_CHANGES called with unapplied old changes.";
 
   Cryptographer* crypto = dir_manager()->GetCryptographer(trans);
+  const syncable::ImmutableEntryKernelMutationMap& mutations =
+      write_transaction_info.Get().mutations;
   for (syncable::EntryKernelMutationMap::const_iterator it =
-           mutations.begin(); it != mutations.end(); ++it) {
+           mutations.Get().begin(); it != mutations.Get().end(); ++it) {
     bool existed_before = !it->second.original.ref(syncable::IS_DEL);
     bool exists_now = !it->second.mutated.ref(syncable::IS_DEL);
 
@@ -1538,12 +1574,6 @@ void SyncManager::SyncInternal::RequestNudgeForDataType(
 void SyncManager::SyncInternal::OnSyncEngineEvent(
     const SyncEngineEvent& event) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!HaveObservers()) {
-    LOG(INFO)
-        << "OnSyncEngineEvent returning because observers_.size() is zero";
-    return;
-  }
-
   // Only send an event if this is due to a cycle ending and this cycle
   // concludes a canonical "sync" process; that is, based on what is known
   // locally we are "all happy" and up-to-date.  There may be new changes on
@@ -1563,17 +1593,13 @@ void SyncManager::SyncInternal::OnSyncEngineEvent(
       // yet, prompt the user for a passphrase.
       if (cryptographer->has_pending_keys()) {
         VLOG(1) << "OnPassPhraseRequired Sent";
-        ObserverList<SyncManager::Observer> temp_obs_list;
-        CopyObservers(&temp_obs_list);
-        FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+        FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                           OnPassphraseRequired(sync_api::REASON_DECRYPTION));
       } else if (!cryptographer->is_ready() &&
                  event.snapshot->initial_sync_ended.test(syncable::NIGORI)) {
         VLOG(1) << "OnPassphraseRequired sent because cryptographer is not "
                 << "ready";
-        ObserverList<SyncManager::Observer> temp_obs_list;
-        CopyObservers(&temp_obs_list);
-        FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+        FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                           OnPassphraseRequired(sync_api::REASON_ENCRYPTION));
       }
 
@@ -1595,9 +1621,7 @@ void SyncManager::SyncInternal::OnSyncEngineEvent(
 
     if (!event.snapshot->has_more_to_sync) {
       VLOG(1) << "OnSyncCycleCompleted sent";
-      ObserverList<SyncManager::Observer> temp_obs_list;
-      CopyObservers(&temp_obs_list);
-      FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+      FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                         OnSyncCycleCompleted(event.snapshot));
     }
 
@@ -1620,41 +1644,31 @@ void SyncManager::SyncInternal::OnSyncEngineEvent(
   }
 
   if (event.what_happened == SyncEngineEvent::STOP_SYNCING_PERMANENTLY) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnStopSyncingPermanently());
     return;
   }
 
   if (event.what_happened == SyncEngineEvent::CLEAR_SERVER_DATA_SUCCEEDED) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnClearServerDataSucceeded());
     return;
   }
 
   if (event.what_happened == SyncEngineEvent::CLEAR_SERVER_DATA_FAILED) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnClearServerDataFailed());
     return;
   }
 
   if (event.what_happened == SyncEngineEvent::UPDATED_TOKEN) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnUpdatedToken(event.updated_token));
     return;
   }
 
   if (event.what_happened == SyncEngineEvent::ACTIONABLE_ERROR) {
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                       OnActionableError(
                           event.snapshot->errors.sync_protocol_error));
     return;
@@ -1925,37 +1939,23 @@ void SyncManager::SyncInternal::StoreState(
   lookup->SaveChanges();
 }
 
-// Note: it is possible that an observer will remove itself after we have made
-// a copy, but before the copy is consumed. This could theoretically result
-// in accessing a garbage pointer, but can only occur when an about:sync window
-// is closed in the middle of a notification.
-// See crbug.com/85481.
-void SyncManager::SyncInternal::CopyObservers(
-    ObserverList<SyncManager::Observer>* observers_copy) {
-  DCHECK_EQ(0U, observers_copy->size());
-  base::AutoLock lock(observers_lock_);
-  if (observers_.size() == 0)
-    return;
-  ObserverListBase<SyncManager::Observer>::Iterator it(observers_);
-  SyncManager::Observer* obs;
-  while ((obs = it.GetNext()) != NULL)
-    observers_copy->AddObserver(obs);
+void SyncManager::SyncInternal::AddChangeObserver(
+    SyncManager::ChangeObserver* observer) {
+  change_observers_->AddObserver(observer);
 }
 
-bool SyncManager::SyncInternal::HaveObservers() const {
-  base::AutoLock lock(observers_lock_);
-  return observers_.size() > 0;
+void SyncManager::SyncInternal::RemoveChangeObserver(
+    SyncManager::ChangeObserver* observer) {
+  change_observers_->RemoveObserver(observer);
 }
 
 void SyncManager::SyncInternal::AddObserver(
     SyncManager::Observer* observer) {
-  base::AutoLock lock(observers_lock_);
   observers_.AddObserver(observer);
 }
 
 void SyncManager::SyncInternal::RemoveObserver(
     SyncManager::Observer* observer) {
-  base::AutoLock lock(observers_lock_);
   observers_.RemoveObserver(observer);
 }
 
@@ -1967,9 +1967,8 @@ SyncManager::Status SyncManager::GetDetailedStatus() const {
   return data_->GetStatus();
 }
 
-SyncManager::SyncInternal* SyncManager::GetImpl() const { return data_; }
-
 void SyncManager::SaveChanges() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->SaveChanges();
 }
 
@@ -1987,6 +1986,7 @@ UserShare* SyncManager::GetUserShare() const {
 }
 
 void SyncManager::RefreshEncryption() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (data_->UpdateCryptographerFromNigori())
     data_->EncryptDataTypes(syncable::ModelTypeSet());
 }
@@ -2016,31 +2016,15 @@ bool SyncManager::HasUnsyncedItems() const {
   return (trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0);
 }
 
-void SyncManager::LogUnsyncedItems(int level) const {
-  std::vector<int64> unsynced_handles;
-  sync_api::ReadTransaction trans(FROM_HERE, GetUserShare());
-  trans.GetWrappedTrans()->directory()->GetUnsyncedMetaHandles(
-      trans.GetWrappedTrans(), &unsynced_handles);
-
-  for (std::vector<int64>::const_iterator it = unsynced_handles.begin();
-       it != unsynced_handles.end(); ++it) {
-    ReadNode node(&trans);
-    if (node.InitByIdLookup(*it)) {
-      scoped_ptr<DictionaryValue> value(node.GetDetailsAsValue());
-      std::string info;
-      base::JSONWriter::Write(value.get(), true, &info);
-      VLOG(level) << info;
-    }
-  }
-}
-
 void SyncManager::TriggerOnNotificationStateChangeForTest(
     bool notifications_enabled) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   data_->OnNotificationStateChange(notifications_enabled);
 }
 
 void SyncManager::TriggerOnIncomingNotificationForTest(
     const syncable::ModelTypeBitSet& model_types) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   syncable::ModelTypePayloadMap model_types_with_payloads =
       syncable::ModelTypePayloadMapFromBitSet(model_types,
           std::string());