Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

chrome/browser/sync/glue/sync_backend_host.h

Index: chrome/browser/sync/glue/sync_backend_host.h
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index a77026b25b274d59b3df06253c4ea2ab6423ad34..9513a8e2e344f2da833a3188beba9292352e4387 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -25,6 +25,7 @@
 #include "sync/internal_api/public/engine/model_safe_worker.h"
 #include "sync/internal_api/public/sessions/sync_session_snapshot.h"
 #include "sync/internal_api/public/sync_manager.h"
+#include "sync/internal_api/public/sync_manager_factory.h"

[rlarocque, 2012/07/04 06:39:35]

Interesting... Is this part of the un-revert, or was it part of your original
patch?

[Nicolas Zea, 2012/07/09 18:40:45]

Neither, it's an addition to support testing via dependency injection.

 #include "sync/internal_api/public/util/report_unrecoverable_error_function.h"
 #include "sync/internal_api/public/util/unrecoverable_error_handler.h"
 #include "sync/internal_api/public/util/weak_handle.h"
@@ -170,6 +171,7 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
       syncer::ModelTypeSet initial_types,
       const syncer::SyncCredentials& credentials,
       bool delete_sync_data_folder,
+      syncer::SyncManagerFactory* sync_manager_factory,
       syncer::UnrecoverableErrorHandler* unrecoverable_error_handler,
       syncer::ReportUnrecoverableErrorFunction
           report_unrecoverable_error_function);
@@ -276,7 +278,7 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
   // TODO(akalin): Figure out a better way for tests to hook into
   // SyncBackendHost.
 
-  typedef base::Callback<syncer::HttpPostProviderFactory*(void)>
+  typedef base::Callback<scoped_ptr<syncer::HttpPostProviderFactory>(void)>

[rlarocque, 2012/07/04 06:39:35]

I'm pretty sure our scoped_ptr class supports this, and it seems like a good
idea.  I can't help but wonder if there's a reason why we don't see this pattern
more often.

I'm probably just being paranoid.

[Nicolas Zea, 2012/07/09 18:40:45]

It's a relatively new (past 6 months I think) addition to scoped_ptr, for just
this kind of situation. We haven't transitioned all places we pass ownership of
pointers, but we should. In this case, it wasn't clear there were two pointers
whose ownership was being passed to the sync manager, and hence I was having
memory leaks in my fake. Switching to scoped ptr solved that.

       MakeHttpBridgeFactoryFn;
 
   struct DoInitializeOptions {
@@ -292,6 +294,7 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
         const syncer::SyncCredentials& credentials,
         ChromeSyncNotificationBridge* chrome_sync_notification_bridge,
         syncer::SyncNotifierFactory* sync_notifier_factory,
+        syncer::SyncManagerFactory* sync_manager_factory,
         bool delete_sync_data_folder,
         const std::string& restored_key_for_bootstrapping,
         syncer::SyncManager::TestingMode testing_mode,
@@ -312,6 +315,7 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
     syncer::SyncCredentials credentials;
     ChromeSyncNotificationBridge* const chrome_sync_notification_bridge;
     syncer::SyncNotifierFactory* const sync_notifier_factory;
+    syncer::SyncManagerFactory* const sync_manager_factory;
     std::string lsid;
     bool delete_sync_data_folder;
     std::string restored_key_for_bootstrapping;
@@ -321,6 +325,14 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
         report_unrecoverable_error_function;
   };
 
+  // Allow derived classes to start the sync thread early.
+  // Returns true on success, false otherwise. If the thread was already
+  // running, does nothing and returns true.
+  bool StartSyncThread();
+
+  // Getter for derived classes. May be NULL if the thread is not running.
+  MessageLoop* sync_loop() const { return sync_thread_.message_loop(); }

[rlarocque, 2012/07/04 06:39:35]

This should be on several lines.

Also, are you sure it makes sense to make this function const?  It returns a
non-const pointer.

[Nicolas Zea, 2012/07/09 18:40:45]

SImple inlined methods like this are acceptable to have on a single line from
what I've seen, although I haven't seen any rule in style guide one way or the
other about it. That said, it's not the first time in this file.

Also, it's fine that it doesn't return a const pointer. The point is it doesn't
modify anything itself, and hence can be called from within a const method.
Getter's in general, whether they're for pointers or not, are typically const.

+
   // Allows tests to perform alternate core initialization work.
   virtual void InitCore(const DoInitializeOptions& options);
 
@@ -333,6 +345,13 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
       const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
       const base::Closure& retry_callback);
 
+  // InitializationComplete passes through the SyncBackendHost to forward

[rlarocque, 2012/07/04 06:39:35]

Was this move necessary?  If not, could you undo it to preserve 'git blame'
history?

[Nicolas Zea, 2012/07/09 18:40:45]

Ah, originally I was calling this from a child, but looks like not anymore.
Moved back.

+  // on to |frontend_|, and so that tests can intercept here if they need to
+  // set up initial conditions.
+  void HandleInitializationCompletedOnFrontendLoop(
+      const syncer::WeakHandle<syncer::JsBackend>& js_backend,
+      bool success);
+
   // Called when the syncer has finished performing a configuration.
   void FinishConfigureDataTypesOnFrontendLoop(
       const syncer::ModelTypeSet types_to_configure,
@@ -349,6 +368,8 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
                             // has been created.
     NOT_INITIALIZED,        // Initialization hasn't completed, but we've
                             // constructed a SyncManager.
+    CLEANING_NIGORI,        // A partially downloaded nigori was found. We
+                            // perform a configuration cycle to blow it away.
     DOWNLOADING_NIGORI,     // The SyncManager is initialized, but
                             // we're fetching encryption information.
     REFRESHING_NIGORI,      // The SyncManager is initialized, and we
@@ -366,13 +387,6 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
   // Downloading of nigori failed and will be retried.
   void OnNigoriDownloadRetry();
 
-  // InitializationComplete passes through the SyncBackendHost to forward
-  // on to |frontend_|, and so that tests can intercept here if they need to
-  // set up initial conditions.
-  void HandleInitializationCompletedOnFrontendLoop(
-      const syncer::WeakHandle<syncer::JsBackend>& js_backend,
-      bool success);
-
   // Called from Core::OnSyncCycleCompleted to handle updating frontend
   // thread components.
   void HandleSyncCycleCompletedOnFrontendLoop(