Clean up pref change notification handling.

Clean up pref change notification handling.

This is a complete overhaul of PrefValueStore, PrefStore, PrefNotifier, and PrefService. Specifically:

- Add an observer interface to PrefStore that can be used to notify the upper layers of the pref system about value changes. Currently, it's unused mostly, but that'll change when we refactor ExtensionPrefStore and ConfigurationPolicyPrefStore.
- Make PrefNotifier be a dependency of PrefValueStore. That helps in keeping the pref change detection handling local to PrefValueStore.
- Clean up related unit tests, removing redundant mocks and gmockify others.

BUG=64893
TEST=Compiles and passes tests

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

[Mattias Nissler (ping if slow), 2010-12-01 18:14:04]

This is the current status of my refactoring effort. It's a lot of code because
I have so far failed to find a way to cut it down into smaller pieces...

[danno, 2010-12-02 10:31:52]

here's some stuff to chew on.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.h:22: class PrefNotifier : public
NonThreadSafe {
Make complete delegate interface that the PrefService implements as discussed.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:556:
pref_value_store_->PutUserPrefValue(path, dict);
Call this SetUserPrefValueWithoutNotification/Silent or something.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.h:122: Profile* profile);
How about making this protected and the testingprefservice can only calls
this/makes it public?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service_unittest.cc:39: MATCHER_P3(PrefValueMatches,
prefs, pref_name, value, "") {
Add comment please.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:186: NotifyPrefChanged(name,
USER_STORE);
The user preference store should send this. Add TODO.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:201: NotifyPrefChanged(name,
USER_STORE);
The user preference store should send this. Add TODO.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:512: void
PrefValueStore::CheckInitializationCompleted() {
Code is missing? This needs to be called by the Keeper. Add unit test.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:162: // PrefStores must be listed here
in order from highest to lowest priority.
Can these be private?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:266: void NotifyPrefChanged(const char*
path, PrefStoreType new_store);
OnPrefValueChanged?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:296: void PrefValueChangedInStore(const
std::string& key, PrefStoreType type);
Is this not the same as NotifyPrefChanged?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:299: void
InitializationCompleteForStore(PrefStoreType type);
OnInitializedCompleted?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:311: PrefStore*
GetPrefStore(PrefStoreType type);
inline?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:312: const PrefStore*
GetPrefStore(PrefStoreType type) const;
inline?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:318: // since the notifier is owned by
the corresponding PrefService.
Add initialization completed comment.

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi#newco...
chrome/chrome_common.gypi:289: 'common/pref_store.cc',
Is this still used?

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi#newco...
chrome/chrome_common.gypi:290: 'common/pref_store.h',
pref_store_interface? Oh, the humanity.

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

http://codereview.chromium.org/5441002/diff/1/chrome/common/pref_store.h#newc...
chrome/common/pref_store.h:69: virtual void AddObserver(ObserverInterface*
observer) {};
= 0? If not, please check multiple inheritance guidelines.

[battre (please use the other), 2010-12-02 10:41:19]

first set of comments

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_store.h:33: public NotificationObserver
{
ExtensionPrefStore will be replaced by something completely new with
http://codereview.chromium.org/5213002/

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:329:
prefs_->pref_notifier()->OnPreferenceChanged(kExtensionsPref);
OnPreferenceChanged has the connotation that the preference value has actually
changed. Is that always the case here?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:2330: {
add "// Scope for update notification"?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.cc:51: // class.
This comment became obsolete.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.h:48: // For the given pref_name, fire any
observer of the pref. Virtual so it can
|pref_name| vs. |path| below

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:130: void PutUserPrefValue(const char*
name, Value* in_value);
How about SetUserPrefValueSilently? Put and Set sound very much alike to me.

[battre (please use the other), 2010-12-02 11:51:34]

Some additional comments

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:117: bool ReadOnly();
Can we make this const?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:154: bool PrefValueUserModifiable(const
char* name) const;
Can we make this const?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:159: bool
HasPolicyConflictingUserProxySettings();
Can we make this const?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:231: // Returns true if the actual value
is a valid type for the expected type when
actual value? This description is not quite clear to me.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:248: PrefStoreType last_checked_store);
can we make this const?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:266: void NotifyPrefChanged(const char*
path, PrefStoreType new_store);
can we make this const?

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:317: // Used for generation PREF_CHANGED
notifications. This is a weak reference,
nit: "for generating" or "for generation of"

[Mattias Nissler (ping if slow), 2010-12-02 16:38:23]

Comments address for you reviewing pleasure. Please take another look.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_pref_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_pref_store.h:33: public NotificationObserver
{
On 2010/12/02 10:41:19, battre wrote:
> ExtensionPrefStore will be replaced by something completely new with
> http://codereview.chromium.org/5213002/

I treat this as an FYI even though I already knew :)

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_prefs.cc:329:
prefs_->pref_notifier()->OnPreferenceChanged(kExtensionsPref);
On 2010/12/02 10:41:19, battre wrote:
> OnPreferenceChanged has the connotation that the preference value has actually
> changed. Is that always the case here?

You are right, that is not the case. However, the real problem is that we're
calling into pref_notifier() here, which we shouldn't. The correct way would be
to use something like ScopedPrefUpdate, but there are plans to upgrade this as
well. I put a TODO.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extensions_service_unittest.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extensions_service_unittest.cc:2330: {
On 2010/12/02 10:41:19, battre wrote:
> add "// Scope for update notification"?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.cc:51: // class.
On 2010/12/02 10:41:19, battre wrote:
> This comment became obsolete.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.h:22: class PrefNotifier : public
NonThreadSafe {
On 2010/12/02 10:31:52, danno wrote:
> Make complete delegate interface that the PrefService implements as discussed.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier.h:48: // For the given pref_name, fire any
observer of the pref. Virtual so it can
On 2010/12/02 10:41:19, battre wrote:
> |pref_name| vs. |path| below

not applicable any longer.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:556:
pref_value_store_->PutUserPrefValue(path, dict);
On 2010/12/02 10:31:52, danno wrote:
> Call this SetUserPrefValueWithoutNotification/Silent or something.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.h:122: Profile* profile);
On 2010/12/02 10:31:52, danno wrote:
> How about making this protected and the testingprefservice can only calls
> this/makes it public?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service_unittest.cc:39: MATCHER_P3(PrefValueMatches,
prefs, pref_name, value, "") {
On 2010/12/02 10:31:52, danno wrote:
> Add comment please.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:186: NotifyPrefChanged(name,
USER_STORE);
On 2010/12/02 10:31:52, danno wrote:
> The user preference store should send this. Add TODO.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:201: NotifyPrefChanged(name,
USER_STORE);
On 2010/12/02 10:31:52, danno wrote:
> The user preference store should send this. Add TODO.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.cc:512: void
PrefValueStore::CheckInitializationCompleted() {
On 2010/12/02 10:31:52, danno wrote:
> Code is missing? This needs to be called by the Keeper. Add unit test.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
File chrome/browser/prefs/pref_value_store.h (right):

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:117: bool ReadOnly();
On 2010/12/02 11:51:34, battre wrote:
> Can we make this const?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:130: void PutUserPrefValue(const char*
name, Value* in_value);
On 2010/12/02 10:41:19, battre wrote:
> How about SetUserPrefValueSilently? Put and Set sound very much alike to me.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:154: bool PrefValueUserModifiable(const
char* name) const;
On 2010/12/02 11:51:34, battre wrote:
> Can we make this const?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:159: bool
HasPolicyConflictingUserProxySettings();
On 2010/12/02 11:51:34, battre wrote:
> Can we make this const?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:162: // PrefStores must be listed here
in order from highest to lowest priority.
On 2010/12/02 10:31:52, danno wrote:
> Can these be private?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:231: // Returns true if the actual value
is a valid type for the expected type when
On 2010/12/02 11:51:34, battre wrote:
> actual value? This description is not quite clear to me.

Changed to "actual type".

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:248: PrefStoreType last_checked_store);
On 2010/12/02 11:51:34, battre wrote:
> can we make this const?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:266: void NotifyPrefChanged(const char*
path, PrefStoreType new_store);
On 2010/12/02 11:51:34, battre wrote:
> can we make this const?

Sending a notification has side effects. So making this const would be lying.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:266: void NotifyPrefChanged(const char*
path, PrefStoreType new_store);
On 2010/12/02 10:31:52, danno wrote:
> OnPrefValueChanged?

As discussed, we want to keep this since it hosts the
check-if-user-visible-value-changed logic.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:296: void PrefValueChangedInStore(const
std::string& key, PrefStoreType type);
On 2010/12/02 10:31:52, danno wrote:
> Is this not the same as NotifyPrefChanged?

No, it's the handler for OnPrefValueChanged notifications received from the
PrefStores.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:299: void
InitializationCompleteForStore(PrefStoreType type);
On 2010/12/02 10:31:52, danno wrote:
> OnInitializedCompleted?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:311: PrefStore*
GetPrefStore(PrefStoreType type);
On 2010/12/02 10:31:52, danno wrote:
> inline?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:312: const PrefStore*
GetPrefStore(PrefStoreType type) const;
On 2010/12/02 10:31:52, danno wrote:
> inline?

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:317: // Used for generation PREF_CHANGED
notifications. This is a weak reference,
On 2010/12/02 11:51:34, battre wrote:
> nit: "for generating" or "for generation of"

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/browser/prefs/pref_value...
chrome/browser/prefs/pref_value_store.h:318: // since the notifier is owned by
the corresponding PrefService.
On 2010/12/02 10:31:52, danno wrote:
> Add initialization completed comment.

Done.

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi#newco...
chrome/chrome_common.gypi:289: 'common/pref_store.cc',
On 2010/12/02 10:31:52, danno wrote:
> Is this still used?

Yes.

http://codereview.chromium.org/5441002/diff/1/chrome/chrome_common.gypi#newco...
chrome/chrome_common.gypi:290: 'common/pref_store.h',
On 2010/12/02 10:31:52, danno wrote:
> pref_store_interface? Oh, the humanity.

No, since it has implementation.

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

http://codereview.chromium.org/5441002/diff/1/chrome/common/pref_store.h#newc...
chrome/common/pref_store.h:69: virtual void AddObserver(ObserverInterface*
observer) {};
On 2010/12/02 10:31:52, danno wrote:
> = 0? If not, please check multiple inheritance guidelines.

There are no cases of multiple inheritance concerning PrefStore.

[danno, 2010-12-06 09:20:13]

here you go...

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier.h:11: // This is the delegate interface used
by PrefValueStore to notify its owner
bit: how about just // Delegate (i.e. s/This is the d/D/)

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier.h:14: // cleaned up all public use of this
interface.
s/use/uses

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:37: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
const

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:62: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
const

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:91: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
const

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:37: // order they are added. These
should only be accessed externally for unit
nit: consistency with number of spaces after ".", here it's one, last sentence
two.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:41: PrefObserverMap;
nit: space between the typedefs and the method below.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:42: const PrefObserverMap*
pref_observers() { return &pref_observers_; }
method const too?

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

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:68: StringValue
expected_value(new_pref_value);
can you make this const?

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:80: StringValue
expected_default_value("");
this too?

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:8: #include
"chrome/browser/prefs/pref_notifier.h"
alphabetize

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/testin...
File chrome/browser/prefs/testing_pref_store.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/testin...
chrome/browser/prefs/testing_pref_store.h:40: void
NotifyInitializationCompleted();
Doesn't the compiler complain about name hiding here? In any case, is this
preferable to keeping them protected and making them accessible from the test
base?

[Mattias Nissler (ping if slow), 2010-12-06 10:58:12]

Feedback addressed, new version is up.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier.h:11: // This is the delegate interface used
by PrefValueStore to notify its owner
On 2010/12/06 09:20:14, danno wrote:
> bit: how about just // Delegate (i.e. s/This is the d/D/)

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier.h:14: // cleaned up all public use of this
interface.
On 2010/12/06 09:20:14, danno wrote:
> s/use/uses

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.cc (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:37: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
On 2010/12/06 09:20:14, danno wrote:
> const

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:62: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
On 2010/12/06 09:20:14, danno wrote:
> const

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.cc:91: PrefObserverMap::iterator
observer_iterator = pref_observers_.find(path);
On 2010/12/06 09:20:14, danno wrote:
> const

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
File chrome/browser/prefs/pref_notifier_impl.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:37: // order they are added. These
should only be accessed externally for unit
On 2010/12/06 09:20:14, danno wrote:
> nit: consistency with number of spaces after ".", here it's one, last sentence
> two.

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:41: PrefObserverMap;
On 2010/12/06 09:20:14, danno wrote:
> nit: space between the typedefs and the method below.

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_n...
chrome/browser/prefs/pref_notifier_impl.h:42: const PrefObserverMap*
pref_observers() { return &pref_observers_; }
On 2010/12/06 09:20:14, danno wrote:
> method const too?

Done.

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

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:68: StringValue
expected_value(new_pref_value);
On 2010/12/06 09:20:14, danno wrote:
> can you make this const?

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:80: StringValue
expected_default_value("");
On 2010/12/06 09:20:14, danno wrote:
> this too?

Done. And a few others as well.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_v...
File chrome/browser/prefs/pref_value_store.cc (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/pref_v...
chrome/browser/prefs/pref_value_store.cc:8: #include
"chrome/browser/prefs/pref_notifier.h"
On 2010/12/06 09:20:14, danno wrote:
> alphabetize

Done.

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/testin...
File chrome/browser/prefs/testing_pref_store.h (right):

http://codereview.chromium.org/5441002/diff/20001/chrome/browser/prefs/testin...
chrome/browser/prefs/testing_pref_store.h:40: void
NotifyInitializationCompleted();
On 2010/12/06 09:20:14, danno wrote:
> Doesn't the compiler complain about name hiding here? In any case, is this
> preferable to keeping them protected and making them accessible from the test
> base?

No name hiding problems, these are just regular overrides that happen to be
public. Added virtual modifiers to make this more obvious.

I don't like the friend declaration approach here. This code is meant to be
accessible to all tests that need it and since this class is exclusively here
for testing, I think we should design it so it's most useful for tests, i.e.
making these public.

[danno, 2010-12-06 14:17:53]

LGTM if you address the comment.

http://codereview.chromium.org/5441002/diff/29001/chrome/common/pref_store_ba...
File chrome/common/pref_store_base.h (right):

http://codereview.chromium.org/5441002/diff/29001/chrome/common/pref_store_ba...
chrome/common/pref_store_base.h:24: void NotifyPrefValueChanged(const
std::string& key);
These should be virtual too.

[Mattias Nissler (ping if slow), 2010-12-07 08:43:18]

Rebased after Dominics ExtensionPref stuff went in, please take another look.

[danno, 2010-12-07 15:54:56]

LGTM.