Mark supervised profiles as such immediately when they're created.

Mark supervised profiles as such immediately when they're created.

This happens in Profile::InitProfileUserPrefs(), so we add a new method Profile::Delegate::OnPrefsLoaded() which is called immediately after the PrefService has been creted, and which ProfileManager implements by calling InitProfileUserPrefs().

Also, remove ManagedUserService::InitForTesting in favor of marking the profile as managed at creation.

BUG=279307
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229237

[Bernhard Bauer, 2013-10-01 16:44:11]

Please review.

Rachel: profiles/
Sergiu: managed_mode/

Thanks!

[rpetterson, 2013-10-01 20:19:56]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:554: InitProfileUserPrefs(profile);
It doesn't really change the call flow, so I'm curious why you moved this up
here?

[Bernhard Bauer, 2013-10-01 20:42:15]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:554: InitProfileUserPrefs(profile);
On 2013/10/01 20:19:57, rpetterson wrote:
> It doesn't really change the call flow, so I'm curious why you moved this up
> here?

InitProfileUserPrefs() is where we mark a profile as managed when it is
initially created. I need to run this before the BrowserContextKeyedServices are
created, because some of them might do their initialization differently for a
managed profile. I updated the CL description to (hopefully) make that a bit
clearer.

[Sergiu, 2013-10-02 08:56:17]

