Removed ConfigurationPolicyProvider::Provide().

Removed ConfigurationPolicyProvider::Provide().

Refactored tests to use ConfigurationPolicyProvider::policies(), and all the
providers to load PolicyBundles.

BUG=108993
TEST=Nothing breaks, unit_tests green


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

[Joao da Silva, 2012-05-14 09:59:57]

This is the 2nd half after http://codereview.chromium.org/10386097/. The
remaining changes after these two should be more self-contained :-)

Please review, thanks!

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

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_loader_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:69:
EXPECT_CALL(*delegate, MockLoad()).WillOnce(Return(result));
It feels brittle to pass a pointer that's to-be-owned by the caller of the
function eventually. If you have the MockLoad() helper, can't you just make the
copy in MockLoad()? That will also avoid the need for two copies of the mock
here. Then again, that might be hairy when using actions.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:39: EXPECT_EQ(1U,
policies.size());
Might want to check that other namespaces are empty here.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:63:
expected.Equals(provider.policies().Get(POLICY_DOMAIN_CHROME, "")));
Would be nice if we could just compare the policy bundle here (and below).

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider.cc:71: // Class
ConfigurationPolicyProvider.
If you add that here, you probably want another comment at line 65.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:63: virtual
PolicyScope GetPolicyScope() const = 0;
Minor style nit you may choose to ignore: Might as well pass these as
constructor parameters and have const accessors. The rest of the functions here
are actually meant to "take an action".

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
chrome/browser/policy/file_based_policy_provider_unittest.cc:39:
MOCK_METHOD0(MockLoad, PolicyBundle*());
Should document that this actually needs to return ownership as well. Or as
noted before make it such that we don't need to transfer ownership, which makes
things easier in case you want something to call this function multiple times.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
chrome/browser/policy/file_based_policy_provider_unittest.cc:67:
EXPECT_TRUE(provider.policies().Equals(expected_bundle));
And if you were to avoid passing ownership, you could just test against |bundle|
here.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:16: bool BundleEntryEquals(const
PolicyBundle::MapType::value_type& a,
Unused?

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:121: }
return false? Could as well put the statement in line 119 here.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:136: void PolicyBundle::Dump() const {
Remove?

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle.h (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.h:64: // XXX TODO: remove
so?

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle_unittest.cc:210: .Set(kPolicy0,
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
4 spaces indentation

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle_unittest.cc:216: .Set(kPolicy0,
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
same here

[Joao da Silva, 2012-05-15 13:07:01]

PTAL

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_loader_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_loader_unittest.cc:69:
EXPECT_CALL(*delegate, MockLoad()).WillOnce(Return(result));
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> It feels brittle to pass a pointer that's to-be-owned by the caller of the
> function eventually. If you have the MockLoad() helper, can't you just make
the
> copy in MockLoad()? That will also avoid the need for two copies of the mock
> here. Then again, that might be hairy when using actions.

I wanted to keep MockLoad() similar to the function it is mocking, just without
the scoped_ptr. The code is cleaner when MockLoad() creates a copy though, so
I've made this change as suggested.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:39: EXPECT_EQ(1U,
policies.size());
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Might want to check that other namespaces are empty here.

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/asyn...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:63:
expected.Equals(provider.policies().Get(POLICY_DOMAIN_CHROME, "")));
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Would be nice if we could just compare the policy bundle here (and below).

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider.cc:71: // Class
ConfigurationPolicyProvider.
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> If you add that here, you probably want another comment at line 65.

I didn't even notice this :-) This was the original code, and the change I made
was to move FixDeprecatedPolicies() to the anonymous namespace. This comment has
been removed.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:63: virtual
PolicyScope GetPolicyScope() const = 0;
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Minor style nit you may choose to ignore: Might as well pass these as
> constructor parameters and have const accessors. The rest of the functions
here
> are actually meant to "take an action".

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
chrome/browser/policy/file_based_policy_provider_unittest.cc:39:
MOCK_METHOD0(MockLoad, PolicyBundle*());
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Should document that this actually needs to return ownership as well. Or as
> noted before make it such that we don't need to transfer ownership, which
makes
> things easier in case you want something to call this function multiple times.

Modified as suggested before.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/file...
chrome/browser/policy/file_based_policy_provider_unittest.cc:67:
EXPECT_TRUE(provider.policies().Equals(expected_bundle));
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> And if you were to avoid passing ownership, you could just test against
|bundle|
> here.

Yep, done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:16: bool BundleEntryEquals(const
PolicyBundle::MapType::value_type& a,
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Unused?

Yes. Equals() was initially implemented as std::equal with this predicate, but
then it failed for the reason explained in the comment there.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:121: }
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> return false? Could as well put the statement in line 119 here.

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.cc:136: void PolicyBundle::Dump() const {
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> Remove?

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle.h (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle.h:64: // XXX TODO: remove
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> so?

omgwtfbbq

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_bundle_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle_unittest.cc:210: .Set(kPolicy0,
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> 4 spaces indentation

Done.

http://codereview.chromium.org/10384145/diff/13001/chrome/browser/policy/poli...
chrome/browser/policy/policy_bundle_unittest.cc:216: .Set(kPolicy0,
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
On 2012/05/14 16:55:59, Mattias Nissler wrote:
> same here

Done.

[Mattias Nissler (ping if slow), 2012-05-15 17:07:23]

LGTM with nits.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:46: EXPECT_EQ(it,
provider_bundle.end());
We can probably replace this block by
EXPECT_TRUE(bundle.Equals(provider.policies());

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_test_base.h (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:35:
bundle->CopyFrom(*loaded);
nit: Can we return NULL if loaded == NULL?

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_test.h:64: virtual
PolicyLevel GetPolicyLevel() const;
no need to make these virtual any longer, and the should be unix_hacker_style as
well.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:38:
bundle->CopyFrom(*loaded);
same comment as before.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_bundle.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
chrome/browser/policy/policy_bundle.cc:96: for (;;) {
not sure what the style rule for this is, but while (true) wins the git grep
count over for (;;) ;)

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
chrome/browser/policy/policy_bundle.cc:102: if (it_this != end() && it_other !=
other.end()) {
It's probably easier to read this if you were to put here:

if (it_this == end() && it_other == other.end())
  break;

and then un-indent the if branch.

[Joao da Silva, 2012-05-16 10:25:44]

Thanks for the  review. I'll land if the bots are green and
http://codereview.chromium.org/10377115/ lands.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_provider_unittest.cc:46: EXPECT_EQ(it,
provider_bundle.end());
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> We can probably replace this block by
> EXPECT_TRUE(bundle.Equals(provider.policies());

Done.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
File chrome/browser/policy/asynchronous_policy_test_base.h (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/async...
chrome/browser/policy/asynchronous_policy_test_base.h:35:
bundle->CopyFrom(*loaded);
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> nit: Can we return NULL if loaded == NULL?

Yes, it will be passed along to ConfigurationPolicyProvider::UpdatePolicy()
which handles it correctly. Done.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_test.h:64: virtual
PolicyLevel GetPolicyLevel() const;
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> no need to make these virtual any longer, and the should be unix_hacker_style
as
> well.

Done.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:38:
bundle->CopyFrom(*loaded);
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> same comment as before.

Done.

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_bundle.cc (right):

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
chrome/browser/policy/policy_bundle.cc:96: for (;;) {
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> not sure what the style rule for this is, but while (true) wins the git grep
> count over for (;;) ;)

I find this style cute:

#define ever ;;

for (ever) { ... }

Changed to while(true) :-)

http://codereview.chromium.org/10384145/diff/8007/chrome/browser/policy/polic...
chrome/browser/policy/policy_bundle.cc:102: if (it_this != end() && it_other !=
other.end()) {
On 2012/05/15 17:07:24, Mattias Nissler wrote:
> It's probably easier to read this if you were to put here:
> 
> if (it_this == end() && it_other == other.end())
>   break;
> 
> and then un-indent the if branch.

Done. I think you meant that the test for the break is

if (it_this == end() || it_other == other.end())

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

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

[commit-bot: I haz the power, 2012-05-16 13:08:54]

Change committed as 137409