wifi_sync: allow WifiCredentialSyncableServiceFactory to ignore LoginState

components/wifi_sync/wifi_credential_syncable_service_factory.cc

Index: components/wifi_sync/wifi_credential_syncable_service_factory.cc
diff --git a/components/wifi_sync/wifi_credential_syncable_service_factory.cc b/components/wifi_sync/wifi_credential_syncable_service_factory.cc
index cf3b46020cc7d1c61b50313e0da097190e90d282..fbc99df49dd9ab93e28c9c492fbb77b9acf58f72 100644
--- a/components/wifi_sync/wifi_credential_syncable_service_factory.cc
+++ b/components/wifi_sync/wifi_credential_syncable_service_factory.cc
@@ -7,9 +7,12 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/wifi_sync/wifi_config_delegate.h"
 #include "components/wifi_sync/wifi_credential_syncable_service.h"
+#include "content/public/browser/browser_context.h"
 
 #if defined(OS_CHROMEOS)
+#include "base/files/file_path.h"
 #include "chromeos/login/login_state.h"
 #include "chromeos/network/network_handler.h"
 #include "components/wifi_sync/wifi_config_delegate_chromeos.h"
@@ -17,31 +20,6 @@
 
 namespace wifi_sync {
 
-namespace {
-
-scoped_ptr<WifiConfigDelegate> BuildConfigDelegate(
-    content::BrowserContext* context) {
-#if defined(OS_CHROMEOS)
-  const chromeos::LoginState* login_state = chromeos::LoginState::Get();
-  DCHECK(login_state->IsUserLoggedIn());
-  DCHECK(!login_state->primary_user_hash().empty());
-  // TODO(quiche): Verify that |context| is the primary user's context.
-
-  // Note: NetworkHandler is a singleton that is managed by
-  // ChromeBrowserMainPartsChromeos, and destroyed after all
-  // KeyedService instances are destroyed.
-  chromeos::NetworkHandler* network_handler = chromeos::NetworkHandler::Get();
-  return make_scoped_ptr(new WifiConfigDelegateChromeOs(
-      login_state->primary_user_hash(),
-      network_handler->managed_network_configuration_handler()));
-#else
-  NOTREACHED();
-  return nullptr;
-#endif
-}
-
-}  // namespace
-
 // static
 WifiCredentialSyncableService*
 WifiCredentialSyncableServiceFactory::GetForBrowserContext(
@@ -56,12 +34,19 @@ WifiCredentialSyncableServiceFactory::GetInstance() {
   return Singleton<WifiCredentialSyncableServiceFactory>::get();
 }
 
+#if defined(OS_CHROMEOS)
+void WifiCredentialSyncableServiceFactory::IgnoreLoginStateForTest() {

[stevenjb, 2015/01/28 01:04:58]

Rather than having a method to set this, would it be straightforward to have a
BuildWifiConfigDelegateForTest method instead?

[mukesh agrawal, 2015/01/28 20:07:23]

Unfortunately, we don't have control over construction of the
WifiCredentialSyncableService instances. They're constructed by the sync
integration test framework, using ProfileSyncComponentsFactoryImpl.

FWIW, here's my best guess at the call-stack (after collapsing callbacks as if
they completed synchronously):

SyncTest::SetupSync() {  // called from each wifi_sync integration test
  for (int i = 0; i < num_clients_; ++i) {
    ProfileSyncServiceHarness* client = GetClient(i);
    client->ProfileSyncServiceHarness::SetupSync() {
      ProfileSyncService::OnUserChoseDatatypes() {
        ProfileSyncService::ChangePreferredDataTypes() {
          ProfileSyncService::ReconfigureDataTypeManager() {
            ProfileSyncService::ConfigureDataTypeManager() {
              DataTypeManagerImpl::Configure() {
                DataTypeManagerImpl::ConfigureImpl() {
                  DataTypeManagerImpl::Restart() {
                    SyncBackendHostImpl::ConfigureDataTypes() {
                      SyncBackendHostImpl::RequestConfigureSyncer() {
                        SyncBackendHostCore::DoConfigureSyncer() {
                          SyncManagerImpl::ConfigureSyncer() {
                            SyncSchedulerImpl::ScheduleConfiguration() {
                              SyncBackendHostCore::DoFinishConfigureDataTypes()
{
                               
SyncBackendHostImpl::FinishConfigureDataTypesOnFrontendLoop() {
                                  DataTypeManagerImpl::DownloadReady() {
                                    DataTypeManagerImpl::StartNextAssociation()
{
                                     
ModelAssociationManager::StartAssociationAsync() {
                                        UIDataTypeController::StartAssociating()
{
                                          UIDataTypeController::Associate() {
                                            SharedChangeProcessor::Connect() {
                                             
ProfileSyncComponentsFactoryImpl::GetSyncableServiceForType() {
                                               
wifi_sync::WifiCredentialSyncableServiceFactory::
                                                    GetForBrowserContext();

+  use_login_state_chrome_os_ = false;
+}
+#endif
+
 // Private methods.
 
 WifiCredentialSyncableServiceFactory::WifiCredentialSyncableServiceFactory()
     : BrowserContextKeyedServiceFactory(
         "WifiCredentialSyncableService",
-        BrowserContextDependencyManager::GetInstance()) {
+        BrowserContextDependencyManager::GetInstance()),
+      use_login_state_chrome_os_(true) {
 }
 
 WifiCredentialSyncableServiceFactory::~WifiCredentialSyncableServiceFactory() {
@@ -71,7 +56,45 @@ KeyedService* WifiCredentialSyncableServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   // TODO(quiche): Figure out if this behaves properly for multi-profile.
   // crbug.com/430681.
-  return new WifiCredentialSyncableService(BuildConfigDelegate(context));
+  return new WifiCredentialSyncableService(BuildWifiConfigDelegate(context));
+}
+
+scoped_ptr<WifiConfigDelegate>
+WifiCredentialSyncableServiceFactory::BuildWifiConfigDelegate(
+    content::BrowserContext* context) const {

[stevenjb, 2015/01/28 01:04:58]

This extra layer just adds confusion IMHO, I would just fold this into the
method above.

[mukesh agrawal, 2015/01/28 20:07:23]

Done.

+#if defined(OS_CHROMEOS)
+  return BuildWifiConfigDelegateChromeOs(context);
+#else
+  NOTREACHED();
+  return nullptr;
+#endif
 }
 
+#if defined(OS_CHROMEOS)
+scoped_ptr<WifiConfigDelegate>
+WifiCredentialSyncableServiceFactory::BuildWifiConfigDelegateChromeOs(
+    content::BrowserContext* context) const {
+  // Note: NetworkHandler is a singleton that is managed by
+  // ChromeBrowserMainPartsChromeos, and destroyed after all
+  // KeyedService instances are destroyed.
+  chromeos::NetworkHandler* network_handler = chromeos::NetworkHandler::Get();
+  return make_scoped_ptr(new WifiConfigDelegateChromeOs(
+      GetShillProfile(context),
+      network_handler->managed_network_configuration_handler()));
+}
+
+std::string WifiCredentialSyncableServiceFactory::GetShillProfile(

[stevenjb, 2015/01/28 01:04:57]

This doesn't return a Profile as the name suggests, but rather an identifier, we
should rename this to reflect that.

[mukesh agrawal, 2015/01/28 20:07:24]

How's this? (I consider GetShillProfileId, but that might imply
"/profile/user-hash", instead of just "user-hash".)

+    content::BrowserContext* context) const {
+  if (use_login_state_chrome_os_) {
+    const chromeos::LoginState* login_state = chromeos::LoginState::Get();
+    DCHECK(login_state->IsUserLoggedIn());
+    DCHECK(!login_state->primary_user_hash().empty());
+    // TODO(quiche): Verify that |context| is the primary user's context.
+    return login_state->primary_user_hash();
+  } else {
+    return context->GetPath().BaseName().value();
+  }
+}
+#endif
+
 }  // namespace wifi_sync