Include only policy definitions that apply to the platfrom in the policy definition list.

Include only policy definitions that apply to the platfrom in the policy definition list.

BUG=chromium-os:19411
TEST=compiles, unit tests pass.


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

[Mattias Nissler (ping if slow), 2011-11-07 10:50:07]

Joao, I've moved the policy definition list changes and test cleanup to this
separate CL for your reviewing pleasure ;)

[Joao da Silva, 2011-11-07 13:06:17]

Do you want to make the same changes to
configuration_policy_provider_mac_unittest.cc and _win_unittest.cc?

The mac bots failed one of the tests. ApplicationLocaleValue is a win-only
policy, but is in the _mac_unittest.cc list, so it should be removed. It started
failing now because the CL doesn't include win-only policies on the mac build
too anymore, which is cool :-)

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:9: #include
"base/path_service.h"
Nit: not used

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:137:
base::FundamentalValue test_value("string_value");
Did you mean a StringValue here?

[Mattias Nissler (ping if slow), 2011-11-09 14:37:27]

My dear Mr. da Silva, care to take another look? If rewritten most of the policy
provider test code.

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:9: #include
"base/path_service.h"
On 2011/11/07 13:06:17, Joao da Silva wrote:
> Nit: not used

Done.

http://codereview.chromium.org/8467011/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider_unittest.cc:137:
base::FundamentalValue test_value("string_value");
On 2011/11/07 13:06:17, Joao da Silva wrote:
> Did you mean a StringValue here?

Of course I did. Good catch! Pointers shouldn't be implicitly convertable to
integers...

[Joao da Silva, 2011-11-09 15:37:47]

That was a lot of work! Looks good, ready to stamp after the CHECK issue and the
failing test are fixed.

I'd run the CL through the linux_valgrind bot too, since TestWithParam does
funny things with its argument, such as printing the raw bytes. I wonder if it
does the right thing with function pointers.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_mac_unittest.cc:73:
CFNumberCreate(NULL, kCFNumberIntType, &policy_value),
Nit: indent (caused by extra space after prefs_)

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_test.cc:25: extern const
ConfigurationPolicyType kPolicyString =
Nit: remove extern here and on the 3 next too

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_test.cc:34: static const
PolicyDefinitionList::Entry kEntries[] = {
Nit: not sure if we prefer an anonymous namespace instead of static. It's up to
you :-)

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_win_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:19: using
content::BrowserThread;
Nit: not needed

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:111:
CHECK_EQ(ERROR_SUCCESS, result);
CHECK failure will crash the unit_test process. It'd be better to just ASSERT
and make only this test fail.

http://codereview.chromium.org/8467011/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8467011/diff/5001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1469:
'browser/policy/configuration_policy_provider_test.h',
Nit: not sure if in this case this goes here, or after _mac_unittest.cc.

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

New version is up for review.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_mac_unittest.cc:73:
CFNumberCreate(NULL, kCFNumberIntType, &policy_value),
On 2011/11/09 15:37:47, Joao da Silva wrote:
> Nit: indent (caused by extra space after prefs_)

Done.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_test.cc:25: extern const
ConfigurationPolicyType kPolicyString =
On 2011/11/09 15:37:47, Joao da Silva wrote:
> Nit: remove extern here and on the 3 next too

Done.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_test.cc:34: static const
PolicyDefinitionList::Entry kEntries[] = {
On 2011/11/09 15:37:47, Joao da Silva wrote:
> Nit: not sure if we prefer an anonymous namespace instead of static. It's up
to
> you :-)

I think the additional noise generated by the anonymous namespace is detrimental
if we're talking about just a single declaration.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_win_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:19: using
content::BrowserThread;
On 2011/11/09 15:37:47, Joao da Silva wrote:
> Nit: not needed

Done.

http://codereview.chromium.org/8467011/diff/5001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:111:
CHECK_EQ(ERROR_SUCCESS, result);
On 2011/11/09 15:37:47, Joao da Silva wrote:
> CHECK failure will crash the unit_test process. It'd be better to just ASSERT
> and make only this test fail.

I figured out something that works :)

http://codereview.chromium.org/8467011/diff/5001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8467011/diff/5001/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1469:
'browser/policy/configuration_policy_provider_test.h',
On 2011/11/09 15:37:47, Joao da Silva wrote:
> Nit: not sure if in this case this goes here, or after _mac_unittest.cc.

Done.

[Joao da Silva, 2011-11-09 18:02:40]

LGTM

http://codereview.chromium.org/8467011/diff/5003/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_win_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5003/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:89: void
DeleteRegistrySandbox();
Nit: remove this 3 dead declarations

[Mattias Nissler (ping if slow), 2011-11-09 18:06:05]

Thanks for the review, I'll submit this tomorrow.

http://codereview.chromium.org/8467011/diff/5003/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_provider_win_unittest.cc
(right):

http://codereview.chromium.org/8467011/diff/5003/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_provider_win_unittest.cc:89: void
DeleteRegistrySandbox();
On 2011/11/09 18:02:40, Joao da Silva wrote:
> Nit: remove this 3 dead declarations

Done.

[commit-bot: I haz the power, 2011-11-10 09:09:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/8467011/5004

[commit-bot: I haz the power, 2011-11-10 10:08:24]

Change committed as 109405