Move ManagedModePolicyProvider functionality to ManagedUserSettingsService.

Move ManagedModePolicyProvider functionality to ManagedUserSettingsService.

TBR=sky@chromium.org,rsimha@chromium.org
BUG=280674
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222494

[Bernhard Bauer, 2013-09-09 10:16:39]

Please review.

Pam: managed_mode/
Mattias: policy/

Thanks!

[Mattias Nissler (ping if slow), 2013-09-09 11:23:10]

Thanks, LGTM. Can you also switch the policy definitions in
chrome/app/policy/policy_templates.json to deprecated?

[Pam (message me for reviews), 2013-09-09 15:13:48]

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_settings_service.cc (right):

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_settings_service.cc:128:
content::RecordAction(UserMetricsAction("ManagedUsers_UploadItem_Queued"));
A quick comment here explaining when (whence) queued items get uploaded would be
nice.

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_settings_service.cc:161:
SyncMergeResult ManagedUserSettingsService::MergeDataAndStartSyncing(
I'm assuming the bulk of this is copied from the policy provider, so only giving
it a cursory review. Please let me know if that's not the case.

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:115: const
scoped_refptr<PrefStore>& extension_prefs,
No extension prefs in local state anymore? Is this a related change?

[Bernhard Bauer, 2013-09-09 15:31:30]

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
File chrome/browser/managed_mode/managed_user_settings_service.cc (right):

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_settings_service.cc:128:
content::RecordAction(UserMetricsAction("ManagedUsers_UploadItem_Queued"));
On 2013/09/09 15:13:49, Pam (also PM for reviews) wrote:
> A quick comment here explaining when (whence) queued items get uploaded would
be
> nice.

Done.

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/managed_m...
chrome/browser/managed_mode/managed_user_settings_service.cc:161:
SyncMergeResult ManagedUserSettingsService::MergeDataAndStartSyncing(
On 2013/09/09 15:13:49, Pam (also PM for reviews) wrote:
> I'm assuming the bulk of this is copied from the policy provider, so only
giving
> it a cursory review. 

That is correct.

> Please let me know if that's not the case.

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:115: const
scoped_refptr<PrefStore>& extension_prefs,
On 2013/09/09 15:13:49, Pam (also PM for reviews) wrote:
> No extension prefs in local state anymore? Is this a related change?

Only related insofar as I was changing the signatures anyway. There never was an
extension PrefStore in Local State (extensions can only be installed per
profile), and I suspect this dates back from when they were both constructed in
the same way.

[Pam (message me for reviews), 2013-09-10 14:31:54]

lgtm

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
File chrome/browser/prefs/chrome_pref_service_factory.cc (left):

https://codereview.chromium.org/23376002/diff/287001/chrome/browser/prefs/chr...
chrome/browser/prefs/chrome_pref_service_factory.cc:115: const
scoped_refptr<PrefStore>& extension_prefs,
On 2013/09/09 15:31:31, Bernhard Bauer wrote:
> On 2013/09/09 15:13:49, Pam (also PM for reviews) wrote:
> > No extension prefs in local state anymore? Is this a related change?
> 
> Only related insofar as I was changing the signatures anyway. There never was
an
> extension PrefStore in Local State (extensions can only be installed per
> profile), and I suspect this dates back from when they were both constructed
in
> the same way.

Duh, makes sense.

[Bernhard Bauer, 2013-09-10 14:53:30]

Adding more OWNERS:

Joao: policy templates
Elliot: profiles

[Elliot Glaysher, 2013-09-10 21:25:10]

lgtm

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
Is any progress being made on separating out PrefService so it could depend on
things like this?

[Joao da Silva, 2013-09-11 08:06:44]

policy_templates.json lgtm

[Bernhard Bauer, 2013-09-11 08:10:33]

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
On 2013/09/10 21:25:10, Elliot Glaysher wrote:
> Is any progress being made on separating out PrefService so it could depend on
> things like this?

Hm... Mattias, do you know what's missing?

[commit-bot: I haz the power, 2013-09-11 08:10:54]

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

[Mattias Nissler (ping if slow), 2013-09-11 08:17:04]

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
On 2013/09/11 08:10:34, Bernhard Bauer wrote:
> On 2013/09/10 21:25:10, Elliot Glaysher wrote:
> > Is any progress being made on separating out PrefService so it could depend
on
> > things like this?
> 
> Hm... Mattias, do you know what's missing?

I've been chatting about this every now and then with Joao. What needs to happen
mostly is initializing PrefService as a BrowserContextKeyedService, and making
all consuming services declare their dependency properly.

[Bernhard Bauer, 2013-09-11 08:20:39]

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
On 2013/09/11 08:17:05, Mattias Nissler wrote:
> On 2013/09/11 08:10:34, Bernhard Bauer wrote:
> > On 2013/09/10 21:25:10, Elliot Glaysher wrote:
> > > Is any progress being made on separating out PrefService so it could
depend
> on
> > > things like this?
> > 
> > Hm... Mattias, do you know what's missing?
> 
> I've been chatting about this every now and then with Joao. What needs to
happen
> mostly is initializing PrefService as a BrowserContextKeyedService, and making
> all consuming services declare their dependency properly.

Ok... Do we also need to deal with things like extension prefs (which need to be
loaded from Preferences before we can... initialize Preferences)?

[Joao da Silva, 2013-09-11 08:24:59]

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
On 2013/09/11 08:17:05, Mattias Nissler wrote:
> On 2013/09/11 08:10:34, Bernhard Bauer wrote:
> > On 2013/09/10 21:25:10, Elliot Glaysher wrote:
> > > Is any progress being made on separating out PrefService so it could
depend
> on
> > > things like this?
> > 
> > Hm... Mattias, do you know what's missing?
> 
> I've been chatting about this every now and then with Joao. What needs to
happen
> mostly is initializing PrefService as a BrowserContextKeyedService, and making
> all consuming services declare their dependency properly.

Yep. It's hard to do it because PrefService waits asynchronously for the
JsonPrefStore to be ready, but BCKSs can't be created asynchronously yet. It's
also not clear what the impact of that async initialization would be on startup
time, when the first Profile is loaded synchronously (because we can't show UI
until we have the prefs ready).

[commit-bot: I haz the power, 2013-09-11 08:27:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Bernhard Bauer, 2013-09-11 08:50:46]

TBR'ing remaining OWNERS:

Raghu for Sync
Scott for rest of chrome/

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/23376002/diff/306001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:420:
ManagedUserSettingsServiceFactory::GetForProfile(this);
On 2013/09/11 08:25:00, Joao da Silva wrote:
> On 2013/09/11 08:17:05, Mattias Nissler wrote:
> > On 2013/09/11 08:10:34, Bernhard Bauer wrote:
> > > On 2013/09/10 21:25:10, Elliot Glaysher wrote:
> > > > Is any progress being made on separating out PrefService so it could
> depend
> > on
> > > > things like this?
> > > 
> > > Hm... Mattias, do you know what's missing?
> > 
> > I've been chatting about this every now and then with Joao. What needs to
> happen
> > mostly is initializing PrefService as a BrowserContextKeyedService, and
making
> > all consuming services declare their dependency properly.
> 
> Yep. It's hard to do it because PrefService waits asynchronously for the
> JsonPrefStore to be ready, but BCKSs can't be created asynchronously yet. 

Erik Wright is working on an asynchronous DI framework that could help with
that:
https://docs.google.com/a/chromium.org/document/d/1slbNnVtq6ZelEOr-2G0R9iXcaa...

> It's
> also not clear what the impact of that async initialization would be on
startup
> time, when the first Profile is loaded synchronously (because we can't show UI
> until we have the prefs ready).

Assuming that we use some sort of asynchronous dependency injection,
initialization should be the same, except that we block until prefs have been
loaded the first time, no?

[commit-bot: I haz the power, 2013-09-11 08:51:18]

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

[commit-bot: I haz the power, 2013-09-11 11:50:05]

Change committed as 222494