Instantiation of OwnerKeyUtil is delegated to OwnerSettingsServiceFactory.

Instantiation of OwnerKeyUtil is delegated to OwnerSettingsServiceFactory, as we're going to move as much as possible from OwnerSettingsService to components/ownership/* and because OwnerSettingsServiceFactory should do all platform-specific business.

BUG=398856
TEST=existing browser_tests and unit_tests

Committed: https://crrev.com/a3e9d8b6ec39cb3fe8f075fe509ff29c3d3a2abb
Cr-Commit-Position: refs/heads/master@{#293486}

[ygorshenin1, 2014-09-02 11:32:31]

ygorshenin@chromium.org changed reviewers:
+ dpolukhin@chromium.org, erikwright@chromium.org

[ygorshenin1, 2014-09-02 11:32:31]

+ Dmitry for chrome/browser/chromeos/*
+ Erik for components/*

[Dmitry Polukhin, 2014-09-02 13:55:37]

lgtm

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

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:58: #include
"chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
I think technically, Include What You Use would necessitate including
owner_settings_service.h too. You are depending on a fully-defined type by
invoking a method of this class, and there is no reason to assume that the
factory's header provides such a fully-defined type.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service.cc:179:
OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();
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.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_factory.h (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_factory.h:34:
owner_key_util_ = owner_key_util;
I'm not sure if it's appropriate to inline an assignment to a refptr. I'd guess
not. Which would also save you an include in this header.

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

PTAL

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:58: #include
"chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
But I'm not using OwnerSettingsService class in this translation unit.
OwnerSettingsServiceFactory::GetInstance() returns an instance of an
OwnerSettingsServiceFactory, not OwnerSettingsService.

On 2014/09/02 19:13:55, erikwright wrote:
> I think technically, Include What You Use would necessitate including
> owner_settings_service.h too. You are depending on a fully-defined type by
> invoking a method of this class, and there is no reason to assume that the
> factory's header provides such a fully-defined type.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service.cc:179:
OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();
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:
> 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.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_factory.h (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_factory.h:34:
owner_key_util_ = owner_key_util;
On 2014/09/02 19:13:56, erikwright wrote:
> I'm not sure if it's appropriate to inline an assignment to a refptr. I'd
guess
> not. Which would also save you an include in this header.

Done.

[erikwright (departed), 2014-09-03 14:07:17]

LGTM.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:58: #include
"chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
On 2014/09/03 10:14:58, ygorshenin1 wrote:
> But I'm not using OwnerSettingsService class in this translation unit.
> OwnerSettingsServiceFactory::GetInstance() returns an instance of an
> OwnerSettingsServiceFactory, not OwnerSettingsService.
> 
> On 2014/09/02 19:13:55, erikwright wrote:
> > I think technically, Include What You Use would necessitate including
> > owner_settings_service.h too. You are depending on a fully-defined type by
> > invoking a method of this class, and there is no reason to assume that the
> > factory's header provides such a fully-defined type.

My bad, thanks.

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/516243002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service.cc:179:
OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();
On 2014/09/03 10:14:58, ygorshenin1 wrote:
> 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:
> > 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.
> 

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.

https://codereview.chromium.org/516243002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_factory.h (right):

https://codereview.chromium.org/516243002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_factory.h:15: #include
"components/ownership/owner_key_util.h"
nit: you should be able to forward-decl this now.

[ygorshenin1, 2014-09-04 07:35:53]

Many thanks!

https://codereview.chromium.org/516243002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ownership/owner_settings_service_factory.h (right):

https://codereview.chromium.org/516243002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/ownership/owner_settings_service_factory.h:15: #include
"components/ownership/owner_key_util.h"
On 2014/09/03 14:07:16, erikwright wrote:
> nit: you should be able to forward-decl this now.

Done.

[ygorshenin1, 2014-09-04 07:36:02]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-09-04 07:36:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/516243002/100001

[commit-bot: I haz the power, 2014-09-04 08:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 08:19:01]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ygorshenin1, 2014-09-04 14:23:54]

The CQ bit was checked by ygorshenin@chromium.org

[ygorshenin1, 2014-09-04 14:23:54]

The CQ bit was unchecked by ygorshenin@chromium.org

[ygorshenin1, 2014-09-04 14:23:55]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-09-04 14:25:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/516243002/120001

[commit-bot: I haz the power, 2014-09-04 15:28:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:28:17]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[ygorshenin1, 2014-09-05 07:20:20]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-09-05 07:21:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/516243002/120001

[commit-bot: I haz the power, 2014-09-05 10:00:36]

Committed patchset #7 (id:120001) as 8ca00474720174ce983209a9a1c9ab303ce4ec51

[commit-bot: I haz the power, 2014-09-10 03:38:21]

Patchset 7 (id:??) landed as
https://crrev.com/a3e9d8b6ec39cb3fe8f075fe509ff29c3d3a2abb
Cr-Commit-Position: refs/heads/master@{#293486}