Make SyncBackendRegistrar aware of loaded data

sync/internal_api/sync_manager_impl.cc

Index: sync/internal_api/sync_manager_impl.cc
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 6a164d8f0ff2aac4ff56893cfc13914c323e1737..a170d01861925ef78a30e44df29e2d11cba95e3a 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -369,7 +369,6 @@ bool SyncManagerImpl::Init(
     bool use_ssl,
     const scoped_refptr<base::TaskRunner>& blocking_task_runner,
     scoped_ptr<HttpPostProviderFactory> post_factory,
-    const ModelSafeRoutingInfo& model_safe_routing_info,
     const std::vector<ModelSafeWorker*>& workers,
     ExtensionsActivityMonitor* extensions_activity_monitor,
     SyncManager::ChangeDelegate* change_delegate,
@@ -383,6 +382,8 @@ bool SyncManagerImpl::Init(
   CHECK(!initialized_);
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(post_factory.get());
+  DCHECK(!credentials.email.empty());
+  DCHECK(!credentials.sync_token.empty());
   DVLOG(1) << "SyncManager starting Init...";
 
   weak_handle_this_ = MakeWeakHandle(weak_ptr_factory_.GetWeakPtr());
@@ -411,6 +412,7 @@ bool SyncManagerImpl::Init(
           credentials.email, absolute_db_path).Pass();
 
   DCHECK(backing_store.get());
+  share_.name = credentials.email;

[rlarocque, 2012/07/25 18:45:08]

Formerly part of SignIn()

   share_.directory.reset(
       new syncable::Directory(encryptor_,
                               unrecoverable_error_handler_,
@@ -425,28 +427,48 @@ bool SyncManagerImpl::Init(
 
   connection_manager_->AddListener(this);
 
-  // Build a SyncSessionContext and store the worker in it.
-  DVLOG(1) << "Sync is bringing up SyncSessionContext.";
-  std::vector<SyncEngineEventListener*> listeners;
-  listeners.push_back(&allstatus_);
-  listeners.push_back(this);
-  session_context_ = internal_components_factory->BuildContext(
-      connection_manager_.get(),
-      directory(),
-      model_safe_routing_info,
-      workers,
-      extensions_activity_monitor,
-      &throttled_data_type_tracker_,
-      listeners,
-      &debug_info_event_listener_,
-      &traffic_recorder_).Pass();
-  session_context_->set_account_name(credentials.email);
-  scheduler_ = internal_components_factory->BuildScheduler(
-      name_, session_context_.get()).Pass();
-
-  bool success = SignIn(credentials);
-
+  DVLOG(1) << "Username: " << username_for_share();

[rlarocque, 2012/07/25 18:45:08]

Inlined SignIn code begins here.

+  bool success = OpenDirectory();
   if (success) {

[tim (not reviewing), 2012/07/26 18:48:05]

I know this code did if (success) { before, but now there's even more
indentation.  What do we have to worry about if (!success)?  Could we bail?  I
think the only thing we're factoring out this way is firing the notification,
which already has a ternary to distinguish |success| anyway.  I think it'd be
clearer to do

if (!success) {
  FOR_EACH_OBSERVER(...)
  return false;
}

// carry on

[rlarocque, 2012/07/26 21:59:54]

Good idea.

+    // Retrieve and set the sync notifier state.
+    std::string unique_id = directory()->cache_guid();
+    DVLOG(1) << "Read notification unique ID: " << unique_id;
+    allstatus_.SetUniqueId(unique_id);
+    sync_notifier_->SetUniqueId(unique_id);
+
+    std::string state = directory()->GetNotificationState();
+    if (VLOG_IS_ON(1)) {
+      std::string encoded_state;
+      base::Base64Encode(state, &encoded_state);
+      DVLOG(1) << "Read notification state: " << encoded_state;
+    }
+
+    // TODO(tim): Remove once invalidation state has been migrated to new
+    // InvalidationStateTracker store. Bug 124140.
+    sync_notifier_->SetStateDeprecated(state);
+
+    connection_manager_->set_auth_token(credentials.sync_token);

[rlarocque, 2012/07/25 18:45:08]

These two lines are effectively what UpdateCredentials does in this context. 
Normally, it would also:
- set observering_ip_address_changes_, but that's redundant because we just set
it a few lines earlier.
- not update the notifier's credentials if set_auth_token fails, but this call
to set_auth_token will not fail.
- Call scheduler_->OnCredentialsUpdated() if initialized_, but initialized_ is
not set right now.

Not calling scheduler_->OnCredentialsUpdated() is a real change.  We would have
called it when SignIn() was in its traditional location.  Fortunately, the call
has no effect during startup; it's used mainly to clear auth errors.

[tim (not reviewing), 2012/07/26 18:48:05]

What is the benefit of not calling UpdateCredentials? There's a lot to gain from
reusing a single code path and set of assumptions to handle all this and rely on
pre/post conditions. 

[rlarocque, 2012/07/26 21:59:54]

It's debatable.  The arguments for are:
- UpdateCredentials() behaves differently in init and non-init situations.  By
moving the initialization work here, we could remove its if (initialized_)
block.
- The inlined call gets simpler too.  We no longer need to check if
set_auth_token() fails and there's no need to set the boolean flag.
- Since so many member variables are involved, the initialization process is
made more readable by limiting the number of functions we use.  Otherwise it's
difficult to tell which member variables have and have not been initialized.

I prefer it this way, but I'm also willing to put it back the way it was.  There
are good arguments for reusing the code, too.

+    sync_notifier_->UpdateCredentials(credentials.email,

[rlarocque, 2012/07/25 18:45:08]

I think it's safe to initialize the sync_notifier here.  We're on the sync
thread, and won't release the thread until the SyncScheduler and context are
prepared to handle notifications.  

We could move it to below the session_context_ initialization and
scheduler_->Start() calls if you're worried, though.

[tim (not reviewing), 2012/07/26 18:48:05]

I think this is okay too - the call to AddObserver is the critical one below,
because calling sync_notifier->UpdateCredentials (and SetState) can trigger
notifications, and we don't want to handle them if we're !initialized_.

+                                      credentials.sync_token);
+
+    // Build a SyncSessionContext and store the worker in it.
+    DVLOG(1) << "Sync is bringing up SyncSessionContext.";
+    std::vector<SyncEngineEventListener*> listeners;
+    listeners.push_back(&allstatus_);
+    listeners.push_back(this);
+    session_context_ = internal_components_factory->BuildContext(

[rlarocque, 2012/07/25 18:45:08]

Warning: Real code change here.

We used to create the session context regardless of whether or not the directory
open failed.  With this refactoring, it is initialized only when the directory
opened successfully.  This is probably better, though we should be wary of any
change.

[tim (not reviewing), 2012/07/26 18:48:05]

Agreed, it would be strange if we were relying on a notification (from the
context) in the case where we failed to open the directory, which is the only
possible side effect I can see.  Although SendInitialSnapshot comes to mind...
do the integration tests like this?

+        connection_manager_.get(),
+        directory(),
+        workers,
+        extensions_activity_monitor,
+        &throttled_data_type_tracker_,
+        listeners,
+        &debug_info_event_listener_,
+        &traffic_recorder_).Pass();
+    session_context_->set_account_name(credentials.email);
+    scheduler_ = internal_components_factory->BuildScheduler(
+        name_, session_context_.get()).Pass();
+
     scheduler_->Start(SyncScheduler::CONFIGURATION_MODE);
 
     initialized_ = true;
@@ -482,7 +504,8 @@ bool SyncManagerImpl::Init(
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        success));
+                        success, success ? InitialSyncEndedTypes()
+                                         : syncer::ModelTypeSet()));
   if (!success)
     return false;
 
@@ -668,35 +691,6 @@ bool SyncManagerImpl::OpenDirectory() {
   return true;
 }
 
-bool SyncManagerImpl::SignIn(const SyncCredentials& credentials) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(share_.name.empty());
-  share_.name = credentials.email;
-
-  DVLOG(1) << "Signing in user: " << username_for_share();
-  if (!OpenDirectory())
-    return false;
-
-  // Retrieve and set the sync notifier state. This should be done
-  // only after OpenDirectory is called.
-  std::string unique_id = directory()->cache_guid();
-  std::string state = directory()->GetNotificationState();
-  DVLOG(1) << "Read notification unique ID: " << unique_id;
-  if (VLOG_IS_ON(1)) {
-    std::string encoded_state;
-    base::Base64Encode(state, &encoded_state);
-    DVLOG(1) << "Read notification state: " << encoded_state;
-  }
-  allstatus_.SetUniqueId(unique_id);
-  sync_notifier_->SetUniqueId(unique_id);
-  // TODO(tim): Remove once invalidation state has been migrated to new
-  // InvalidationStateTracker store. Bug 124140.
-  sync_notifier_->SetStateDeprecated(state);
-
-  UpdateCredentials(credentials);
-  return true;
-}
-
 bool SyncManagerImpl::PurgePartiallySyncedTypes() {
   ModelTypeSet partially_synced_types = ModelTypeSet::All();
   partially_synced_types.RemoveAll(InitialSyncEndedTypes());