Make SyncBackendRegistrar aware of loaded data

chrome/browser/sync/test_profile_sync_service.cc

Index: chrome/browser/sync/test_profile_sync_service.cc
diff --git a/chrome/browser/sync/test_profile_sync_service.cc b/chrome/browser/sync/test_profile_sync_service.cc
index 2738fbb2d65f1b0b877741f64151d64c8d858dee..f4a9d4bd6df48f5457fad4fe4139667c68b763ac 100644
--- a/chrome/browser/sync/test_profile_sync_service.cc
+++ b/chrome/browser/sync/test_profile_sync_service.cc
@@ -30,12 +30,17 @@ SyncBackendHostForProfileSyncTest::SyncBackendHostForProfileSyncTest(
     Profile* profile,
     const base::WeakPtr<SyncPrefs>& sync_prefs,
     const base::WeakPtr<InvalidatorStorage>& invalidator_storage,
+    syncer::TestIdFactory& id_factory,
+    base::Callback<void(UserShare*)>& callback,
     bool set_initial_sync_ended_on_init,
     bool synchronous_init,
     bool fail_initial_download,
     bool use_real_database)
     : browser_sync::SyncBackendHost(
         profile->GetDebugName(), profile, sync_prefs, invalidator_storage),
+      id_factory_(id_factory),
+      callback_(callback),
+      set_initial_sync_ended_on_init_(set_initial_sync_ended_on_init),
       synchronous_init_(synchronous_init),
       fail_initial_download_(fail_initial_download),
       use_real_database_(use_real_database) {}
@@ -86,6 +91,56 @@ void SyncBackendHostForProfileSyncTest::RequestConfigureSyncer(
                                          ready_task);
 }
 
+void SyncBackendHostForProfileSyncTest
+        ::HandleSyncManagerInitializationOnFrontendLoop(
+    const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success,
+    syncer::ModelTypeSet restored_types) {
+  // Here's our opportunity to pretend to do things that the SyncManager would
+  // normally do during initialization, but can't because this is a test.
+  bool send_passphrase_required = false;
+  if (success) {
+    // Set up any nodes the test wants around before model association.
+    if (!callback_.is_null()) {
+      callback_.Run(GetUserShareForTest());

[tim (not reviewing), 2012/07/20 21:00:57]

Plumbing the UserShare* around sure got a bit messy :/  Especially since long
term it would be nice to not actually require UserShare (or even expose it
further than SyncBackendHost).

The callbacks all seem to have access to the AbstractProfileSyncServiceTest*, so
it shouldn't actually be required since, if you're going to keep a
GetUserShareForTest method around, they should be able to query for it on-demand
via the TestProfileSyncService?

[rlarocque, 2012/07/20 21:17:59]

The main problem is that that no one is allowed to touch UserShare until
PSS::OnBackendInitialized() returns.  There are DCHECKs that ensure this, and
for good reason.

I consider the callbacks to be running in the SyncManager context because
they're trying to replace the SyncManager's directory initialization routines. 
In a way, each one of them is like a fragment of a MockSyncManager that no one
wanted to write the boilerplate for.  I guess that means I could give them a
pointer to the SyncManager, but I'd prefer to give them only the UserShare.

[tim (not reviewing), 2012/07/23 20:33:19]

I agree it's good to keep the DCHECKs in the general case. Note there are
DCHECKs in SyncManagerImpl::GetUserShare as well, though. I don't want to make
code access the UserShare before it's safe to do so, I'm just looking for a way
other than passing it around in this callback -- if we're calling it here and
passing it through to Run(), then it is safe to query for it instead from within
the callback Run() impl.  We could possibly even enforce that tests *only*
access it during this callback, if we really wanted to.

Alternatively, we could remove SyncManager::GetUserShare altogether and pass it
back on the OnInitializationComplete callback. That's the most extreme way to
disallow access to an uninitialized UserShare, but it would require caching the
value so that things can open transactions (though the actual value should never
change aside from null -> !null -> null).

[rlarocque, 2012/07/23 20:50:30]

The callbacks only have access to a TestProfileSyncService.  At the point of the
callback, the TestProfileSyncService is waiting for an OnBackendInitialized()
call, and (in theory) has no idea what state the backed is in.  It felt wrong to
add a getter to the TestProfileSyncService that would grab the directory at that
point.  Even if we did add such a getter, I think we'd still need to go through
the SyncBackendHost, so the SyncBackendHost::GetUserShareForTest() would still
be necessary.  
I considered that.  Although it works great for init, it doesn't work as well
during shutdown.  If we put the PSS in charge of storing a pointer to the
UserShare, it's hard (though not impossible) to guarantee there are no pointers
to it left by the time it's deleted.  

[tim (not reviewing), 2012/07/23 21:01:58]

Yeah, I think it would be too (my initial comment noted 'if you're going to keep
a GetUserShareForTest method around'). The extra plumbing in this patch though
still seems unnecessary.  If we're passing it in the callback, then we're
effectively telling the Run implementation that it's safe to use that pointer,
right?  So can we just make that explicit either in the documentation or extra
DCHECKs?

   
PSS definitely shouldn't concern itself with this, but SBH could (in theory).
Seems like it could be cleared at exactly the right time in SBH::Shutdown (so
after StopSyncingForShutdown, and after ChangeProcessors are stopped).
 

+      callback_.Reset();
+    }
+
+    // Pretend we downloaded initial updates and set initial sync ended bits
+    // if we were asked to.
+    if (set_initial_sync_ended_on_init_) {
+      UserShare* user_share = GetUserShareForTest();
+      Directory* directory = user_share->directory.get();
+
+      if (!directory->initial_sync_ended_for_type(syncer::NIGORI)) {
+        ProfileSyncServiceTestHelper::CreateRoot(
+            syncer::NIGORI, GetUserShareForTest(),
+            &id_factory_);
+
+        // A side effect of adding the NIGORI mode (normally done by the
+        // syncer) is a decryption attempt, which will fail the first time.
+        send_passphrase_required = true;
+      }
+
+      SetInitialSyncEndedForAllTypes();
+      restored_types = syncer::ModelTypeSet::All();
+    }
+  }
+
+  SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
+      js_backend, success, restored_types);
+}
+
+void SyncBackendHostForProfileSyncTest::SetInitialSyncEndedForAllTypes() {
+  UserShare* user_share = GetUserShareForTest();
+  Directory* directory = user_share->directory.get();
+
+  for (int i = syncer::FIRST_REAL_MODEL_TYPE;
+       i < syncer::MODEL_TYPE_COUNT; ++i) {
+    directory->set_initial_sync_ended_for_type(
+        syncer::ModelTypeFromInt(i), true);
+  }
+}
+
 }  // namespace browser_sync
 
 syncer::TestIdFactory* TestProfileSyncService::id_factory() {
@@ -98,13 +153,22 @@ browser_sync::SyncBackendHostForProfileSyncTest*
       ProfileSyncService::GetBackendForTest());
 }
 
