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

chrome/browser/extensions/extension_pref_store.cc

Index: chrome/browser/extensions/extension_pref_store.cc
diff --git a/chrome/browser/extensions/extension_pref_store.cc b/chrome/browser/extensions/extension_pref_store.cc
index 27719ed3a69345009a2b73be9c7c7156c619e99e..ac29280f0a301a76af48a87fff48e5854689ca51 100644
--- a/chrome/browser/extensions/extension_pref_store.cc
+++ b/chrome/browser/extensions/extension_pref_store.cc
@@ -7,79 +7,185 @@
 #include "base/logging.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
 #include "chrome/common/extensions/extension.h"
-#include "chrome/common/notification_service.h"
+#include "chrome/common/notification_details.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
+
+const char ExtensionPrefStore::kExtensionPreferencesKey[] =
+    "extension_preferences";
+
+// ExtensionPrefs implementation:
+
+const char ExtensionPrefStore::ExtensionPrefs::kIdKey[] = "id";
+const char ExtensionPrefStore::ExtensionPrefs::kPreferencesKey[] =
+    "preferences";
+
+ExtensionPrefStore::ExtensionPrefs::ExtensionPrefs(DictionaryValue* dict)
+    : dict_(dict) {
+  DCHECK(dict_->HasKey("id"));
+  DCHECK(dict_->HasKey("preferences"));
+  DCHECK(dict_->size()==2);

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

Use DCHECK_EQ(2, dict->size()) for better error messages.

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

Done.

+}
+
+ExtensionPrefStore::ExtensionPrefs::~ExtensionPrefs()
+  {}

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

Nit: Either both braces on the previous line, or the opening one on the previous
line, and the closing one here. Same for ExtensionStack below.

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

Done.

+
+DictionaryValue* ExtensionPrefStore::ExtensionPrefs::Create(
+    const std::string& extension_id) {
+  scoped_ptr<DictionaryValue> dict(new DictionaryValue());
+  dict->Set(kIdKey, Value::CreateStringValue(extension_id));
+  dict->Set(kPreferencesKey, new DictionaryValue());
+  return dict.release();
+}
+
+std::string ExtensionPrefStore::ExtensionPrefs::extension_id() const {
+  std::string string_value;
+  CHECK(dict_->GetString(kIdKey, &string_value));

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

I think in release builds we try to only crash to avoid something even worse,
like memory corruption. If the "id" key is missing, that points to preference
corruption (either caused by the user, or by us), and we should try to recover
from it.

TL;DR: My gut feeling says DCHECK here.

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

Done. If this fails, there won't be a crash but the issue that preference values
might never be purged even if all extensions are uninstalled. I have converted
this to DCHECK.

+  return string_value;
+}
+
+DictionaryValue* ExtensionPrefStore::ExtensionPrefs::pref_values() {
+  DictionaryValue* value = NULL;
+  CHECK(dict_->GetDictionary(kPreferencesKey, &value));
+  return value;
+}
+
+void ExtensionPrefStore::ExtensionPrefs::set(const std::string& key,
+                                             Value *value) {
+  if (value == NULL)
+    pref_values()->RemoveWithoutPathExpansion(key, NULL);
+  else
+    pref_values()->SetWithoutPathExpansion(key, value);
+}
+
+Value* ExtensionPrefStore::ExtensionPrefs::get(const std::string& key) {
+  Value* value = NULL;
+  pref_values()->GetWithoutPathExpansion(key, &value);
+  return value;  // NULL if Get failed.

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

Nit: That comment is unnecessary.

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

Done.

+}
+
+
+// ExtensionStack implementation:
+
+ExtensionPrefStore::ExtensionStack::ExtensionStack()
+: list_(NULL) {}
+
+ExtensionPrefStore::ExtensionStack::~ExtensionStack()
+{}
+
+void ExtensionPrefStore::ExtensionStack::init(ListValue* list) {
+  list_ = list;
+}
+
+bool ExtensionPrefStore::ExtensionStack::is_initialized() const {
+  return list_ != NULL;
+}
+
+size_t ExtensionPrefStore::ExtensionStack::size() const {
+  DCHECK(is_initialized());
+  return list_->GetSize();
+}
+
+ExtensionPrefStore::ExtensionPrefs
+    ExtensionPrefStore::ExtensionStack::get(int index) {
+  DCHECK(is_initialized());
+  DictionaryValue* dict;
+  CHECK(list_->GetDictionary(index, &dict));
+  return ExtensionPrefs(dict);
+}
+
+void ExtensionPrefStore::ExtensionStack::remove(int index) {
+  DCHECK(is_initialized());
+  CHECK(list_->Remove(index, NULL));
+}
+
+ExtensionPrefStore::ExtensionPrefs
+    ExtensionPrefStore::ExtensionStack::CreateEntry(
+        const std::string& extension_id) {
+  DCHECK(is_initialized());
+  list_->Insert(0, ExtensionPrefs::Create(extension_id));
+  return get(0);
+}
+
+
+// ExtensionPrefStore implementation:
 
 ExtensionPrefStore::ExtensionPrefStore(Profile* profile,
                                        PrefNotifier::PrefStoreType type)
     : prefs_(new DictionaryValue()),
       profile_(profile),
