Fix for Bug 50726 "Save extension list and "winning" prefs from extensions"

chrome/browser/extensions/extension_pref_store.h

Index: chrome/browser/extensions/extension_pref_store.h
diff --git a/chrome/browser/extensions/extension_pref_store.h b/chrome/browser/extensions/extension_pref_store.h
index f1767589a9aee4a67b2dec4814c4e19716e3803f..1f718f21807fb569d1f24504b6f0e29ffaf51440 100644
--- a/chrome/browser/extensions/extension_pref_store.h
+++ b/chrome/browser/extensions/extension_pref_store.h
@@ -6,15 +6,13 @@
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_PREF_STORE_H_
 #pragma once
 
-#include <list>
-#include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/scoped_ptr.h"
-#include "base/stl_util-inl.h"
 #include "chrome/browser/prefs/pref_notifier.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
@@ -22,6 +20,7 @@
 
 class DictionaryValue;
 class Extension;
+class ListValue;
 class PrefService;
 class Profile;
 class Value;
@@ -30,12 +29,22 @@ class Value;
 // proxy settings. A stack of relevant extensions is stored in order of
 // their addition to this PrefStore. For each preference, the last-added
 // enabled extension that tries to set it overrules any others.
+//
+// The preferences of extensions are embedded into the user's persistent pref
+// store such that the precendence of extensions is persisted and such that
+// preferences are available immediately after the browser is restarted (i.e.
+// even before extensions have been started).
+// The ExtensionPrefStore hooks into the "extension_preferences" branch of the
+// persistent user prefs. The structure is as follows:
+// extension_preferences: (
+//   { id: "extension_id_1", preferences: { ...preferences of ext. 1... } },
+//   { id: "extension_id_2", preferences: { ...preferences of ext. 2... } },
+//   ... )
+// In this example, extension 1 has higher precendence than extension 2.

[Bernhard Bauer, 2010/11/12 18:16:57]

Nit: "precedence"

[battre (please use the other), 2010/11/12 19:07:09]

Done.

+// Keys in extension_preferences/preferences are stored without path expansion.
 class ExtensionPrefStore : public PrefStore,
                            public NotificationObserver {
  public:
-  // Maps preference paths to their values.
-  typedef std::map<const char*, Value*> PrefValueMap;
-
   // The type passed as Details for an EXTENSION_PREF_CHANGED notification.
   // The nested pairs are <extension, <pref_path, pref_value> >. This is here,
   // rather than in (say) notification_type.h, to keep the dependency on
@@ -43,6 +52,11 @@ class ExtensionPrefStore : public PrefStore,
   typedef std::pair<const Extension*, std::pair<const char*, Value*> >
       ExtensionPrefDetails;
 
+  // The local-state ExtensionPrefStore (shared among all profiles on a
+  // machine), indicated by a NULL |profile|, is prohibited from storing
+  // preferences. Since extensions are installed per profile, allowing them to
+  // control machine-wide settings could lead to unsolvable conflicts, and
+  // should not be necessary anyway.
   ExtensionPrefStore(Profile* profile, PrefNotifier::PrefStoreType type);
   virtual ~ExtensionPrefStore();
 
@@ -58,33 +72,94 @@ class ExtensionPrefStore : public PrefStore,
   virtual void UninstallExtension(const Extension* extension);
 
   // PrefStore methods:
-  virtual DictionaryValue* prefs() const { return prefs_.get(); }
-
+  virtual DictionaryValue* prefs() const;
   virtual PrefReadError ReadPrefs() { return PREF_READ_ERROR_NONE; }
 
+  static const char kExtensionPreferencesKey[];
  protected:
   // Returns a vector of the extension IDs in the extension_stack_.
   // This should only be accessed by subclasses for unit-testing.
   void GetExtensionIDs(std::vector<std::string>* result);
 
-  // Returns the applicable pref service from the profile (if we have one) or
-  // the browser's local state. This should only be accessed or overridden by
-  // subclasses for unit-testing.
-  virtual PrefService* GetPrefService();
+  // For PrefService injection for unit-testing
+  virtual PrefService* GetPrefService() const;
+
+  // Schedules a write of the backing user-pref store.
+  virtual void SchedulePersist();
 
  private:
-  // Associates an extension with the prefs it sets. Owns the pref values.
-  struct ExtensionPrefs {
-    ExtensionPrefs(const Extension* extension, PrefValueMap* values);
+  // For visibility of helper classes:
+  FRIEND_TEST_ALL_PREFIXES(ExtensionPrefStoreTest, ExtensionPrefs);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionPrefStoreTest, ExtensionStack);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionPrefStoreTest, InstallRegisteredExtensions);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionPrefStoreTest,
+                           UninstallRegisteredExtensions);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionPrefStoreTest,
+                           ReadFromRegisteredExtensions);
+
+  // Wrapper around one Dictionary embedded in the "extension_preferences"
+  // list. Contains an "id"->String and "preferences"->Dictionary entry.
+  class ExtensionPrefs {
+   public:
+    // Wraps itself around the DictionaryValue that is supposed to follow
+    // the predefined structure (see ExtensionPrefStore). The ownership
+    // of dict remains at the caller.
+    explicit ExtensionPrefs(DictionaryValue* dict);
     ~ExtensionPrefs();
 
-    const Extension* extension;
-    PrefValueMap* pref_values;
+    // Creates a DictionaryValue { id: |extension_id|, preferences: {} }
+    // and passes ownership to the caller.
+    static DictionaryValue* Create(const std::string& extension_id);
+
+    // Returns the id of the extension managed by this ExtensionPrefs
+    std::string extension_id() const;
+
+    // Returns a pointer to the preference values, the ownership remains
+    // at |dict| passed to the constructor.
+    DictionaryValue* pref_values();
+
+    // Sets a |value| and passes ownership to |dict|. Deletes the value, if
+    // |value| is NULL.
+    void set(const std::string& key, Value* value);

[Bernhard Bauer, 2010/11/12 18:16:57]

Nit-ish: Use |Set| and |Get|. Normally we only use hacker_style method names for
accessors (so they're consistent with the field they're accessing). Same for
ExtensionStack.

[battre (please use the other), 2010/11/12 19:07:09]

Done.

+
+    // Retrieves a value but retains ownership at |dict|. Returns NULL if the
+    // key is not contained in the dictionary.
+    Value* get(const std::string& key);
+
+    // Keys in |dict_|:
+    static const char kIdKey[];
+    static const char kPreferencesKey[];
+   private:
+    // Weak reference
+    DictionaryValue* dict_;
   };
 
