Sanitize PrefStore interface.

chrome/common/pref_store.h

Index: chrome/common/pref_store.h
diff --git a/chrome/common/pref_store.h b/chrome/common/pref_store.h
index 808ffbd20d986e369b63148ee03c21bd1f5af304..4a4ccff8dcf1e4c74ccd8441994f3189ac15ebe9 100644
--- a/chrome/common/pref_store.h
+++ b/chrome/common/pref_store.h
@@ -10,7 +10,6 @@
 
 #include "base/basictypes.h"
 
-class DictionaryValue;
 class Value;
 
 // This is an abstract interface for reading and writing from/to a persistent
@@ -33,35 +32,16 @@ class PrefStore {
     virtual void OnInitializationCompleted() = 0;
   };
 
-  // Unique integer code for each type of error so we can report them
-  // distinctly in a histogram.
-  // NOTE: Don't change the order here as it will change the server's meaning
-  // of the histogram.
-  enum PrefReadError {
-    PREF_READ_ERROR_NONE = 0,
-    PREF_READ_ERROR_JSON_PARSE,
-    PREF_READ_ERROR_JSON_TYPE,
-    PREF_READ_ERROR_ACCESS_DENIED,
-    PREF_READ_ERROR_FILE_OTHER,
-    PREF_READ_ERROR_FILE_LOCKED,
-    PREF_READ_ERROR_NO_FILE,
-    PREF_READ_ERROR_JSON_REPEAT,
-    PREF_READ_ERROR_OTHER,
-    PREF_READ_ERROR_FILE_NOT_SPECIFIED
+  // Return values for GetValue().
+  enum ReadResult {
+    // Value found and returned.
+    READ_OK,
+    // No value present, but skip other pref stores and use default.

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

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"?

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

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.

+    READ_USE_DEFAULT,
+    // No value present.
+    READ_NO_VALUE,
   };
 
-  // To require that the default value be used for a preference, a
-  // PrefStore can set the value in its own prefs dictionary to the
-  // sentinel Value returned by this function.
-  // TODO(danno): Instead of having a sentinel value, pref stores
-  // should return a richer set of information from the property
-  // accessor methods to indicate that the default should be used.
-  static Value* CreateUseDefaultSentinelValue();
-
-  // Returns true if a value is the special sentinel value created by
-  // CreateUseDefaultSentinelValue.
-  static bool IsUseDefaultSentinelValue(Value* value);
-
   PrefStore() {}
   virtual ~PrefStore() {}
 
@@ -72,22 +52,9 @@ class PrefStore {
   // Whether the store has completed all asynchronous initialization.
   virtual bool IsInitializationComplete() { return true; }
 
-  // Whether the store is in a pseudo-read-only mode where changes are not
-  // actually persisted to disk.  This happens in some cases when there are
-  // read errors during startup.
-  virtual bool ReadOnly() const { return true; }
-
-  // TODO(danno): PrefValueStore shouldn't allow direct access to the
-  // DictionaryValue. Instead, it should have getters that return a
-  // richer set of information for a pref, including if the store
-  // wants to return the default value for a preference.
-  virtual DictionaryValue* prefs() const = 0;
-
-  virtual PrefReadError ReadPrefs() = 0;
-
-  virtual bool WritePrefs() { return true; }
-
-  virtual void ScheduleWritePrefs() { }
+  // Get the value for a given preference |key| and stores it in |result|.
+  // Ownership remains with the PrefStore.

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

Does not modify |result| if the key does not exist.

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

Done.

+  virtual ReadResult GetValue(const std::string& key, Value** result) const = 0;
 
   DISALLOW_COPY_AND_ASSIGN(PrefStore);
 };