Sanitize PrefStore interface.

Sanitize PrefStore interface.

This reworks the PrefStore interface, specifically:

- Split up the interface into PrefStore, which only provides reading functionality, and the derived PersistentPrefStore for the actual user pref store
- Remove the hurt-me-plenty prefs() function from PrefStore, instead provide Get/Set/Remove operations
- Remove special handling of default and user pref store from PrefValueStore and put it into PrefService
- Pref change notification handling now almost exclusively handled through PrefValueStore
- Adjust all consumers of these interfaces (but keep ConfigurationPolicyPrefStore untouched, that's up next on the list)

BUG=64232
TEST=existing unit tests

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

[Mattias Nissler (ping if slow), 2010-12-07 16:07:47]

Please have a look. I know it's a lot, sorry.

[Paweł Hajdan Jr., 2010-12-07 17:30:00]

Drive-by.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:149: #if 0
Please DISABLE the test if needed. Otherwise bit rot is extremely likely to
happen. By the way, all disabled tests should have a bug with Tests-Disabled
label.

[Mattias Nissler (ping if slow), 2010-12-08 09:22:38]

+gfeher (FYI)

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:149: #if 0
On 2010/12/07 17:30:00, Paweł Hajdan Jr. wrote:
> Please DISABLE the test if needed. Otherwise bit rot is extremely likely to
> happen. By the way, all disabled tests should have a bug with Tests-Disabled
> label.

Actually, I intended to temporary disable them for development, since I feel
these tests do not really belong here. They are really proxy policy integration
tests, not PrefService unit tests. Danno, should we just remove them or move the
code somewhere else?

[danno, 2010-12-08 09:32:49]

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:150: TEST(PrefServiceTest,
ProxyFromCommandLineNotPolicy) {
This test can be removed. It is covered by the CommandLinePrefStoreTests.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:162: TEST(PrefServiceTest,
ProxyPolicyOverridesCommandLineOptions) {
This test shouldn't be removed, it should be re-written. It's still important:
AFAIK, nowhere else in the code do we tests that setting a managed proxy policy
automatically sets non-affected proxy preference to their default value.

[battre (please use the other), 2010-12-08 12:24:15]

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:17: // Set an extension
preference |value| for |key|. Takes ownership of |value|.
Does passing value=NULL have the connotation of a Remove?

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:16: bool PrefValueMap::GetValue(const
std::string& key, Value** value) const {
DCHECK(value)?
or: if value == NULL, don't write but still return true/false depending on
whether the variable exists

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.h:28: // the value changed.
Has value==NULL connotation of RemoveValue?

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:249: *out_value = NULL;
Several getters like DictionaryValue::GetWithoutPathExpansion seem to have the
semantic "if value cannot be found, do not touch |out_value|".

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
File chrome/browser/prefs/testing_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:56: bool GetString(const std::string&
key, std::string* value);
const? maybe not because it is a testing class? - same below

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:58: class ExtensionPrefStore;
used?

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:48: virtual void SetValueSilently(const
std::string& key, Value* value) = 0;
value==NULL is remove?

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:25: class ObserverInterface {
Observer instead of ObserverInterface?

grep -r Observer chrome/ | wc -l
5231
grep -r ObserverInterface chrome/ | wc -l
0

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:39: // No value present, but skip other pref stores
and use default.
I don't understand the "but skip other pref stores" part. Is this an instruction
to the caller? How would this be decided? Is this part of a policy that says
"use this value / use no value (override by NULL) / use what ever somebody
defines"?

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:56: // Ownership remains with the PrefStore.
Does not modify |result| if the key does not exist.

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
chrome/test/testing_pref_service.h:41: const Value* GetPref(TestingPrefStore*
pref_store, const char* path);
Any reason why this has to be a TestingPrefStore instead of a PrefStore?

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
chrome/test/testing_pref_service.h:41: const Value* GetPref(TestingPrefStore*
pref_store, const char* path);
const? maybe not because it is a Testing class

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
chrome/test/testing_pref_service.h:44: void SetPref(TestingPrefStore*
pref_store, const char* path, Value* value);
Any reason why this has to be a TestingPrefStore instead of a persistent
PrefStore?

[gfeher, 2010-12-08 12:34:35]

Just some random notes.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:327: &changed_managed_platform_paths);
Please indent.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:42: // into the user pref store. Using
SetValue is not an options since existing
s/options/option/

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs.cc
File chrome/service/service_prefs.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs...
chrome/service/service_prefs.cc:35: value->GetAsBoolean(result);
How about indicating missing prefs to the consumer in a more explicit way, like
with a bool return value?

I admit I don't yet understand the role of ServicePrefs.

[danno, 2010-12-08 13:08:45]

a few issues and lots of nits.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:20: void
ExtensionPrefStore::OnInitializationCompleted() {
DCHECK that it's not already initialized?

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:28: virtual bool
IsInitializationComplete();
should this guy be const? (superclass def, too)

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:56: preferences_file,
indenting

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service_unittest.cc:250: class
PrefServiceMock : public PrefService {
Here it's called a PrefServiceMock, in test_extension_prefs it's a
MockPrefService. Perhaps consistency in names and slightly more descriptive
ones, even if they are in different namespaces?

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:473: prefs_->Set(*i,
Value::CreateNullValue());
Through comment or well named method it should be clear why it's OK to use the
Null in this store as a sentinel.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_ch...
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_ch...
chrome/browser/prefs/pref_change_registrar_unittest.cc:21: class MockPrefService
: public TestingPrefService {
Choice of mock name if you clean up the other Mock/PrefService/Mock.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:162: PersistentPrefStore::PrefReadError
error = user_pref_store_->ReadPrefs();
const

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:17: Map::const_iterator entry =
prefs_.find(key);
const

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:29: Map::iterator entry =
prefs_.find(key);
const

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:43: Map::iterator entry =
prefs_.find(key);
const

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.h:20: ~PrefValueMap();
virtual just in case?

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store_unittest.cc:11: #include
"chrome/browser/policy/dummy_configuration_policy_provider.h"
alphabetize

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
File chrome/browser/prefs/testing_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:13: #include
"chrome/browser/prefs/pref_value_map.h"
alphabetize

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:17: // |TestingPrefStore| is a stub
implementation of the |PrefStore| interface.
|PersistentPrefStore|, but how about just:
|TestingPrefStore| is a preference store that allows tests to explicitly
manipulate the contents of the store, triggering notifications where
appropriate.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:26: virtual ReadResult GetValue(const
std::string& key, Value** result) const;
Since GetValue is inherited from PersistentPrefStore's parent, is it misleading
to list it under "PrefStore overrides"? Especially since you treat
AddObserver/RemoveObserver differently.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:63: virtual void
RemoveObserver(PrefStore::ObserverInterface* observer);
Why are these made private? Seems like for testing you would like to keep them
public.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/value_m...
File chrome/browser/prefs/value_map_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/value_m...
chrome/browser/prefs/value_map_pref_store.h:14: #include
"chrome/browser/prefs/pref_value_map.h"
alpha

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:58: class ExtensionPrefStore;
Doesn't look like this actually gets used in this file.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_model.cc:847: if (!id_string.empty()
&& !*is_managed) {
does this really belong in this CL?

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:15: #include
"chrome/common/persistent_pref_store.h"
alpha

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:49: // Overriden from PrefStore.
Why do you make these private? By upcasting you can always call them, so I don't
see the utility.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:13: // This interface is a complement to
the PrefStore interface, declaring
s/a complement/complimentary/

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs.h
File chrome/service/service_prefs.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs...
chrome/service/service_prefs.h:13: // Manages persistent preferences for the
service process. This is basically a
This should probably be called ServiceProcessPrefs. Service can be confused with
PrefService.

[danno, 2010-12-08 13:20:59]

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:13: // This interface is a complement to
the PrefStore interface, declaring
Whoops, sorry, should be complementary :-)
On 2010/12/08 13:08:45, danno wrote:
> s/a complement/complimentary/

[Mattias Nissler (ping if slow), 2010-12-09 10:20:20]

Answered/addressed all your comments. Please take another look at the updated
code. Thanks!

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.cc:20: void
ExtensionPrefStore::OnInitializationCompleted() {
On 2010/12/08 13:08:45, danno wrote:
> DCHECK that it's not already initialized?

It doesn't really hurt to call this twice, but I don't feel strong and put the
DCHECK anyway.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:17: // Set an extension
preference |value| for |key|. Takes ownership of |value|.
On 2010/12/08 12:24:15, battre wrote:
> Does passing value=NULL have the connotation of a Remove?

No. Actually, it DCHECKs.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_pref_store.h:28: virtual bool
IsInitializationComplete();
On 2010/12/08 13:08:45, danno wrote:
> should this guy be const? (superclass def, too)

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/te...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/extensions/te...
chrome/browser/extensions/test_extension_prefs.cc:56: preferences_file,
On 2010/12/08 13:08:45, danno wrote:
> indenting

This was actually intentionall to make it clear these are arguments to the
constructor (otherwise they'd align with the constructor name). I can change it
if you insist, but I find this more readable.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/net/pref_prox...
File chrome/browser/net/pref_proxy_config_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/net/pref_prox...
chrome/browser/net/pref_proxy_config_service_unittest.cc:250: class
PrefServiceMock : public PrefService {
On 2010/12/08 13:08:45, danno wrote:
> Here it's called a PrefServiceMock, in test_extension_prefs it's a
> MockPrefService. Perhaps consistency in names and slightly more descriptive
> ones, even if they are in different namespaces?

Solution: Throw design patterns at it :) I introduced PrefServiceMockBuilder
which solves the problem of creating customized PrefService instances for tests.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_pref_store.cc:473: prefs_->Set(*i,
Value::CreateNullValue());
On 2010/12/08 13:08:45, danno wrote:
> Through comment or well named method it should be clear why it's OK to use the
> Null in this store as a sentinel.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_ch...
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_ch...
chrome/browser/prefs/pref_change_registrar_unittest.cc:21: class MockPrefService
: public TestingPrefService {
On 2010/12/08 13:08:45, danno wrote:
> Choice of mock name if you clean up the other Mock/PrefService/Mock.

Doesn't apply any longer.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service.cc:162: PersistentPrefStore::PrefReadError
error = user_pref_store_->ReadPrefs();
On 2010/12/08 13:08:45, danno wrote:
> const

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:150: TEST(PrefServiceTest,
ProxyFromCommandLineNotPolicy) {
On 2010/12/08 09:32:50, danno wrote:
> This test can be removed. It is covered by the CommandLinePrefStoreTests.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_se...
chrome/browser/prefs/pref_service_unittest.cc:162: TEST(PrefServiceTest,
ProxyPolicyOverridesCommandLineOptions) {
On 2010/12/08 09:32:50, danno wrote:
> This test shouldn't be removed, it should be re-written. It's still important:
> AFAIK, nowhere else in the code do we tests that setting a managed proxy
policy
> automatically sets non-affected proxy preference to their default value.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:16: bool PrefValueMap::GetValue(const
std::string& key, Value** value) const {
On 2010/12/08 12:24:15, battre wrote:
> DCHECK(value)?
> or: if value == NULL, don't write but still return true/false depending on
> whether the variable exists

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:17: Map::const_iterator entry =
prefs_.find(key);
On 2010/12/08 13:08:45, danno wrote:
> const

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:29: Map::iterator entry =
prefs_.find(key);
On 2010/12/08 13:08:45, danno wrote:
> const

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.cc:43: Map::iterator entry =
prefs_.find(key);
On 2010/12/08 13:08:45, danno wrote:
> const

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_map.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.h:20: ~PrefValueMap();
On 2010/12/08 13:08:45, danno wrote:
> virtual just in case?

Adds the vtable pointer when we really don't need it. But better safe than sorry
I guess. Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_map.h:28: // the value changed.
On 2010/12/08 12:24:15, battre wrote:
> Has value==NULL connotation of RemoveValue?

No, it DCHECKs. Added comment to that effect.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:249: *out_value = NULL;
On 2010/12/08 12:24:15, battre wrote:
> Several getters like DictionaryValue::GetWithoutPathExpansion seem to have the
> semantic "if value cannot be found, do not touch |out_value|".

But this doesn't and I don't want to change behavior here.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store.cc:327: &changed_managed_platform_paths);
On 2010/12/08 12:34:35, gfeher wrote:
> Please indent.

You mean correctly? ;)
Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
File chrome/browser/prefs/pref_value_store_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/pref_va...
chrome/browser/prefs/pref_value_store_unittest.cc:11: #include
"chrome/browser/policy/dummy_configuration_policy_provider.h"
On 2010/12/08 13:08:45, danno wrote:
> alphabetize

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
File chrome/browser/prefs/testing_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:13: #include
"chrome/browser/prefs/pref_value_map.h"
On 2010/12/08 13:08:45, danno wrote:
> alphabetize

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:17: // |TestingPrefStore| is a stub
implementation of the |PrefStore| interface.
On 2010/12/08 13:08:45, danno wrote:
> |PersistentPrefStore|, but how about just:
> |TestingPrefStore| is a preference store that allows tests to explicitly
> manipulate the contents of the store, triggering notifications where
> appropriate. 

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:26: virtual ReadResult GetValue(const
std::string& key, Value** result) const;
On 2010/12/08 13:08:45, danno wrote:
> Since GetValue is inherited from PersistentPrefStore's parent, is it
misleading
> to list it under "PrefStore overrides"? Especially since you treat
> AddObserver/RemoveObserver differently.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:56: bool GetString(const std::string&
key, std::string* value);
On 2010/12/08 12:24:15, battre wrote:
> const? maybe not because it is a testing class? - same below

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/testing...
chrome/browser/prefs/testing_pref_store.h:63: virtual void
RemoveObserver(PrefStore::ObserverInterface* observer);
On 2010/12/08 13:08:45, danno wrote:
> Why are these made private? Seems like for testing you would like to keep them
> public.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/value_m...
File chrome/browser/prefs/value_map_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/prefs/value_m...
chrome/browser/prefs/value_map_pref_store.h:14: #include
"chrome/browser/prefs/pref_value_map.h"
On 2010/12/08 13:08:45, danno wrote:
> alpha

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:58: class ExtensionPrefStore;
On 2010/12/08 13:08:45, danno wrote:
> Doesn't look like this actually gets used in this file.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/search_engine...
File chrome/browser/search_engines/template_url_model.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/browser/search_engine...
chrome/browser/search_engines/template_url_model.cc:847: if (!id_string.empty()
&& !*is_managed) {
On 2010/12/08 13:08:45, danno wrote:
> does this really belong in this CL?

If I don't do this, a unit test in template_url_model_unittest.cc crashes,
because it cannot cope with notifications being sent out incrementally instead
of after resetting all prefs.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:15: #include
"chrome/common/persistent_pref_store.h"
On 2010/12/08 13:08:45, danno wrote:
> alpha

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:49: // Overriden from PrefStore.
On 2010/12/08 13:08:45, danno wrote:
> Why do you make these private? By upcasting you can always call them, so I
don't
> see the utility.

I don't care really, but I thought consumers shouldn't be interested in calling
these anyway, so they're out of the way here. Moved them up now.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
File chrome/common/persistent_pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:13: // This interface is a complement to
the PrefStore interface, declaring
On 2010/12/08 13:21:00, danno wrote:
> Whoops, sorry, should be complementary :-)
> On 2010/12/08 13:08:45, danno wrote:
> > s/a complement/complimentary/
> 

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:42: // into the user pref store. Using
SetValue is not an options since existing
On 2010/12/08 12:34:35, gfeher wrote:
> s/options/option/

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/persistent_pre...
chrome/common/persistent_pref_store.h:48: virtual void SetValueSilently(const
std::string& key, Value* value) = 0;
On 2010/12/08 12:24:15, battre wrote:
> value==NULL is remove?

Added a comment saying that value must be non-NULL to SetValue().

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h
File chrome/common/pref_store.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:25: class ObserverInterface {
On 2010/12/08 12:24:15, battre wrote:
> Observer instead of ObserverInterface?
> 
> grep -r Observer chrome/ | wc -l
> 5231
> grep -r ObserverInterface chrome/ | wc -l
> 0

Well, fight with Danno over it, I don't really care.
Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:39: // No value present, but skip other pref stores
and use default.
On 2010/12/08 12:24:15, battre wrote:
> I don't understand the "but skip other pref stores" part. Is this an
instruction
> to the caller? How would this be decided? Is this part of a policy that says
> "use this value / use no value (override by NULL) / use what ever somebody
> defines"?

This kludge is here for proxy prefs and tells the PrefValueStore to return the
default value. So it is an instruction for the caller. It's up to the
implementor of the interface to decide when it wants to return this.

http://codereview.chromium.org/5646003/diff/2001/chrome/common/pref_store.h#n...
chrome/common/pref_store.h:56: // Ownership remains with the PrefStore.
On 2010/12/08 12:24:15, battre wrote:
> Does not modify |result| if the key does not exist.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs.cc
File chrome/service/service_prefs.cc (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs...
chrome/service/service_prefs.cc:35: value->GetAsBoolean(result);
On 2010/12/08 12:34:35, gfeher wrote:
> How about indicating missing prefs to the consumer in a more explicit way,
like
> with a bool return value?
> 
> I admit I don't yet understand the role of ServicePrefs.

If the consumers would need that. But so far they don't, so lets add it once we
need it.

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs.h
File chrome/service/service_prefs.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/service/service_prefs...
chrome/service/service_prefs.h:13: // Manages persistent preferences for the
service process. This is basically a
On 2010/12/08 13:08:45, danno wrote:
> This should probably be called ServiceProcessPrefs. Service can be confused
with
> PrefService.

Done.

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
chrome/test/testing_pref_service.h:41: const Value* GetPref(TestingPrefStore*
pref_store, const char* path);
On 2010/12/08 12:24:15, battre wrote:
> Any reason why this has to be a TestingPrefStore instead of a PrefStore?

Because we know it'll be a TestingPrefStore and for symmetry with SetPref and
RemovePref. Furthermore, this is private, so we can always adjust the interface
making local changes.

const: done.

http://codereview.chromium.org/5646003/diff/2001/chrome/test/testing_pref_ser...
chrome/test/testing_pref_service.h:44: void SetPref(TestingPrefStore*
pref_store, const char* path, Value* value);
On 2010/12/08 12:24:15, battre wrote:
> Any reason why this has to be a TestingPrefStore instead of a persistent
> PrefStore?

See the other comment.

[Nikita (slow), 2010-12-09 10:24:41]

Before you land this CL please make sure that user preference persistence is not
broken on cros TOT.

For me cros TOT version has such regression:
I'm unable to save for instance setting to enable screen lock (Settings >
Personal).
Haven't investigated it yet.

It would be hard to track regression down if this CL will be landed.

[danno, 2010-12-09 14:14:44]

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:182:
builder.WithManagedPlatformProvider(provider.get());
command line is missing.

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:219:
builder.WithManagedPlatformProvider(provider.get());
command line missing

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:254: scoped_ptr<PrefService>
prefs2(builder.Create());
please fix command line missing at all places in this file.

[Mattias Nissler (ping if slow), 2010-12-09 14:57:11]

Danno's comments fixed, please take another look.

@Nikita: Your problem is unrelated to this CL, I've filed
http://crbug.com/66045, which also has more information of what's wrong.

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:182:
builder.WithManagedPlatformProvider(provider.get());
On 2010/12/09 14:14:44, danno wrote:
> command line is missing.

Done.

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:219:
builder.WithManagedPlatformProvider(provider.get());
On 2010/12/09 14:14:44, danno wrote:
> command line missing

Done.

http://codereview.chromium.org/5646003/diff/35001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:254: scoped_ptr<PrefService>
prefs2(builder.Create());
On 2010/12/09 14:14:44, danno wrote:
> please fix command line missing at all places in this file.

Done.

[Aaron Boodman, 2010-12-21 18:18:58]

Is it now possible to have ExtensionPrefs directly implement PrefStore? And
instead of maintaining a DictionaryValue internally, just handle the
Get/Set/Remove calls directly?

[Mattias Nissler (ping if slow), 2010-12-21 20:43:33]

On 2010/12/21 18:18:58, Aaron Boodman wrote:
> Is it now possible to have ExtensionPrefs directly implement PrefStore? And
> instead of maintaining a DictionaryValue internally, just handle the
> Get/Set/Remove calls directly?

There is still the initialization order problem: We need the ExtensionPrefStore
to construct the PrefService, and ExtensionPrefs has a PrefService dependency.

A second issue is that Dominic is working on a CL that adds a separate
PrefService for incognito profiles: http://codereview.chromium.org/5915004/
This actually results in ExtensionPrefs driving two different
ExtensionPrefStores, one for the original profile, one for incognito.

Finally, if I understand you correctly, you propose to implement Get/Set/Remove
in ExtensionPrefs in a way that would manipulate the underlying extension pref
cache value on the fly through the PrefService interface. I think this approach
is a bit fragile, since we would then end up with GetValue() recursing: Client
code reads pref -> PrefService checks ExtensionPrefStore::GetValue() ->
ExtensionPrefs reads the extension pref value from the PrefService and we enter
the GetValue() path again. That doesn't mean we loop at this point (as long as
ExtensionPrefs doesn't override its own extension pref cache dictionaries), but
the reentrancy still bothers me.