Refactored ConfigurationPolicyProvider to provide PolicyBundles instead of PolicyMaps.

Refactored ConfigurationPolicyProvider to provide PolicyBundles instead of PolicyMaps.

Also refactored the PolicyServiceImpl to merge data from PolicyBundles.

BUG=108993
TEST=All works as before, unit_tests green


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137244

[Joao da Silva, 2012-05-11 15:56:32]

Here's the next one. The next steps are to move all providers/loaders to provide
a PolicyBundle, and then remove ConfigurationPolicyProvider::Provide().

Please review, thanks!

[Mattias Nissler (ping if slow), 2012-05-14 09:07:21]

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_loader.h:112: UpdateCallback
updates_callback_;
Update vs updates is inconsistent. Can we rename to update_callback_?

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_provider.cc:104: 
remove newline?

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider.h:66: void
UpdateProviderPolicy(scoped_ptr<PolicyBundle> bundle);
Why not just UpdatePolicy()?

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/mock...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/mock...
chrome/browser/policy/mock_configuration_policy_provider.h:29: //
PolicyDirectory that maps the Chrome namespace to a copy of |policy|.
Bundle

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:97: if (it == registrars_.end())
NOTREACHED()? Or can this situation actually happen?

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:107: for (RegistrarList::iterator
it = registrars_.begin();
std::find? Also, how about making this a std::map<Provider*, Registrar*>?

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
Can we have a unit test for this?

[Joao da Silva, 2012-05-14 09:39:16]

Please have another look.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_loader.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_loader.h:112: UpdateCallback
updates_callback_;
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> Update vs updates is inconsistent. Can we rename to update_callback_?

Done.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/clou...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/clou...
chrome/browser/policy/cloud_policy_provider.cc:104: 
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> remove newline?

Done.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider.h:66: void
UpdateProviderPolicy(scoped_ptr<PolicyBundle> bundle);
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> Why not just UpdatePolicy()?

Done.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/mock...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/mock...
chrome/browser/policy/mock_configuration_policy_provider.h:29: //
PolicyDirectory that maps the Chrome namespace to a copy of |policy|.
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> Bundle

Done.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:97: if (it == registrars_.end())
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> NOTREACHED()? Or can this situation actually happen?

GetRegistrar() already has a NOTREACHED() for this situation, but this method
has to return when that happens in release mode.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:107: for (RegistrarList::iterator
it = registrars_.begin();
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> std::find? Also, how about making this a std::map<Provider*, Registrar*>?

std::find() doesn't quite work, because what is being searched for isn't a value
contained in |registrars_| but a value whose ->provider() equals |provider|.
This could be done with std::find_if and a special predicate but I don't think
it'd improve readability.

|registrars_| is also in decreasing order of priority, and it must be preserved
so that merges take the right priorities into account. That's why it isn't a
map.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
On 2012/05/14 09:07:21, Mattias Nissler wrote:
> Can we have a unit test for this?

This is covered by unit tests in policy_service_unittest.cc:

- NotifyObservers tests that the right notifications are triggered
- Priorities tests that the merging takes the providers' priorities into account
- RefreshPolicies tests that the refresh callbacks are invoked at the right time

Do you have some other test in mind?

[Mattias Nissler (ping if slow), 2012-05-14 11:41:24]

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:97: if (it == registrars_.end())
On 2012/05/14 09:39:17, Joao da Silva wrote:
> On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > NOTREACHED()? Or can this situation actually happen?
> 
> GetRegistrar() already has a NOTREACHED() for this situation, but this method
> has to return when that happens in release mode.

OK.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:107: for (RegistrarList::iterator
it = registrars_.begin();
On 2012/05/14 09:39:17, Joao da Silva wrote:
> On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > std::find? Also, how about making this a std::map<Provider*, Registrar*>?
> 
> std::find() doesn't quite work, because what is being searched for isn't a
value
> contained in |registrars_| but a value whose ->provider() equals |provider|.
> This could be done with std::find_if and a special predicate but I don't think
> it'd improve readability.
> 
> |registrars_| is also in decreasing order of priority, and it must be
preserved
> so that merges take the right priorities into account. That's why it isn't a
> map.

I understand now.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
On 2012/05/14 09:39:17, Joao da Silva wrote:
> On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > Can we have a unit test for this?
> 
> This is covered by unit tests in policy_service_unittest.cc:
> 
> - NotifyObservers tests that the right notifications are triggered
> - Priorities tests that the merging takes the providers' priorities into
account
> - RefreshPolicies tests that the refresh callbacks are invoked at the right
time
> 
> Do you have some other test in mind?

I was referring to checking whether different namespaces are handled properly.
Can we have a test case for that?

Also, I didn't see these tests when checking because they are in
policy_service_unittest.cc, whereas I had expected
policy_service_impl_unittest.cc. Move? Maybe not in this CL though.

[Joao da Silva, 2012-05-14 12:53:30]

Please have yet another look :-)

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
On 2012/05/14 11:41:24, Mattias Nissler wrote:
> On 2012/05/14 09:39:17, Joao da Silva wrote:
> > On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > > Can we have a unit test for this?
> > 
> > This is covered by unit tests in policy_service_unittest.cc:
> > 
> > - NotifyObservers tests that the right notifications are triggered
> > - Priorities tests that the merging takes the providers' priorities into
> account
> > - RefreshPolicies tests that the refresh callbacks are invoked at the right
> time
> > 
> > Do you have some other test in mind?
> 
> I was referring to checking whether different namespaces are handled properly.
> Can we have a test case for that?

Merging different namespaces within a bundle is tested at
policy_bundle_unittest.cc. I've added a new test that merges policy from more
than one provider and into shared namespaces.

> 
> Also, I didn't see these tests when checking because they are in
> policy_service_unittest.cc, whereas I had expected
> policy_service_impl_unittest.cc. Move? Maybe not in this CL though.

Renamed to policy_service_impl_unittest.cc.

[Mattias Nissler (ping if slow), 2012-05-14 13:59:37]

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
On 2012/05/14 12:53:30, Joao da Silva wrote:
> On 2012/05/14 11:41:24, Mattias Nissler wrote:
> > On 2012/05/14 09:39:17, Joao da Silva wrote:
> > > On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > > > Can we have a unit test for this?
> > > 
> > > This is covered by unit tests in policy_service_unittest.cc:
> > > 
> > > - NotifyObservers tests that the right notifications are triggered
> > > - Priorities tests that the merging takes the providers' priorities into
> > account
> > > - RefreshPolicies tests that the refresh callbacks are invoked at the
right
> > time
> > > 
> > > Do you have some other test in mind?
> > 
> > I was referring to checking whether different namespaces are handled
properly.
> > Can we have a test case for that?
> 
> Merging different namespaces within a bundle is tested at
> policy_bundle_unittest.cc. I've added a new test that merges policy from more
> than one provider and into shared namespaces.
> 
> > 
> > Also, I didn't see these tests when checking because they are in
> > policy_service_unittest.cc, whereas I had expected
> > policy_service_impl_unittest.cc. Move? Maybe not in this CL though.
> 
> Renamed to policy_service_impl_unittest.cc.

I actually meant testing the notification generation below.

http://codereview.chromium.org/10386097/diff/13024/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_unittest.cc (right):

http://codereview.chromium.org/10386097/diff/13024/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_unittest.cc:54: PolicyLevel level,
PolicyScope scope) {
Individual lines for parameter declarations. bundle and value can stay on the
same per the rules, but level and scope should not be on the same line IMHO.

[Joao da Silva, 2012-05-14 15:26:51]

Moar tests.

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl.cc (right):

http://codereview.chromium.org/10386097/diff/11001/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl.cc:129: void
PolicyServiceImpl::MergeAndTriggerUpdates() {
On 2012/05/14 13:59:37, Mattias Nissler wrote:
> On 2012/05/14 12:53:30, Joao da Silva wrote:
> > On 2012/05/14 11:41:24, Mattias Nissler wrote:
> > > On 2012/05/14 09:39:17, Joao da Silva wrote:
> > > > On 2012/05/14 09:07:21, Mattias Nissler wrote:
> > > > > Can we have a unit test for this?
> > > > 
> > > > This is covered by unit tests in policy_service_unittest.cc:
> > > > 
> > > > - NotifyObservers tests that the right notifications are triggered
> > > > - Priorities tests that the merging takes the providers' priorities into
> > > account
> > > > - RefreshPolicies tests that the refresh callbacks are invoked at the
> right
> > > time
> > > > 
> > > > Do you have some other test in mind?
> > > 
> > > I was referring to checking whether different namespaces are handled
> properly.
> > > Can we have a test case for that?
> > 
> > Merging different namespaces within a bundle is tested at
> > policy_bundle_unittest.cc. I've added a new test that merges policy from
more
> > than one provider and into shared namespaces.
> > 
> > > 
> > > Also, I didn't see these tests when checking because they are in
> > > policy_service_unittest.cc, whereas I had expected
> > > policy_service_impl_unittest.cc. Move? Maybe not in this CL though.
> > 
> > Renamed to policy_service_impl_unittest.cc.
> 
> I actually meant testing the notification generation below.

Ah got it, I misunderstood what you meant. Added a new test for that.

http://codereview.chromium.org/10386097/diff/13024/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_unittest.cc (right):

http://codereview.chromium.org/10386097/diff/13024/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_unittest.cc:54: PolicyLevel level,
PolicyScope scope) {
On 2012/05/14 13:59:37, Mattias Nissler wrote:
> Individual lines for parameter declarations. bundle and value can stay on the
> same per the rules, but level and scope should not be on the same line IMHO.

Done.

[Mattias Nissler (ping if slow), 2012-05-15 09:40:33]

Looking good, just some nits to fix before final approval

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl_unittest.cc (right):

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:206:
POLICY_DOMAIN_EXTENSIONS, kExtension0, &extension0_observer);
I think this would become more readable if you put the line break after
kExtension0 (also below).

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:223:
OnPolicyUpdated(POLICY_DOMAIN_CHROME, "", _, _));
add a matcher for checking the correct PolicyMap gets passed?

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:258:
policy_service_->RemoveObserver(
same comment regarding line breaking.

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:434: bundle0.get(),
"bundle0", POLICY_LEVEL_RECOMMENDED, POLICY_SCOPE_USER);
break line after second parameter?

[Joao da Silva, 2012-05-15 13:06:31]

PTAL

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
File chrome/browser/policy/policy_service_impl_unittest.cc (right):

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:206:
POLICY_DOMAIN_EXTENSIONS, kExtension0, &extension0_observer);
On 2012/05/15 09:40:33, Mattias Nissler wrote:
> I think this would become more readable if you put the line break after
> kExtension0 (also below).

