Sanitize PrefStore interface.

chrome/browser/prefs/testing_pref_store.h

Index: chrome/browser/prefs/testing_pref_store.h
diff --git a/chrome/browser/prefs/testing_pref_store.h b/chrome/browser/prefs/testing_pref_store.h
index fe014f45730ccbb33c692ce4dc0ac09eef0dcff9..b8ebd405c219f279372e6c2afbbaa117cdaa64d8 100644
--- a/chrome/browser/prefs/testing_pref_store.h
+++ b/chrome/browser/prefs/testing_pref_store.h
@@ -7,46 +7,63 @@
 #pragma once
 
 #include "base/basictypes.h"
+#include "base/observer_list.h"
 #include "base/scoped_ptr.h"
-#include "chrome/common/pref_store_base.h"
+#include "chrome/common/persistent_pref_store.h"
+#include "chrome/browser/prefs/pref_value_map.h"

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

alphabetize

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

Done.

 
 class DictionaryValue;
 
 // |TestingPrefStore| is a stub implementation of the |PrefStore| interface.

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

|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. 

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

Done.

 // It allows to get and set the state of the |PrefStore| as well as triggering
 // notifications.
-class TestingPrefStore : public PrefStoreBase {
+class TestingPrefStore : public PersistentPrefStore {
  public:
   TestingPrefStore();
   virtual ~TestingPrefStore() {}
 
-  virtual DictionaryValue* prefs() const { return prefs_.get(); }
-
-  virtual PrefStore::PrefReadError ReadPrefs();
-
+  // PersistentPrefStore overrides:
+  virtual ReadResult GetValue(const std::string& key, Value** result) const;

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

Since GetValue is inherited from PersistentPrefStore's parent, is it misleading
to list it under "PrefStore overrides"? Especially since you treat
AddObserver/RemoveObserver differently.

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

Done.

+  virtual void SetValue(const std::string& key, Value* value);
+  virtual void SetValueSilently(const std::string& key, Value* value);
+  virtual void RemoveValue(const std::string& key);
   virtual bool ReadOnly() const { return read_only_; }
-
+  virtual PersistentPrefStore::PrefReadError ReadPrefs();
   virtual bool WritePrefs();
+  virtual void ScheduleWritePrefs() {}
 
   // Getter and Setter methods for setting and getting the state of the
-  // |DummyPrefStore|.
+  // |TestingPrefStore|.
   virtual void set_read_only(bool read_only) { read_only_ = read_only; }
-  virtual void set_prefs(DictionaryValue* prefs) { prefs_.reset(prefs); }
   virtual void set_prefs_written(bool status) { prefs_written_ = status; }
   virtual bool get_prefs_written() { return prefs_written_; }
 
-  // Publish these functions so testing code can call them.
-  virtual void NotifyPrefValueChanged(const std::string& key);
-  virtual void NotifyInitializationCompleted();
-
   // Whether the store has completed all asynchronous initialization.
   virtual bool IsInitializationComplete() { return init_complete_; }
 
-  // Mark the store as having completed initialization.
+  // Marks the store as having completed initialization.
   void SetInitializationCompleted();
 
+  // Used for tests to trigger notifications explicitly.
+  void NotifyPrefValueChanged(const std::string& key);
+  void NotifyInitializationCompleted();
+
+  // Some convenience getters/setters.
+  void SetString(const std::string& key, const std::string& value);
+  void SetInteger(const std::string& key, int value);
+  void SetBoolean(const std::string& key, bool value);
+
+  bool GetString(const std::string& key, std::string* value);

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

const? maybe not because it is a testing class? - same below

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

Done.

+  bool GetInteger(const std::string& key, int* value);
+  bool GetBoolean(const std::string& key, bool* value);
+
  private:
-  scoped_ptr<DictionaryValue> prefs_;
+  // Overriden from PrefStore.
+  virtual void AddObserver(PrefStore::ObserverInterface* observer);
+  virtual void RemoveObserver(PrefStore::ObserverInterface* observer);

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

Why are these made private? Seems like for testing you would like to keep them
public.

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

Done.

+
+  // Stores the preference values.
+  PrefValueMap prefs_;
 
   // Flag that indicates if the PrefStore is read-only
   bool read_only_;
@@ -57,6 +74,8 @@ class TestingPrefStore : public PrefStoreBase {
   // Whether initialization has been completed.
   bool init_complete_;
 
+  ObserverList<PrefStore::ObserverInterface, true> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(TestingPrefStore);
 };