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

chrome/browser/sync/internal_api/sync_manager.h

Index: chrome/browser/sync/internal_api/sync_manager.h
diff --git a/chrome/browser/sync/internal_api/sync_manager.h b/chrome/browser/sync/internal_api/sync_manager.h
index ef997a4c7109d80809feb9df4815cccc67984ae7..6b2a8aec76c130f547cea9dc0457d17e331f2e8c 100644
--- a/chrome/browser/sync/internal_api/sync_manager.h
+++ b/chrome/browser/sync/internal_api/sync_manager.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback_old.h"
+#include "base/threading/thread_checker.h"
 #include "chrome/browser/sync/internal_api/change_record.h"
 #include "chrome/browser/sync/internal_api/configure_reason.h"
 #include "chrome/browser/sync/protocol/sync_protocol_error.h"
@@ -65,12 +66,15 @@ struct SyncCredentials {
   std::string sync_token;
 };
 
-// SyncManager encapsulates syncable::DirectoryManager and serves as the parent
-// of all other objects in the sync API.  SyncManager is thread-safe.  If
-// multiple threads interact with the same local sync repository (i.e. the
-// same sqlite database), they should share a single SyncManager instance.  The
-// caller should typically create one SyncManager for the lifetime of a user
-// session.
+// SyncManager encapsulates syncable::DirectoryManager and serves as
+// the parent of all other objects in the sync API.  If multiple
+// threads interact with the same local sync repository (i.e. the same
+// sqlite database), they should share a single SyncManager instance.
+// The caller should typically create one SyncManager for the lifetime
+// of a user session.
+//
+// Unless stated otherwise, all methods of SyncManager should be
+// called on the same thread.
 class SyncManager {
  public:
   // SyncInternal contains the implementation of SyncManager, while abstracting
@@ -163,17 +167,12 @@ class SyncManager {
     bool crypto_has_pending_keys;
   };
 
-  // An interface the embedding application implements to receive notifications
-  // from the SyncManager.  Register an observer via SyncManager::AddObserver.
-  // This observer is an event driven model as the events may be raised from
-  // different internal threads, and simply providing an "OnStatusChanged" type
-  // notification complicates things such as trying to determine "what changed",
-  // if different members of the Status object are modified from different
-  // threads.  This way, the event is explicit, and it is safe for the Observer
-  // to dispatch to a native thread or synchronize accordingly.
-  class Observer {
+  // An interface the embedding application implements to be notified
+  // on change events.  Note that these methods may be called on *any*
+  // thread.
+  class ChangeDelegate {
    public:
-    // Notify the observer that changes have been applied to the sync model.
+    // Notify the delegate that changes have been applied to the sync model.
     //
     // This will be invoked on the same thread as on which ApplyChanges was
     // called. |changes| is an array of size |change_count|, and contains the
@@ -216,6 +215,46 @@ class SyncManager {
     // I/O to when it no longer holds any lock).
     virtual void OnChangesComplete(syncable::ModelType model_type) = 0;
 
+   protected:
+    virtual ~ChangeDelegate();
+  };
+
+  // Like ChangeDelegate, except called only on the sync thread and
+  // not while a transaction is held.  For objects that want to know
+  // when changes happen, but don't need to process them.
+  class ChangeObserver {
+   public:
+    // Ids referred to in |changes| may or may not be in |mutations|.
+    // If they're not, that means that the node didn't actually
+    // change, but we marked them as changed for some other reason
+    // (e.g., siblings of re-ordered nodes).
+    //
+    // TODO(sync, long-term): Ideally, ChangeDelegate would also be
+    // passed |mutations| instead of a transaction that would have to
+    // be used to look up the changed nodes.  That is,
+    // ChangeDelegate::OnChangesApplied() would still be called under
+    // the transaction, but all the needed data will be passed down.
+    //
+    // Even more ideally, we would have sync semantics such that we'd
+    // be able to apply changes without being under a transaction.
+    // But that's a ways off...
+    virtual void OnChangesApplied(
+        syncable::ModelType model_type,
+        const syncable::ImmutableEntryKernelMutationMap& mutations,

[tim (not reviewing), 2011/09/20 16:04:59]

We shouldn't add new deps on syncable:: stuff here.  Adding deps rules was a
first step in formalizing this, some things ( like model_type ) still leak
through and they're called out in the deps files (for ex, see glue/DEPS)

[akalin, 2011/09/20 21:14:42]

Introduced the notion of a write transaction ID, and just use that.  Done.

+        const ImmutableChangeRecordList& changes) = 0;
+
+    virtual void OnChangesComplete(syncable::ModelType model_type) = 0;
+
+   protected:
+    virtual ~ChangeObserver();
+  };
+
+  // An interface the embedding application implements to receive
+  // notifications from the SyncManager.  Register an observer via
+  // SyncManager::AddObserver.  All methods are called only on the
+  // sync thread.
+  class Observer {
+   public:
     // A round-trip sync-cycle took place and the syncer has resolved any
     // conflicts that may have arisen.
     virtual void OnSyncCycleCompleted(
@@ -340,8 +379,7 @@ class SyncManager {
         const syncable::ModelTypeSet& encrypted_types) = 0;
 
     virtual void OnActionableError(
-        const browser_sync::SyncProtocolError& sync_protocol_error)
-            = 0;
+        const browser_sync::SyncProtocolError& sync_protocol_error) = 0;
 
    protected:
     virtual ~Observer();
@@ -376,6 +414,7 @@ class SyncManager {
             bool use_ssl,
             HttpPostProviderFactory* post_factory,
             browser_sync::ModelSafeWorkerRegistrar* registrar,
+            ChangeDelegate* change_delegate,
             const std::string& user_agent,
             const SyncCredentials& credentials,
             sync_notifier::SyncNotifier* sync_notifier,
@@ -383,13 +422,14 @@ class SyncManager {
             bool setup_for_test_mode);
 
   // Returns the username last used for a successful authentication.
-  // Returns empty if there is no such username.
+  // Returns empty if there is no such username.  May be called on any
+  // thread.
   const std::string& GetAuthenticatedUsername();
 
   // Check if the database has been populated with a full "initial" download of
   // sync items for each data type currently present in the routing info.
   // Prerequisite for calling this is that OnInitializationComplete has been
-  // called.
+  // called.  May be called from any thread.
   bool InitialSyncEndedForAllEnabledTypes();
 
   // Update tokens that we're using in Sync. Email must stay the same.
@@ -434,6 +474,10 @@ class SyncManager {
   // Request a clearing of all data on the server
   void RequestClearServerData();
 
+  // Add/remove change observers.
+  void AddChangeObserver(ChangeObserver* observer);
+  void RemoveChangeObserver(ChangeObserver* observer);
+
   // Adds a listener to be notified of sync events.
   // NOTE: It is OK (in fact, it's probably a good idea) to call this before
   // having received OnInitializationCompleted.
@@ -446,26 +490,26 @@ class SyncManager {
 
   // Status-related getters. Typically GetStatusSummary will suffice, but
   // GetDetailedSyncStatus can be useful for gathering debug-level details of
-  // the internals of the sync engine.
+  // the internals of the sync engine.  May be called on any thread.
   Status::Summary GetStatusSummary() const;
   Status GetDetailedStatus() const;
 
   // Whether or not the Nigori node is encrypted using an explicit passphrase.
+  // May be called on any thread.
   bool IsUsingExplicitPassphrase();
 
-  // Get the internal implementation for use by BaseTransaction, etc.
-  SyncInternal* GetImpl() const;
-
   // Call periodically from a database-safe thread to persist recent changes
   // to the syncapi model.
   void SaveChanges();
 
+  // Requests the syncer to stop as soon as possible.  May be called
+  // from any thread.
   void RequestEarlyExit();
 
   // Issue a final SaveChanges, close sqlite handles, and stop running threads.
-  // Must be called from the same thread that called Init().
   void Shutdown();
 
+  // May be called from any thread.
   UserShare* GetUserShare() const;
 
   // Inform the cryptographer of the most recent passphrase and set of encrypted
@@ -479,25 +523,23 @@ class SyncManager {
   // without clearing the server data.
   void EnableEncryptEverything();
 
-  // Returns true if we are currently encrypting all sync data.
+  // Returns true if we are currently encrypting all sync data.  May
+  // be called on any thread.
   bool EncryptEverythingEnabled() const;
 
   // Gets the set of encrypted types from the cryptographer
-  // Note: opens a transaction.
+  // Note: opens a transaction.  May be called from any thread.
   syncable::ModelTypeSet GetEncryptedDataTypes() const;
 
   // Reads the nigori node to determine if any experimental types should be
   // enabled.
-  // Note: opens a transaction.
+  // Note: opens a transaction.  May be called on any thread.
   bool ReceivedExperimentalTypes(syncable::ModelTypeSet* to_add) const;
 
   // Uses a read-only transaction to determine if the directory being synced has
-  // any remaining unsynced items.
+  // any remaining unsynced items.  May be called on any thread.
   bool HasUnsyncedItems() const;
 
-  // Logs the list of unsynced meta handles.
-  void LogUnsyncedItems(int level) const;
-
   // Functions used for testing.
 
   void TriggerOnNotificationStateChangeForTest(
@@ -507,6 +549,8 @@ class SyncManager {
       const syncable::ModelTypeBitSet& model_types);
 
  private:
+  base::ThreadChecker thread_checker_;
+
   // An opaque pointer to the nested private class.
   SyncInternal* data_;