Instantiation of OwnerKeyUtil is delegated to OwnerSettingsServiceFactory.

chrome/browser/chromeos/ownership/owner_settings_service.cc

Index: chrome/browser/chromeos/ownership/owner_settings_service.cc
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service.cc b/chrome/browser/chromeos/ownership/owner_settings_service.cc
index e2b380559e3f5d2e5300bba1f6c2db3a39756ce7..5623e22311ec4a8cf78e03f52e1a1477fe1c6fb4 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service.cc
@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
-#include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
@@ -17,9 +16,7 @@
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/session_manager_operation.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "components/ownership/owner_key_util_impl.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
@@ -43,7 +40,6 @@ namespace chromeos {
 
 namespace {
 
-scoped_refptr<OwnerKeyUtil>* g_owner_key_util_for_testing = NULL;
 DeviceSettingsService* g_device_settings_service_for_testing = NULL;
 
 bool IsOwnerInTests(const std::string& user_id) {
@@ -179,11 +175,9 @@ bool DoesPrivateKeyExistAsyncHelper(
 // not. Responds via |callback|.
 void DoesPrivateKeyExistAsync(
     const OwnerSettingsService::IsOwnerCallback& callback) {
-  scoped_refptr<OwnerKeyUtil> owner_key_util;
-  if (g_owner_key_util_for_testing)
-    owner_key_util = *g_owner_key_util_for_testing;
-  else
-    owner_key_util = OwnerSettingsService::MakeOwnerKeyUtil();
+  scoped_refptr<OwnerKeyUtil> owner_key_util =
+      OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();

[erikwright (departed), 2014/09/02 19:13:56]

This circular dependency between the factory and the service seems pretty
undesirable. It's called by a static method, IsOwnerForSafeModeAsync, which is
called by ChromeCryptohomeAuthenticator.

IsOwnerForSafeModeAsync needs to use ProfileHelper, which is presumably going to
be a barrier to componentization anyway.

I think IsOwnerForSafeModeAsync should be an instance method and
ChromeCryptohomeAuthenticator should be responsible for looking up the instance
of OwnerSettingsService for a given Profile.

[ygorshenin1, 2014/09/03 10:14:58]

I agree with you that we need to get rid of the circular dependency here.
Unfortunately this method is called when only in the safe mode and when profile
corresponding to user_id is not ready yet. For now I propose to pass an
OwnerKeyUtil instance as an argument to
OwnerSettingsService::IsOwnerForSafeModeAsync(). When OwnerSettingsService will
be moved to components/ownership/* all Chrome-specific things like
IsOwnerForSafeModeAsync() will remain in an OwnerSettingsServiceChrome.

On 2014/09/02 19:13:56, erikwright wrote:

[erikwright (departed), 2014/09/03 14:07:16]

In my first reading I thought you were using a Profile, now I see you are using
a profile FilePath. I'm happy with the removal of the circular dependency for
now. Thanks.

+
   if (!owner_key_util) {
     callback.Run(false);
     return;
@@ -241,9 +235,11 @@ bool CheckManagementModeTransition(em::PolicyData::ManagementMode current_mode,
 
 }  // namespace
 
-OwnerSettingsService::OwnerSettingsService(Profile* profile)
+OwnerSettingsService::OwnerSettingsService(
+    Profile* profile,
+    const scoped_refptr<OwnerKeyUtil>& owner_key_util)
     : profile_(profile),
-      owner_key_util_(MakeOwnerKeyUtil()),
+      owner_key_util_(owner_key_util),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
       weak_factory_(this) {
@@ -401,29 +397,6 @@ void OwnerSettingsService::IsOwnerForSafeModeAsync(
 }
 
 // static
-scoped_refptr<ownership::OwnerKeyUtil>
-OwnerSettingsService::MakeOwnerKeyUtil() {
-  base::FilePath public_key_path;
-  if (!PathService::Get(chromeos::FILE_OWNER_KEY, &public_key_path))
-    return NULL;
-  return new ownership::OwnerKeyUtilImpl(public_key_path);
-}
-
-// static
-void OwnerSettingsService::SetOwnerKeyUtilForTesting(
-    const scoped_refptr<OwnerKeyUtil>& owner_key_util) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (g_owner_key_util_for_testing) {
-    delete g_owner_key_util_for_testing;
-    g_owner_key_util_for_testing = NULL;
-  }
-  if (owner_key_util.get()) {
-    g_owner_key_util_for_testing = new scoped_refptr<OwnerKeyUtil>();
-    *g_owner_key_util_for_testing = owner_key_util;
-  }
-}
-
-// static
 void OwnerSettingsService::SetDeviceSettingsServiceForTesting(
     DeviceSettingsService* device_settings_service) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -532,8 +505,6 @@ void OwnerSettingsService::HandleError(DeviceSettingsService::Status status,
 
 scoped_refptr<OwnerKeyUtil> OwnerSettingsService::GetOwnerKeyUtil() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (g_owner_key_util_for_testing)
-    return *g_owner_key_util_for_testing;
   return owner_key_util_;
 }