[Sync] Refactor non-frontend DTC to handle new API properly.

chrome/browser/sync/glue/generic_change_processor.h

Index: chrome/browser/sync/glue/generic_change_processor.h
diff --git a/chrome/browser/sync/glue/generic_change_processor.h b/chrome/browser/sync/glue/generic_change_processor.h
index 1afe628c87340e2c320d1df0b6e9c0bb0edd5979..9d5264b157de5259446b1d29809748edf33a426d 100644
--- a/chrome/browser/sync/glue/generic_change_processor.h
+++ b/chrome/browser/sync/glue/generic_change_processor.h
@@ -9,6 +9,8 @@
 #include <vector>
 
 #include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
 #include "chrome/browser/sync/api/sync_change_processor.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 
@@ -26,13 +28,35 @@ namespace browser_sync {
 // handles all interaction with the sync api, both translating pushes from the
 // local service into transactions and receiving changes from the sync model,
 // which then get converted into SyncChange's and sent to the local service.
-class GenericChangeProcessor : public ChangeProcessor,
-                               public SyncChangeProcessor {
+//
+// As a rule, the GenericChangeProcessor is not thread safe. It does support
+// being created on an arbitrary thread, as long as Connect(..) is called on
+// the thread the datatype resides. In addition, Disconnect() may be called
+// from any thread.
+class GenericChangeProcessor
+    : public ChangeProcessor,
+      public SyncChangeProcessor,
+      public base::RefCountedThreadSafe<GenericChangeProcessor> {

[akalin, 2011/10/06 05:33:29]

Okay, as you probably expected, I don't like this very much.  My main problem is
that it complicates the GenericChangeProcessor quite a bit for the sake of the
non-UI data types.

Here's my idea:

- Keep the GenericChangeProcessor like it was before, and in fact make it
explicitly non-thread-safe (since it lives on the native thread)
- Make the SyncableService own its SyncChangeProcessor, as we discussed (and
temporarily have the DTC own it, like in this CL)
- For UI data types, pass the GenericChangeProcessor directly to the
SyncableService/DTC as the SyncChangeProcessor.  Everything works out since
everything is on the UI thread.
- For non-UI data types, make a ref-counted wrapper, say,
NonFrontendGenericChangeProcessorWrapper (ugh), that owns the
GenericChangeProcessor, and has the same Connect/Disconnect functionality as you
added here.
- Then, wrap NFGCPW with DataTypeChangeProcessor (maybe rename to
NonFrontendSyncChangeProcessor) and pass that to the non-UI data type.

What do you think?

[Nicolas Zea, 2011/10/06 22:10:54]

Done.

  public:
-  GenericChangeProcessor(SyncableService* local_service,
-                         UnrecoverableErrorHandler* error_handler,
+  // Create an uninitialized change processor (to be later connected).
+  explicit GenericChangeProcessor(UnrecoverableErrorHandler* error_handler);
+  // Create a change processor and connect it to the syncer.
+  GenericChangeProcessor(UnrecoverableErrorHandler* error_handler,
+                         SyncableService* local_service,
                          sync_api::UserShare* user_share);
-  virtual ~GenericChangeProcessor();
+
+  // Connect to the syncer and the SyncableService specified. Should be called
+  // on the same thread the datatype resides.
+  virtual void Connect(SyncableService* local_service,
+                       sync_api::UserShare* user_share);
+
+  // Disconnects from the syncer. May be called from any thread. After this, all
+  // attempts to interact with the change processor by |local_service_| are
+  // dropped and return errors. The syncer will be safe to shut down from the
+  // point of view of this datatype.
+  // Note: Once disconnected, you cannot reconnect without creating a new
+  // change processor.
+  virtual void Disconnect();
 
   // ChangeProcessor interface.
   // Build and store a list of all changes into |syncer_changes_|.
@@ -53,15 +77,21 @@ class GenericChangeProcessor : public ChangeProcessor,
                                        SyncDataList* current_sync_data);
 
   // Generic versions of AssociatorInterface methods. Called by
-  // SyncableServiceAdapter.
-  bool SyncModelHasUserCreatedNodes(syncable::ModelType type,
-                                    bool* has_nodes);
-  bool CryptoReadyIfNecessary(syncable::ModelType type);
+  // SyncableServiceAdapter or the DataTypeController.
+  virtual bool SyncModelHasUserCreatedNodes(syncable::ModelType type,
+                                            bool* has_nodes);
+  virtual bool CryptoReadyIfNecessary(syncable::ModelType type);
+
  protected:
+  friend class base::RefCountedThreadSafe<GenericChangeProcessor>;
+
+  virtual ~GenericChangeProcessor() OVERRIDE;
+
   // ChangeProcessor interface.
   virtual void StartImpl(Profile* profile) OVERRIDE;  // Not implemented.
   virtual void StopImpl() OVERRIDE;  // Not implemented.
   virtual sync_api::UserShare* share_handle() OVERRIDE;
+
  private:
   // The SyncableService this change processor will forward changes on to.
   SyncableService* local_service_;
@@ -78,6 +108,16 @@ class GenericChangeProcessor : public ChangeProcessor,
   // when it starts up). As such we can't wait until Start(_) has been called,
   // and have to keep a local pointer to the user_share.
   sync_api::UserShare* user_share_;
+
+  // Monitor lock for this object. All methods that interact with the Syncer
+  // must aquire this lock and check whether we're disconnected or not. Once
+  // disconnected, all attempted changes to or loads from the syncer return
+  // errors. This enables us to shut down the syncer without having to wait
+  // for possibly non-UI thread datatypes to complete work.
+  base::Lock monitor_lock_;
+  bool disconnected_;
+
+  DISALLOW_COPY_AND_ASSIGN(GenericChangeProcessor);
 };
 
 }  // namespace browser_sync