Make SyncBackendRegistrar aware of loaded data

sync/internal_api/sync_manager_impl.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 "sync/internal_api/sync_manager_impl.h"
 
 #include <string>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ skipping 351 matching lines @@
 }
 
 bool SyncManagerImpl::Init(
     const FilePath& database_location,
     const WeakHandle<JsEventHandler>& event_handler,
     const std::string& sync_server_and_path,
     int port,
     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,
     const SyncCredentials& credentials,
     scoped_ptr<SyncNotifier> sync_notifier,
     const std::string& restored_key_for_bootstrapping,
     scoped_ptr<InternalComponentsFactory> internal_components_factory,
     Encryptor* encryptor,
     UnrecoverableErrorHandler* unrecoverable_error_handler,
     ReportUnrecoverableErrorFunction report_unrecoverable_error_function) {
   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());
 
   blocking_task_runner_ = blocking_task_runner;
 
   change_delegate_ = change_delegate;
 
   sync_notifier_ = sync_notifier.Pass();
 
   AddObserver(&js_sync_manager_observer_);
   SetJsEventHandler(event_handler);
 
   AddObserver(&debug_info_event_listener_);
 
   database_path_ = database_location.Append(
       syncable::Directory::kSyncDatabaseFilename);
   encryptor_ = encryptor;
   unrecoverable_error_handler_ = unrecoverable_error_handler;
   report_unrecoverable_error_function_ = report_unrecoverable_error_function;
 
   FilePath absolute_db_path(database_path_);
   file_util::AbsolutePath(&absolute_db_path);
   scoped_ptr<syncable::DirectoryBackingStore> backing_store =
       internal_components_factory->BuildDirectoryBackingStore(
           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_,
                               report_unrecoverable_error_function_,
                               backing_store.release()));
 
   connection_manager_.reset(new SyncAPIServerConnectionManager(
       sync_server_and_path, port, use_ssl, post_factory.release()));
 
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
   observing_ip_address_changes_ = true;
 
   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();
+  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);
 
-  bool success = SignIn(credentials);
+    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;
+    }
 
-  if (success) {
+    // 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;
 
     // Unapplied datatypes (those that do not have initial sync ended set) get
     // re-downloaded during any configuration. But, it's possible for a datatype
     // to have a progress marker but not have initial sync ended yet, making
     // it a candidate for migration. This is a problem, as the DataTypeManager
     // does not support a migration while it's already in the middle of a
     // configuration. As a result, any partially synced datatype can stall the
@@ skipping 15 matching lines @@
   }
 
   // Notify that initialization is complete. Note: This should be the last to
   // execute if |signed_in| is false. Reason being in that case we would
   // post a task to shutdown sync. But if this function posts any other tasks
   // on the UI thread and if shutdown wins then that tasks would execute on
   // a freed pointer. This is because UI thread is not shut down.
   FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
                     OnInitializationComplete(
                         MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()),
-                        success));
+                        success, success ? InitialSyncEndedTypes()
+                                         : syncer::ModelTypeSet()));
   if (!success)
     return false;
 
   sync_notifier_->AddObserver(this);
 
   return success;
 }
 
 void SyncManagerImpl::RefreshNigori(const std::string& chrome_version,
                                     const base::Closure& done_callback) {
@@ skipping 165 matching lines @@
                                   transaction_observer);
   if (open_result != syncable::OPENED) {
     LOG(ERROR) << "Could not open share for:" << username_for_share();
     return false;
   }
 
   connection_manager_->set_client_id(directory()->cache_guid());
   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());
   partially_synced_types.RemoveAll(GetTypesWithEmptyProgressMarkerToken(
       ModelTypeSet::All()));
 
   UMA_HISTOGRAM_COUNTS("Sync.PartiallySyncedTypes",
                        partially_synced_types.Size());
   if (partially_synced_types.Empty())
     return true;
@@ skipping 1181 matching lines @@
 int SyncManagerImpl::GetDefaultNudgeDelay() {
   return kDefaultNudgeDelayMilliseconds;
 }
 
 // static.
 int SyncManagerImpl::GetPreferencesNudgeDelay() {
   return kPreferencesNudgeDelayMilliseconds;
 }
 
 }  // namespace syncer