Make SyncBackendRegistrar aware of loaded data

chrome/browser/sync/test_profile_sync_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/test_profile_sync_service.h"
 
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
 #include "chrome/browser/sync/glue/data_type_controller.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
@@ skipping 12 matching lines @@
 using syncer::sessions::SyncSourceInfo;
 using syncer::UserShare;
 using syncer::syncable::Directory;
 
 namespace browser_sync {
 
 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) {}
 
 SyncBackendHostForProfileSyncTest::~SyncBackendHostForProfileSyncTest() {}
 
 namespace {
 
 scoped_ptr<syncer::HttpPostProviderFactory> MakeTestHttpBridgeFactory() {
   return scoped_ptr<syncer::HttpPostProviderFactory>(
@@ skipping 30 matching lines @@
     const base::Closure& retry_callback) {
   syncer::ModelTypeSet sync_ended;
   if (!fail_initial_download_)
     sync_ended.PutAll(types_to_config);
 
   FinishConfigureDataTypesOnFrontendLoop(types_to_config,
                                          sync_ended,
                                          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() {
   return &id_factory_;
 }
 
 browser_sync::SyncBackendHostForProfileSyncTest*
     TestProfileSyncService::GetBackendForTest() {
   return static_cast<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,
                          behavior),
       synchronous_backend_initialization_(
           synchronous_backend_initialization),
       synchronous_sync_configuration_(false),
       callback_(callback),
       set_initial_sync_ended_on_init_(true),
       fail_initial_download_(false),
       use_real_database_(false) {
   SetSyncSetupCompleted();
 }
 
 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_) {
     MessageLoop::current()->Quit();
   }
 }
 
 void TestProfileSyncService::Observe(
     int type,
     const content::NotificationSource& source,
@@ skipping 16 matching lines @@
 }
 void TestProfileSyncService::set_use_real_database() {
   use_real_database_ = true;
 }
 
 void TestProfileSyncService::CreateBackend() {
   backend_.reset(new browser_sync::SyncBackendHostForProfileSyncTest(
       profile(),
       sync_prefs_.AsWeakPtr(),
       invalidator_storage_.AsWeakPtr(),
+      id_factory_,
+      callback_,
       set_initial_sync_ended_on_init_,
       synchronous_backend_initialization_,
       fail_initial_download_,
       use_real_database_));
 }