Move ownership of the ComponentCloudPolicyService to the broker.

Move ownership of the ComponentCloudPolicyService to the broker.

The ComponentCloudPolicyService needs to work with the SchemaRegistry,
so it was initially owned by the DeviceLocalAccountPolicyProvider
(which always has a valid reference to the registry). However, this
means that the ComponentCloudPolicyService can't precache any policies
until the session is started.

To enable precaching at enrollment time and at the login screen, the
ComponentCloudPolicyService has been moved to the broker instead.
This allows some code simplifications too, but requires changing the
ownership of the SchemaRegistry for device local accounts.

This CL is just moving code around; a subsequent CL will trigger
the precaching.

TBR=zelidrag@chromium.org
BUG=224596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278847

[Joao da Silva, 2014-06-20 01:12:39]

Turns out that the first implementation at
https://codereview.chromium.org/341043005 was more complicated than it needs to
be; in this CL I reverted quite a bit of it.

At first I wanted to avoid the complications with the ownership of the
SchemaRegistry, but it's not possible to precache policy-for-extensions without
having the SchemaRegistry around. So this CL just bites that bullet and moves
the code.

PTAL, thanks!

[bartfab (slow), 2014-06-20 09:44:23]

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:10:
#include "base/callback.h"
Nit: Move this to the header file.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:471:
base::Unretained(this),
Nit: #include "base/bind_helpers.h"

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:42: class
ComponentCloudPolicyService;
You include the header for ComponentCloudPolicyService. No need to
forward-declare it as well.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:55: //
|policy_update_callback| can be invoked to notify observers that the policy
Nit: "can" sounds like it may or may not be invoked. How about "will"?

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:72: //
SchemaRegistryService is going to shutdown soon. This is only called if
Nit: s/shutdown/shut down/

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:99:
scoped_ptr<SchemaRegistryService> release_schema_registry();
1: This should be hacker_style(). It is not a simple accessor but a method that
actually passes back ownership.

2: s/release/pass/

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:136:
base::Closure policy_update_callback_;
Since you have a base::Closure member, you will need the full definition of it,
not just callback_forward.h.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:8: #include
"base/macros.h"
Nit: Move this to the header, which uses DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:42:
g_browser_process->platform_part()->browser_policy_connector_chromeos();
Nit: #include "chrome/browser/browser_process_platform_part_chromeos.h"

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:105: // device local
accounts earlier, so that the external data can be preloaded
Nit: s/device local/device-local/

[Joao da Silva, 2014-06-20 15:36:08]

I've addressed the comments on this CL and on the parent CL too. This has
changed a bit after the work with the ForwardingSchemaRegistry; there's one last
CL remaining after this one, that populates the registry for device-local
accounts.

PTAL, thanks!

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:10:
#include "base/callback.h"
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: Move this to the header file.

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:471:
base::Unretained(this),
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: #include "base/bind_helpers.h"

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:42: class
ComponentCloudPolicyService;
On 2014/06/20 09:44:23, bartfab wrote:
> You include the header for ComponentCloudPolicyService. No need to
> forward-declare it as well.

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:55: //
|policy_update_callback| can be invoked to notify observers that the policy
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: "can" sounds like it may or may not be invoked. How about "will"?

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:72: //
SchemaRegistryService is going to shutdown soon. This is only called if
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: s/shutdown/shut down/

Obsolete

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:99:
scoped_ptr<SchemaRegistryService> release_schema_registry();
On 2014/06/20 09:44:23, bartfab wrote:
> 1: This should be hacker_style(). It is not a simple accessor but a method
that
> actually passes back ownership.
> 
> 2: s/release/pass/

Obsolete

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:136:
base::Closure policy_update_callback_;
On 2014/06/20 09:44:23, bartfab wrote:
> Since you have a base::Closure member, you will need the full definition of
it,
> not just callback_forward.h.

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:8: #include
"base/macros.h"
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: Move this to the header, which uses DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:42:
g_browser_process->platform_part()->browser_policy_connector_chromeos();
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: #include "chrome/browser/browser_process_platform_part_chromeos.h"

Done.

https://codereview.chromium.org/342233005/diff/40001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:105: // device local
accounts earlier, so that the external data can be preloaded
On 2014/06/20 09:44:23, bartfab wrote:
> Nit: s/device local/device-local/

Done.

[bartfab (slow), 2014-06-20 16:41:11]

lgtm

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:37: // Bail out on unit
tests that don't have a UserManager.
Nit: s/ on / in /.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:113: // device-local
accounts earlier, so that the external data can be preloaded
Nit: "earlier" is very hard to interpret in the context of this sentence. How
about something like:

// The SchemaRegistry for a device-local account is owned by its
// DeviceLocalAccountPolicyBroker, which uses the registry to fetch
// and cache policy even if there is no active session for that
// account.
// Use a ForwardingSchemaRegistry that wraps this SchemaRegistry.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.h (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.h:10: #include
"base/basictypes.h"
Nit: Obsoleted by base/macros.h.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:446: # If this is a
|publicaccount| request then get the |username| now and use
Nit: s/request then/request,/

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:449: # Unfortunately the
username can't be obtained from |msg| because that
Nit 1: s/Unfortunately/Unfortunately, /
Nit 2: s/username/|username|/

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:765: elif msg.policy_type ==
'google/chromeos/publicaccount':
This is redundant now.

[Joao da Silva, 2014-06-20 17:25:34]

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.cc (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:37: // Bail out on unit
tests that don't have a UserManager.
On 2014/06/20 16:41:10, bartfab wrote:
> Nit: s/ on / in /.

Done.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.cc:113: // device-local
accounts earlier, so that the external data can be preloaded
On 2014/06/20 16:41:10, bartfab wrote:
> Nit: "earlier" is very hard to interpret in the context of this sentence. How
> about something like:
> 
> // The SchemaRegistry for a device-local account is owned by its
> // DeviceLocalAccountPolicyBroker, which uses the registry to fetch
> // and cache policy even if there is no active session for that
> // account.
> // Use a ForwardingSchemaRegistry that wraps this SchemaRegistry.

Done.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
File chrome/browser/policy/schema_registry_service_factory.h (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/sc...
chrome/browser/policy/schema_registry_service_factory.h:10: #include
"base/basictypes.h"
On 2014/06/20 16:41:11, bartfab wrote:
> Nit: Obsoleted by base/macros.h.

Done.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
File chrome/browser/policy/test/policy_testserver.py (right):

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:446: # If this is a
|publicaccount| request then get the |username| now and use
On 2014/06/20 16:41:11, bartfab wrote:
> Nit: s/request then/request,/

Done.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:449: # Unfortunately the
username can't be obtained from |msg| because that
On 2014/06/20 16:41:11, bartfab wrote:
> Nit 1: s/Unfortunately/Unfortunately, /
> Nit 2: s/username/|username|/

Done.

https://codereview.chromium.org/342233005/diff/60001/chrome/browser/policy/te...
chrome/browser/policy/test/policy_testserver.py:765: elif msg.policy_type ==
'google/chromeos/publicaccount':
On 2014/06/20 16:41:11, bartfab wrote:
> This is redundant now.

Done.

[Joao da Silva, 2014-06-20 18:33:39]

Zel to TBR for chromeos_paths.h

[Joao da Silva, 2014-06-20 18:33:42]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:04:08]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/342233005/150001

[commit-bot: I haz the power, 2014-06-20 22:43:27]

Change committed as 278847