Create additional PrefStore for Device Management policy.

Create additional PrefStore for Device Management policy.

BUG=62478
TEST=PrefValueStore*:PrefService*

Committing for danno@chromium.org

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

[danno, 2010-11-12 12:40:37]

please review.

[Mattias Nissler (ping if slow), 2010-11-12 15:02:13]

Pretty close, a couple of nits mostly.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:166: // policy refresh.  |callback| is
called on the caller's thread as a Task
nit: This file doesn't seem to use 2 spaces after periods.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:177: //   |managed_prefs| contains all
managed platform (non-cloud policy)
should be managed_platfrom_prefs

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:44: const std::string
kSearchProviderName = "Chromium";
the idea of these tests is that this one should also be present in all
lower-priority stores, but with different values.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:304:
EXPECT_EQ(managed_platform_pref::kHomepageValue, actual_str_value);
Here you should insert a test case that tests the cloud policy value overrides
all lower-priority values.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:635:
EXPECT_FALSE(pref_value_store_->PrefValueInExtensionStore(
should be PrefValueInUserStore

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:721: // Ensure that
Ensure what?

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:722:
TEST_F(PrefValueStoreTest, TestPlatformTrumpsDeviceManagementPolicy) {
I believe this test would be covered by the missing one I noted above.

http://codereview.chromium.org/4876002/diff/25002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:82: PrefStore* CreatePrefStoreFromProvider(
Maybe we should name this CreatePolicyPrefStoreFromProvider?

[danno, 2010-11-12 16:17:30]

addressed feedback

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:166: // policy refresh.  |callback| is
called on the caller's thread as a Task
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> nit: This file doesn't seem to use 2 spaces after periods.

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.h:177: //   |managed_prefs| contains all
managed platform (non-cloud policy)
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> should be managed_platfrom_prefs

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:44: const std::string
kSearchProviderName = "Chromium";
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> the idea of these tests is that this one should also be present in all
> lower-priority stores, but with different values.

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:304:
EXPECT_EQ(managed_platform_pref::kHomepageValue, actual_str_value);
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> Here you should insert a test case that tests the cloud policy value overrides
> all lower-priority values.

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:635:
EXPECT_FALSE(pref_value_store_->PrefValueInExtensionStore(
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> should be PrefValueInUserStore

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:721: // Ensure that
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> Ensure what?

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store_unittest.cc:722:
TEST_F(PrefValueStoreTest, TestPlatformTrumpsDeviceManagementPolicy) {
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> I believe this test would be covered by the missing one I noted above.

Done.

http://codereview.chromium.org/4876002/diff/25002/chrome/test/testing_pref_se...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/4876002/diff/25002/chrome/test/testing_pref_se...
chrome/test/testing_pref_service.h:82: PrefStore* CreatePrefStoreFromProvider(
On 2010/11/12 15:02:13, Mattias Nissler wrote:
> Maybe we should name this CreatePolicyPrefStoreFromProvider?

Done.

[Mattias Nissler (ping if slow), 2010-11-12 16:25:21]

lgtm.