+static void DoNothingCallback(syncer::UserShare*) {

[tim (not reviewing), 2012/07/20 21:00:57]

I think you can get rid of this (and more boilerplate elsewhere) and just use
base::Callback() if you can avoid plumbing UserShare*.

+}
+
+// static
+base::Callback<void(syncer::UserShare*)>
+    TestProfileSyncService::NullCallback() {
+  return base::Bind(DoNothingCallback);
+}
+
 TestProfileSyncService::TestProfileSyncService(
     ProfileSyncComponentsFactory* factory,
     Profile* profile,
     SigninManager* signin,
     ProfileSyncService::StartBehavior behavior,
     bool synchronous_backend_initialization,
-    const base::Closure& callback)
+    const base::Callback<void(UserShare*)>& callback)
     : ProfileSyncService(factory,
                          profile,
                          signin,
@@ -122,54 +186,10 @@ TestProfileSyncService::TestProfileSyncService(
 TestProfileSyncService::~TestProfileSyncService() {
 }
 
-void TestProfileSyncService::SetInitialSyncEndedForAllTypes() {
-  UserShare* user_share = GetUserShare();
-  Directory* directory = user_share->directory.get();
-
-  for (int i = syncer::FIRST_REAL_MODEL_TYPE;
-       i < syncer::MODEL_TYPE_COUNT; ++i) {
-    directory->set_initial_sync_ended_for_type(
-        syncer::ModelTypeFromInt(i), true);
-  }
-}
-
 void TestProfileSyncService::OnBackendInitialized(
     const syncer::WeakHandle<syncer::JsBackend>& backend,
     bool success) {
-  bool send_passphrase_required = false;
-  if (success) {
-    // Set this so below code can access GetUserShare().
-    backend_initialized_ = true;
-
-    // Set up any nodes the test wants around before model association.
-    if (!callback_.is_null()) {
-      callback_.Run();
-      callback_.Reset();
-    }
-
-    // Pretend we downloaded initial updates and set initial sync ended bits
-    // if we were asked to.
-    if (set_initial_sync_ended_on_init_) {
-      UserShare* user_share = GetUserShare();
-      Directory* directory = user_share->directory.get();
-
-      if (!directory->initial_sync_ended_for_type(syncer::NIGORI)) {
-        ProfileSyncServiceTestHelper::CreateRoot(
-            syncer::NIGORI, GetUserShare(),
-            id_factory());
-
-        // A side effect of adding the NIGORI mode (normally done by the
-        // syncer) is a decryption attempt, which will fail the first time.
-        send_passphrase_required = true;
-      }
-
-      SetInitialSyncEndedForAllTypes();
-    }
-  }
-
   ProfileSyncService::OnBackendInitialized(backend, success);
-  if (success && send_passphrase_required)
-    OnPassphraseRequired(syncer::REASON_DECRYPTION, sync_pb::EncryptedData());
 
   // TODO(akalin): Figure out a better way to do this.
   if (synchronous_backend_initialization_) {
@@ -206,6 +226,8 @@ void TestProfileSyncService::CreateBackend() {
       profile(),
       sync_prefs_.AsWeakPtr(),
       invalidator_storage_.AsWeakPtr(),
+      id_factory_,
+      callback_,
       set_initial_sync_ended_on_init_,
       synchronous_backend_initialization_,
       fail_initial_download_,