lgtm managed_mode/*

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:30:
manager_(TestingBrowserProcess::GetGlobal()) {}
nit: 1 extra space

[Bernhard Bauer, 2013-10-02 09:05:45]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:30:
manager_(TestingBrowserProcess::GetGlobal()) {}
On 2013/10/02 08:56:18, Sergiu wrote:
> nit: 1 extra space

Sorry, where?

[rpetterson, 2013-10-07 23:22:49]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:554: InitProfileUserPrefs(profile);
On 2013/10/01 20:42:15, Bernhard Bauer wrote:
> On 2013/10/01 20:19:57, rpetterson wrote:
> > It doesn't really change the call flow, so I'm curious why you moved this up
> > here?
> 
> InitProfileUserPrefs() is where we mark a profile as managed when it is
> initially created. I need to run this before the BrowserContextKeyedServices
are
> created, because some of them might do their initialization differently for a
> managed profile. I updated the CL description to (hopefully) make that a bit
> clearer.

I understand better what's going on now. However, I don't think managed mode is
the only type of profile creation which uses a delegate so how will this affect
other profile creation flows? If it won't, perhaps it would be simpler to bump
up InitProfileUserPrefs to just before the browser services creation for
everyone.

[Bernhard Bauer, 2013-10-08 14:56:01]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:554: InitProfileUserPrefs(profile);
On 2013/10/07 23:22:49, rpetterson wrote:
> On 2013/10/01 20:42:15, Bernhard Bauer wrote:
> > On 2013/10/01 20:19:57, rpetterson wrote:
> > > It doesn't really change the call flow, so I'm curious why you moved this
up
> > > here?
> > 
> > InitProfileUserPrefs() is where we mark a profile as managed when it is
> > initially created. I need to run this before the BrowserContextKeyedServices
> are
> > created, because some of them might do their initialization differently for
a
> > managed profile. I updated the CL description to (hopefully) make that a bit
> > clearer.
> 
> I understand better what's going on now. However, I don't think managed mode
is
> the only type of profile creation which uses a delegate so how will this
affect
> other profile creation flows? If it won't, perhaps it would be simpler to bump
> up InitProfileUserPrefs to just before the browser services creation for
> everyone.

This CL actually does change the behavior for everyone, it's just that
supervised user creation is the rationale behind it, so that's what I mentioned
in the description.

Or are you refering to moving it to another place closer to browser services
creation?

[rpetterson, 2013-10-08 18:02:54]

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/13002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:554: InitProfileUserPrefs(profile);
On 2013/10/08 14:56:01, Bernhard Bauer wrote:
> On 2013/10/07 23:22:49, rpetterson wrote:
> > On 2013/10/01 20:42:15, Bernhard Bauer wrote:
> > > On 2013/10/01 20:19:57, rpetterson wrote:
> > > > It doesn't really change the call flow, so I'm curious why you moved
this
> up
> > > > here?
> > > 
> > > InitProfileUserPrefs() is where we mark a profile as managed when it is
> > > initially created. I need to run this before the
BrowserContextKeyedServices
> > are
> > > created, because some of them might do their initialization differently
for
> a
> > > managed profile. I updated the CL description to (hopefully) make that a
bit
> > > clearer.
> > 
> > I understand better what's going on now. However, I don't think managed mode
> is
> > the only type of profile creation which uses a delegate so how will this
> affect
> > other profile creation flows? If it won't, perhaps it would be simpler to
bump
> > up InitProfileUserPrefs to just before the browser services creation for
> > everyone.
> 
> This CL actually does change the behavior for everyone, it's just that
> supervised user creation is the rationale behind it, so that's what I
mentioned
> in the description.
> 
> Or are you refering to moving it to another place closer to browser services
> creation?

Yes. If we're changing it for everyone, there should be just one path. With this
CL, you can init the prefs either in OnPrefsLoaded via the delegate or in
AddProfile. We should eliminate the use of the delegate in this case and just
call InitProfileUserPrefs right before browser services creation (like
ProfileImpl::785). Perhaps make that function static or move it to the profile?

[Bernhard Bauer, 2013-10-09 21:38:13]

OK, I directly call InitProfileUserPrefs now instead of going through the
delegate interface. PTAL?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2013-10-16 19:10:43]

Ping? :)

[rpetterson, 2013-10-16 19:40:02]

LGTM

Sorry this fell into my e-mail abyss! Thank you for pinging :) And thanks for
making those changes.

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1020: managed_user_id = "Test ID";
This seems an odd id to have in real life. Does it only get executed for tests?

[Bernhard Bauer, 2013-10-16 19:47:20]

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:1020: managed_user_id = "Test ID";
On 2013/10/16 19:40:03, rpetterson wrote:
> This seems an odd id to have in real life. Does it only get executed for
tests?

Yes, this only happens if the command line switch is set, which some browser
tests do (as they have a real profile manager and real profiles).

[Bernhard Bauer, 2013-10-16 21:08:57]

Adding more OWNERS:

Benjamin: extensions
Peter: themes
Scott: tests

Thanks!

[not at google - send to devlin, 2013-10-16 21:12:49]

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:568:
InitializeExtensionService(params);
this 1 -> 3 line thing is awkward. Can you make InitializeExtensionServiceParams
more like CreatEExtensionServiceParams and return a scoped_ptr or even just a
value. It's not a big structure.

[sky, 2013-10-16 22:20:55]

chrome/test LGTM

[pkotwicz, 2013-10-16 23:56:54]

I will take a look at the CL on Thursday

[Bernhard Bauer, 2013-10-17 14:12:02]

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/67001/chrome/browser/extensions...
chrome/browser/extensions/extension_service_unittest.cc:568:
InitializeExtensionService(params);
On 2013/10/16 21:12:50, kalman wrote:
> this 1 -> 3 line thing is awkward. Can you make
InitializeExtensionServiceParams
> more like CreatEExtensionServiceParams and return a scoped_ptr or even just a
> value. It's not a big structure.

Done.

[not at google - send to devlin, 2013-10-17 14:25:26]

lgtm

[pkotwicz, 2013-10-17 16:49:08]

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:99: if (!(theme_supplier_ &&
I do not think that this block is needed.
LoadThemePrefs() will set the managed user theme if appropriate

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:30:
manager_(TestingBrowserProcess::GetGlobal()) {}
Nit: one space too many

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:105: class
ThemeServiceManagedUserTest : public ThemeServiceTest {
Nit: Move ThemeServiceManagedUserTest above the ThemeServiceManagedUserTests

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:267: 
Nit: Extra line is unnecessary

[Bernhard Bauer, 2013-10-17 16:55:02]

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service.cc:99: if (!(theme_supplier_ &&
On 2013/10/17 16:49:09, pkotwicz wrote:
> I do not think that this block is needed.
> LoadThemePrefs() will set the managed user theme if appropriate

Cool! Done.

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:30:
manager_(TestingBrowserProcess::GetGlobal()) {}
On 2013/10/17 16:49:09, pkotwicz wrote:
> Nit: one space too many

Done.

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:105: class
ThemeServiceManagedUserTest : public ThemeServiceTest {
On 2013/10/17 16:49:09, pkotwicz wrote:
> Nit: Move ThemeServiceManagedUserTest above the ThemeServiceManagedUserTests

Done.

https://codereview.chromium.org/23868042/diff/80001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:267: 
On 2013/10/17 16:49:09, pkotwicz wrote:
> Nit: Extra line is unnecessary

Removed.

[pkotwicz, 2013-10-17 17:40:19]

LGTM with nit

https://codereview.chromium.org/23868042/diff/95001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/95001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:233: public:
Nit: Add constructor and destructor

[Bernhard Bauer, 2013-10-17 18:14:50]

https://codereview.chromium.org/23868042/diff/95001/chrome/browser/themes/the...
File chrome/browser/themes/theme_service_unittest.cc (right):

https://codereview.chromium.org/23868042/diff/95001/chrome/browser/themes/the...
chrome/browser/themes/theme_service_unittest.cc:233: public:
On 2013/10/17 17:40:20, pkotwicz wrote:
> Nit: Add constructor and destructor

Done.

[commit-bot: I haz the power, 2013-10-17 18:29:54]

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

[commit-bot: I haz the power, 2013-10-17 22:05:29]

Change committed as 229237