+      dummy_prefs(profile ? NULL : new ListValue),
       type_(type) {
   RegisterObservers();
 }
 
 ExtensionPrefStore::~ExtensionPrefStore() {
-  STLDeleteElements(&extension_stack_);
   notification_registrar_.RemoveAll();
 }
 
 void ExtensionPrefStore::InstallExtensionPref(const Extension* extension,
                                               const char* new_pref_path,
                                               Value* new_pref_value) {
-  ExtensionStack::iterator i;
-  for (i = extension_stack_.begin(); i != extension_stack_.end(); ++i) {
-    if ((*i)->extension == extension)
+  if (!extension_stack_.is_initialized())
+    LazyInit();
+
+  int nr_extensions = extension_stack_.size();
+  int i;
+  for (i = 0; i < nr_extensions; ++i) {
+    if (extension_stack_.get(i).extension_id() == extension->id())
       break;
   }
 
   // If this extension is not already in the stack, add it. Otherwise, update
   // or add the value of this preference, but don't change the extension's
-  // position in the stack. We store the extension even if this preference
-  // isn't registered with our PrefService, so that the ordering of extensions
-  // is consistent among all local-state and user ExtensionPrefStores.
-  PrefService* pref_service = GetPrefService();
-  // The pref_service may be NULL in unit testing.
-  bool is_registered_pref = (pref_service == NULL ||
-      pref_service->FindPreference(new_pref_path) != NULL);
-  PrefValueMap* pref_values;
-  if (i == extension_stack_.end()) {
-    pref_values = new PrefValueMap();
-    if (is_registered_pref)
-      (*pref_values)[new_pref_path] = new_pref_value;
-
-    ExtensionPrefs* extension_prefs = new ExtensionPrefs(extension,
-                                                         pref_values);
-    extension_stack_.push_front(extension_prefs);
-  } else if (is_registered_pref) {
-    pref_values = (*i)->pref_values;
-    delete (*pref_values)[new_pref_path];
-    (*pref_values)[new_pref_path] = new_pref_value;
+  // position in the stack.
+  if (i == nr_extensions) {
+    ExtensionPrefs extension_prefs =
+        extension_stack_.CreateEntry(extension->id());
+    extension_prefs.set(new_pref_path, new_pref_value);
+  } else {
+    ExtensionPrefs extension_prefs = extension_stack_.get(i);
+    extension_prefs.set(new_pref_path, new_pref_value);
   }
 
   // Apply the preference to our local |prefs_| store.
   UpdateOnePref(new_pref_path);
+  SchedulePersist();
 }
 
 void ExtensionPrefStore::UninstallExtension(const Extension* extension) {
+  if (!extension_stack_.is_initialized())
+    LazyInit();
+
   // Remove this extension from the stack.
-  for (ExtensionStack::iterator i = extension_stack_.begin();
-      i != extension_stack_.end(); ++i) {
-    if ((*i)->extension == extension) {
-      scoped_ptr<ExtensionPrefs> to_be_deleted(*i);
-      extension_stack_.erase(i);
-      UpdatePrefs(to_be_deleted->pref_values);
+  for (int i = 0, n = extension_stack_.size(); i < n; ++i) {
+    ExtensionPrefs extension_prefs = extension_stack_.get(i);
+    if (extension_prefs.extension_id() == extension->id()) {
+      scoped_ptr<DictionaryValue> dict_values(
+          extension_prefs.pref_values()->DeepCopyWithoutEmptyChildren());
+      extension_stack_.remove(i);
+      UpdatePrefs(dict_values.get());
+      SchedulePersist();
       return;
     }
   }
 }
 
+DictionaryValue* ExtensionPrefStore::prefs() const {
+  if (!extension_stack_.is_initialized())
+    LazyInit();
+  return prefs_.get();
+}
+
 void ExtensionPrefStore::GetExtensionIDs(std::vector<std::string>* result) {
-  for (ExtensionStack::iterator i = extension_stack_.begin();
-      i != extension_stack_.end(); ++i) {
-    (*result).push_back((*i)->extension->id());
-  }
+  if (!extension_stack_.is_initialized())
+    LazyInit();
+  for (int i = 0, n = extension_stack_.size(); i < n; ++i)
+    (*result).push_back(extension_stack_.get(i).extension_id());

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

Nit: result->push_back

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

Done.

 }
 
 // This could be sped up by keeping track of which extension currently controls
@@ -87,16 +193,6 @@ void ExtensionPrefStore::GetExtensionIDs(std::vector<std::string>* result) {
 // installed extensions will be trying to control any preferences, so stick
 // with this simpler algorithm until it causes a problem.
 void ExtensionPrefStore::UpdateOnePref(const char* path) {
-  PrefService* pref_service = GetPrefService();
-
-  // There are at least two PrefServices, one for local state and one for
-  // user prefs. (See browser_main.cc.) Different preferences are registered
-  // in each; if this one doesn't have the desired pref registered, we ignore
-  // it and let the other one handle it.
-  // The pref_service may be NULL in unit testing.
-  if (pref_service && !pref_service->FindPreference(path))
-      return;
-
   // Save the old value before removing it from the local cache.
   Value* my_old_value_ptr = NULL;
   prefs_->Get(path, &my_old_value_ptr);
@@ -110,16 +206,19 @@ void ExtensionPrefStore::UpdateOnePref(const char* path) {
 
   // Find the first extension that wants to set this pref and use its value.
   Value* my_new_value = NULL;
-  for (ExtensionStack::iterator ext_iter = extension_stack_.begin();
-        ext_iter != extension_stack_.end(); ++ext_iter) {
-    PrefValueMap::iterator value_iter = (*ext_iter)->pref_values->find(path);
-    if (value_iter != (*ext_iter)->pref_values->end()) {
-      prefs_->Set(path, (*value_iter).second->DeepCopy());
-      my_new_value = (*value_iter).second;
+  for (int i = 0, n = extension_stack_.size(); i < n; ++i) {
+    DictionaryValue* pref_values = extension_stack_.get(i).pref_values();
+    bool has_pref = pref_values->GetWithoutPathExpansion(path, &my_new_value);
+    if (has_pref) {

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

Nit: You could inline |has_pref|.

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

Had it inlined before and found this easier to read. Would keep it unless you
object.

+      prefs_->Set(path, my_new_value->DeepCopy());
       break;
     }
   }
+  // If any extension contained a value for |path| it is stored in my_new_value
+  // now.
 
+  // May be null in unit tests.
+  PrefService* pref_service = GetPrefService();
   if (pref_service) {
     bool value_changed = true;
     if (!my_old_value.get() && !my_new_value) {
@@ -135,30 +234,41 @@ void ExtensionPrefStore::UpdateOnePref(const char* path) {
   }
 }
 
-void ExtensionPrefStore::UpdatePrefs(const PrefValueMap* pref_values) {
+void ExtensionPrefStore::UpdatePrefs(const DictionaryValue* pref_values) {
   if (!pref_values)
     return;
 
-  for (PrefValueMap::const_iterator i = pref_values->begin();
-        i != pref_values->end(); ++i) {
-    UpdateOnePref(i->first);
+  for (DictionaryValue::key_iterator i = pref_values->begin_keys();
+       i != pref_values->end_keys(); ++i) {
+    UpdateOnePref((*i).c_str());
   }
 }
 
-PrefService* ExtensionPrefStore::GetPrefService() {
-  if (profile_)
-    return profile_->GetPrefs();
-  return g_browser_process->local_state();
+PrefService* ExtensionPrefStore::GetPrefService() const {
+  // In case of local state, there is no profile nor pref service.
+  if (!profile_)
+    return NULL;
+  return profile_->GetPrefs();
+}
+
+void ExtensionPrefStore::SchedulePersist() {
+  PrefService* prefService = GetPrefService();
+  if (prefService)
+    prefService->ScheduleSavePersistentPrefs();
 }
 
 void ExtensionPrefStore::RegisterObservers() {
+  // If profile_==NULL, this ExtensionPrefStore is for local-state.

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

Nit: "If |profile_| is NULL"? In whole sentences, I find prose easier to read
than code. If you want to keep the |==|, please add spaces around it (even code
in comments should keep to the style guide ;-)

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

Done.

+  if (!profile_)
+    return;
+
   notification_registrar_.Add(this,
                               NotificationType::EXTENSION_PREF_CHANGED,
-                              NotificationService::AllSources());
+                              Source<Profile>(profile_));
 
   notification_registrar_.Add(this,
                               NotificationType::EXTENSION_UNLOADED,
-                              NotificationService::AllSources());
+                              Source<Profile>(profile_));
 }
 
 void ExtensionPrefStore::Observe(NotificationType type,
@@ -166,22 +276,15 @@ void ExtensionPrefStore::Observe(NotificationType type,
                                  const NotificationDetails& details) {
   switch (type.value) {
     case NotificationType::EXTENSION_PREF_CHANGED: {
-      Profile* extension_profile = Source<Profile>(source).ptr();
-      // The ExtensionPrefStore for the local state watches all profiles.
-      if (!profile_ || profile_ == extension_profile) {
-        ExtensionPrefStore::ExtensionPrefDetails* data =
-            Details<ExtensionPrefStore::ExtensionPrefDetails>(details).ptr();
-        InstallExtensionPref(data->first, data->second.first,
-            data->second.second);
-      }
+      ExtensionPrefStore::ExtensionPrefDetails* data =
+          Details<ExtensionPrefStore::ExtensionPrefDetails>(details).ptr();
+      InstallExtensionPref(data->first, data->second.first,
+          data->second.second);
       break;
     }
     case NotificationType::EXTENSION_UNLOADED: {
-      Profile* extension_profile = Source<Profile>(source).ptr();
       const Extension* extension = Details<const Extension>(details).ptr();
-      // The ExtensionPrefStore for the local state watches all profiles.
-      if (profile_ == NULL || profile_ == extension_profile)
-        UninstallExtension(extension);
+      UninstallExtension(extension);
       break;
     }
     default: {
@@ -190,10 +293,33 @@ void ExtensionPrefStore::Observe(NotificationType type,
   }
 }
 
-ExtensionPrefStore::ExtensionPrefs::ExtensionPrefs(const Extension* extension,
-    PrefValueMap* values) : extension(extension), pref_values(values) {}
+void ExtensionPrefStore::LazyInit() const {
+  // If profile_==NULL, this ExtensionPrefStore is for local-state.
+  DCHECK(!extension_stack_.is_initialized())
+      << "LazyInit called even though extension_stack_ is already initialized";
+
+  PrefService* pref_service = GetPrefService();
+  if (!pref_service) {
+    extension_stack_.init(dummy_prefs.get());
+    return;
+  }
+  pref_service->RegisterListPref(kExtensionPreferencesKey);
+
+  scoped_ptr<ListValue> dict(
+      pref_service->GetMutableList(kExtensionPreferencesKey));
+
+  extension_stack_.init(dict.release());
 
-ExtensionPrefStore::ExtensionPrefs::~ExtensionPrefs() {
-  STLDeleteValues(pref_values);
-  delete pref_values;
+  for (int i = 0, n = extension_stack_.size(); i < n; ++i) {
+    DictionaryValue* pref_values = extension_stack_.get(i).pref_values();
+    for (DictionaryValue::key_iterator iter = pref_values->begin_keys();
+         iter != pref_values->end_keys(); ++iter) {
+      const std::string& key = *iter;
+      if (!prefs_->HasKey(key)) {
+        Value *value;
+        if (pref_values->GetWithoutPathExpansion(key, &value))
+          prefs_->Set(key, value->DeepCopy());
+      }
+    }
+  }
 }