Done.

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:223:
OnPolicyUpdated(POLICY_DOMAIN_CHROME, "", _, _));
On 2012/05/15 09:40:33, Mattias Nissler wrote:
> add a matcher for checking the correct PolicyMap gets passed?

Done.

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:258:
policy_service_->RemoveObserver(
On 2012/05/15 09:40:33, Mattias Nissler wrote:
> same comment regarding line breaking.

Done.

http://codereview.chromium.org/10386097/diff/13053/chrome/browser/policy/poli...
chrome/browser/policy/policy_service_impl_unittest.cc:434: bundle0.get(),
"bundle0", POLICY_LEVEL_RECOMMENDED, POLICY_SCOPE_USER);
On 2012/05/15 09:40:33, Mattias Nissler wrote:
> break line after second parameter?

Done.

[Mattias Nissler (ping if slow), 2012-05-15 15:18:50]

LGTM. Thanks for bearing with me :)

[Joao da Silva, 2012-05-15 15:20:57]

On 2012/05/15 15:18:50, Mattias Nissler wrote:
> LGTM. Thanks for bearing with me :)

Thank _you_ for the extension review!

[commit-bot: I haz the power, 2012-05-15 15:21:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10386097/9004

[commit-bot: I haz the power, 2012-05-15 15:21:42]

Presubmit check for 10386097-9004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 3.0s to calculate.

[Joao da Silva, 2012-05-15 16:19:19]

@jhawkins: please have a look at the webui/ changes. Thanks!

[James Hawkins, 2012-05-15 16:33:32]

LGTM with nits.

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
chrome/browser/ui/webui/policy_ui.cc:196: scoped_ptr<base::ListValue> list(
nit: No need to use a scoped_ptr here.

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
chrome/browser/ui/webui/policy_ui.cc:198:
service->GetPolicies(policy::POLICY_DOMAIN_CHROME, ""),
nit: "" should either be string16() or std::string() depending on the type of
the paramter.

[Joao da Silva, 2012-05-15 16:42:17]

Thanks for the quick review!

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
File chrome/browser/ui/webui/policy_ui.cc (right):

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
chrome/browser/ui/webui/policy_ui.cc:196: scoped_ptr<base::ListValue> list(
On 2012/05/15 16:33:32, James Hawkins wrote:
> nit: No need to use a scoped_ptr here.

Done.

http://codereview.chromium.org/10386097/diff/9004/chrome/browser/ui/webui/pol...
chrome/browser/ui/webui/policy_ui.cc:198:
service->GetPolicies(policy::POLICY_DOMAIN_CHROME, ""),
On 2012/05/15 16:33:32, James Hawkins wrote:
> nit: "" should either be string16() or std::string() depending on the type of
> the paramter.

Done.

[commit-bot: I haz the power, 2012-05-15 19:11:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10386097/7035

[commit-bot: I haz the power, 2012-05-15 21:18:08]

Change committed as 137244