-  // A pseudo-stack of extensions and their preferences. Extensions are always
-  // added to the head, but may be removed from the middle.
-  typedef std::list<ExtensionPrefs*> ExtensionStack;
+  // Wrapper around a list in a persistent PrefStore in order to manage and
+  // persist preferences of extensions. Each individual extension's preferences
+  // are managed by ExtensionPrefs.
+  class ExtensionStack {
+   public:
+    // Preliminary construction, still requires init-call.
+    ExtensionStack();
+    ~ExtensionStack();
+
+    // Attaches the |list| to this object without taking ownership.
+    void init(ListValue* list);
+    bool is_initialized() const;
+
+    // returns number of registered extensions
+    size_t size() const;
+    ExtensionPrefs get(int index);
+    void remove(int index);
+
+    // Creates and registeres a new entry for the extension with |extension_id|.
+    // This extension gains highest precedence and can override other
+    // extensions' preference values.
+    ExtensionPrefs CreateEntry(const std::string& extension_id);
+   private:
+    // Weak reference
+    ListValue* list_;
+  };
 
   // Applies the highest-priority extension's setting for the given preference
   // path to the |prefs_| store, or clears the setting there if no extensions
@@ -92,7 +167,7 @@ class ExtensionPrefStore : public PrefStore,
   void UpdateOnePref(const char* path);
 
   // Updates each preference in the key set of the |pref_values| map.
-  void UpdatePrefs(const PrefValueMap* pref_values);
+  void UpdatePrefs(const DictionaryValue* pref_values);
 
   // Registers this as an observer for relevant notifications.
   void RegisterObservers();
@@ -102,11 +177,17 @@ class ExtensionPrefStore : public PrefStore,
                const NotificationSource& source,
                const NotificationDetails& details);
 
+  // Initializes the extension_stack_ with empty dictionaries for all
+  // extensions registered in the precedence list.
+  // Declared const for "DictionaryValue* prefs() const"
+  void LazyInit() const;

[Bernhard Bauer, 2010/11/12 18:16:57]

Are you sure that declaring this const is the way to go? I wouldn't have a big
problem with making PrefStore::prefs non-const.

[battre (please use the other), 2010/11/12 19:07:09]

I leave that up to discussion. I have no strong preference. As prefs() returns a
non-const pointer, it is questionable anyway whether it should be const.

+
   // A cache of the highest-priority values for each preference that any
   // extension is controlling, for quick read access. Owns the stored values.
   scoped_ptr<DictionaryValue> prefs_;
 
-  ExtensionStack extension_stack_;
+  // Accessor to preference values stored in user's pref store.
+  mutable ExtensionStack extension_stack_;
 
   NotificationRegistrar notification_registrar_;
 
@@ -115,6 +196,10 @@ class ExtensionPrefStore : public PrefStore,
   // extensions.
   Profile* profile_;
 
+  // Empty list that is only used for "local state" (when there is no user pref
+  // store).
+  scoped_ptr<ListValue> dummy_prefs;
+
   // My PrefStore type, assigned by the PrefValueStore.
   PrefNotifier::PrefStoreType type_;