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

chrome/browser/extensions/extension_prefs.cc

Index: chrome/browser/extensions/extension_prefs.cc
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index 3c789b094fa88fcce2fcb8526a45694e5e428fb3..b4315b5a3898030c559e0a94828bcdf82067b97b 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -8,6 +8,7 @@
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/in_memory_pref_store.h"
 #include "chrome/common/pref_names.h"
 
 using base::Time;
@@ -78,6 +79,14 @@ const char kPrefAppLaunchIndex[] = "app_launcher_index";
 // "A preference for storing extra data sent in update checks for an extension.
 const char kUpdateUrlData[] = "update_url_data";
 
+// A preference that indicates when an extension was installed.
+const char kPrefInstallTime[] = "install_time";
+
+// A preference that indicates the last effective preference values of an
+// extension. The value is a dictionary mapping (non-expanded) preference keys
+// to the values configured by the extension.
+const char kPrefPreferences[] = "preferences";
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -119,6 +128,8 @@ ExtensionPrefs::ExtensionPrefs(PrefService* prefs, const FilePath& root_dir)
   CleanupBadExtensionKeys(prefs);
 
   MakePathsRelative();
+
+  InstallPersistedExtensionControlledPrefs();
 }
 
 ExtensionPrefs::~ExtensionPrefs() {}
@@ -524,12 +535,18 @@ void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension, Extension::State initial_state,
     bool initial_incognito_enabled) {
   const std::string& id = extension->id();
+  const base::Time installTime = GetCurrentTime();
   UpdateExtensionPref(id, kPrefState,
                       Value::CreateIntegerValue(initial_state));
   UpdateExtensionPref(id, kPrefIncognitoEnabled,
                       Value::CreateBooleanValue(initial_incognito_enabled));
   UpdateExtensionPref(id, kPrefLocation,
                       Value::CreateIntegerValue(extension->location()));
+  UpdateExtensionPref(id, kPrefInstallTime,
+                      Value::CreateStringValue(
+                          base::Int64ToString(installTime.ToInternalValue())));
+  UpdateExtensionPref(id, kPrefPreferences, new DictionaryValue());
+
   FilePath::StringType path = MakePathRelative(install_directory_,
       extension->path(), NULL);
   UpdateExtensionPref(id, kPrefPath, Value::CreateStringValue(path));
@@ -551,6 +568,10 @@ void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
   // and install the extension anymore (except when |external_uninstall| is
   // true, which signifies that the registry key was deleted or the pref file
   // no longer lists the extension).
+

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Remove blank line and move the two code lines before the comment.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+  std::vector<std::string> prefKeys;
+  GetExtensionControlledPrefKeys(extension_id, &prefKeys);
+
   if (!external_uninstall && Extension::IsExternalLocation(location)) {
     UpdateExtensionPref(extension_id, kPrefState,
                         Value::CreateIntegerValue(Extension::KILLBIT));
@@ -558,10 +579,15 @@ void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
   } else {
     DeleteExtensionPrefs(extension_id);
   }
+
+  for (std::vector<std::string>::iterator i = prefKeys.begin();
+       i != prefKeys.end(); ++i) {
+    UpdateWinningPref(*i);
+  }
 }
 
 Extension::State ExtensionPrefs::GetExtensionState(
-    const std::string& extension_id) {
+    const std::string& extension_id) const {
   DictionaryValue* extension = GetExtensionPref(extension_id);
 
   // If the extension doesn't have a pref, it's a --load-extension.
@@ -582,6 +608,14 @@ void ExtensionPrefs::SetExtensionState(const Extension* extension,
                                        Extension::State state) {
   UpdateExtensionPref(extension->id(), kPrefState,
                       Value::CreateIntegerValue(state));
+
+  std::vector<std::string> prefKeys;
+  GetExtensionControlledPrefKeys(extension->id(), &prefKeys);
+  for (std::vector<std::string>::iterator i = prefKeys.begin();

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

It might be useful to have a typedef std::vector<std::string> PrefKeySet in the
class declaration so you don't have to spell it out each time.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+       i != prefKeys.end(); ++i) {
+    UpdateWinningPref(*i);
+  }

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

You have this exact loop earlier in the code, maybe it's useful to factor it
into a UpdateExtensionControlledPrefsForExtension function?

[battre (please use the other), 2010/11/19 16:03:18]

It is not that simple: In OnExtensionUninstalled, I first retrieve the set of
extension keys, then remove an extension and finally do the update. I.e. there
is a step between GetExtensionControlledPrefKeys and the UpdateWinningPrefs.

+
   SavePrefsAndNotify();
 }
 
@@ -661,6 +695,16 @@ DictionaryValue* ExtensionPrefs::GetExtensionPref(
   return extension;
 }
 
+DictionaryValue* ExtensionPrefs::GetExtensionControlledPrefs(
+    const std::string& extension_id) const {
+  DictionaryValue* extension = GetExtensionPref(extension_id);
+  if (!extension)
+    return NULL;
+  DictionaryValue* preferences = NULL;
+  extension->GetDictionary(kPrefPreferences, &preferences);
+  return preferences;
+}
+
 // Helper function for GetInstalledExtensionsInfo.
 static ExtensionInfo* GetInstalledExtensionInfoImpl(
     DictionaryValue* extension_data,
@@ -920,6 +964,189 @@ std::string ExtensionPrefs::GetUpdateUrlData(const std::string& extension_id) {
   return data;
 }
 
+base::Time ExtensionPrefs::GetCurrentTime() const {
+  return base::Time::Now();

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

How does that implementation allow to inject a clock? The GetCurrentTime()
function is not virtual, so you cannot override.

[battre (please use the other), 2010/11/19 16:03:18]

that was a bug. Done.

+}
+
+base::Time ExtensionPrefs::GetInstallTime(const DictionaryValue* extension)
+    const {
+  DCHECK(extension);
+  std::string install_time_str("0");
+  extension->GetString(kPrefInstallTime, &install_time_str);
+  int64 install_time_i64 = 0;
+  base::StringToInt64(install_time_str, &install_time_i64);
+  LOG_IF(ERROR, install_time_i64 == 0)
+      << "Error parsing installation time of an extension";

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

period/exclamation mark?

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+  return base::Time::FromInternalValue(install_time_i64);
+}
+
+void ExtensionPrefs::InstallPersistedExtensionControlledPrefs() {
+  // When this is called, the PrefService is initialized and provides access
+  // to the user preferences stored in a JSON file. We take the persisted
+  // preferences of all extensions, calculate the effective preferences
+  // (considering that one extension overrides preferences of other extensions)
+  // and store the effective preferences in the PrefService.
+

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

No blank line. The rationale is that you're commenting on the code that follows,
so separating the two doesn't make much sense.

[battre (please use the other), 2010/11/19 16:03:18]

I considered this a comment about the entire function, not just the following
line. Therefore, I added a blank line. Changed it.

Done.

+  const DictionaryValue* extensions =
+      pref_service()->GetDictionary(kExtensionsPref);
+
+  // Collect extensions, sorted by time (latest installed appears last).
+  std::vector<DictionaryValue*> sorted_extensions;

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

How about just using a set with the right comparator?

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+  for (DictionaryValue::key_iterator ext_id = extensions->begin_keys();
+       ext_id != extensions->end_keys(); ++ext_id) {
+    if (GetExtensionState(*ext_id) != Extension::ENABLED)
+      continue;
+
+    DictionaryValue* extension = GetExtensionPref(*ext_id);
+    CHECK(extension != NULL);
+
+    if (GetInstallTime(extension) == base::Time::FromInternalValue(0)) {
+      // Fix old entry that did not get an installation time entry when
+      // it was installed.
+      const base::Time installTime = GetCurrentTime();
+      extension->Set(kPrefInstallTime,
+                     Value::CreateStringValue(
+                         base::Int64ToString(installTime.ToInternalValue())));
+      SavePrefsAndNotify();

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Can we call this after we're done, i.e. outside the loop?

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+    }
+    DictionaryValue* dummy_prefs;
+    if (!extension->GetDictionary(kPrefPreferences, &dummy_prefs)) {
+      extension->Set(kPrefPreferences, new DictionaryValue());
+    }

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Wouldn't it be more obvious to do the defaulting later? When reading the code
below, I'd think "Wait, what if the dictionary doesn't exist?". So I believe
putting the check there is more understandable.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+
+    // Currently we sort only by time, no tie-breaker.

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

Implement sorting inline? Please don't :) And please also add the tie breaker
(e.g. using the extension id).

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+    std::vector<DictionaryValue*>::iterator insert_pos =
+        sorted_extensions.begin();
+    while (insert_pos != sorted_extensions.end() &&
+           GetInstallTime(extension) > GetInstallTime(*insert_pos)) {
+      ++insert_pos;
+    }
+    sorted_extensions.insert(insert_pos, extension);
+  }
+
+  // Collect all effective preferences (later ones override newer ones).
+  scoped_ptr<DictionaryValue> merged_non_expanded(new DictionaryValue);

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

why is this a scoped_ptr instead of a plain DictionaryValue var?

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+  for (std::vector<DictionaryValue*>::iterator i = sorted_extensions.begin();
+       i != sorted_extensions.end(); ++i) {
+    DictionaryValue* preferences;
+    if ((*i)->GetDictionary(kPrefPreferences, &preferences))
+      merged_non_expanded->MergeDictionary(preferences);
+  }
+
+  // Expand all keys.
+  scoped_ptr<InMemoryPrefStore> extension_prefs(new InMemoryPrefStore);
+  for (DictionaryValue::key_iterator prefkey =
+           merged_non_expanded->begin_keys();
+       prefkey != merged_non_expanded->end_keys();
+       ++prefkey) {
+    Value* value;
+    CHECK(merged_non_expanded->GetWithoutPathExpansion(*prefkey, &value));
+    extension_prefs->prefs()->Set(*prefkey, value->DeepCopy());
+  }
+
+  // Store result in pref service.
+  pref_service()->pref_value_store()->ReplaceExtensionPrefStore(
+      extension_prefs.release());
+}
+
+namespace subtle {

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

you can just use an anonymous namespace here, I that's more common under
chrome/. But then again, since that function has static linkage, you don't need
the namespace at all.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+static bool equalValues(const Value* a, const Value* b) {
+  if ((a == NULL) && (b == NULL)) return true;
+  if ((a == NULL) ^  (b == NULL)) return false;
+  return a->Equals(b);

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

this can be rewritten as a == NULL ? b == NULL : a->Equals(b). YMMV though :) I
believe it should really be a static member in Value.

[battre (please use the other), 2010/11/19 16:03:18]

nice. :-)
Done.

+}
+}
+
+const Value* ExtensionPrefs::WinningExtensionControlledPrefValue(

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

It bothers me a bit that you have the logic that merges down the value in two
different places, i.e. InstallExtensionControlledPreferences and also here. I
don't have good suggestion how to share code other than keeping the dictionaries
in the correct order in memory and merging every time. So I guess I'm OK with
your approach.

[battre (please use the other), 2010/11/19 16:03:18]

I have simplified it a bit at least.

+    const std::string& key) const {
+  Value *winner = NULL;
+  int64 winners_install_time = 0;
+
+  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
+  for (DictionaryValue::key_iterator ext_iter = extensions->begin_keys();
+       ext_iter != extensions->end_keys(); ++ext_iter) {
+    if (GetExtensionState(*ext_iter) != Extension::ENABLED)
+      continue;
+
+    DictionaryValue* extension = GetExtensionPref(*ext_iter);
+    CHECK(extension);
+
+    std::string extension_install_time_s = "0";
+    extension->GetString(kPrefInstallTime, &extension_install_time_s);
+    int64 extension_install_time = 0;
+    base::StringToInt64(extension_install_time_s, &extension_install_time);
+
+    // We do not need to consider extensions that were installed before the
+    // most recent extension found that provides the requested preference.
+    if (extension_install_time < winners_install_time) {
+      continue;
+    }

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

No need for curlies.

[battre (please use the other), 2010/11/19 16:03:18]

Done.

+
+    DictionaryValue* preferences = GetExtensionControlledPrefs(*ext_iter);
+    CHECK(preferences);
+
+    Value *value = NULL;
+    if (preferences->GetWithoutPathExpansion(key, &value)) {
+      // This extension is more recent than the last one providing this pref.
+      winner = value;
+      winners_install_time = extension_install_time;
+    }
+  }
+  return winner;
+}
+
+void ExtensionPrefs::UpdateWinningPref(const std::string& pref_key) {
+  PrefStore* extensionPrefStore =
+      pref_service()->pref_value_store()->GetExtensionPrefStore();
+  const Value* winningPrefValue = WinningExtensionControlledPrefValue(pref_key);
+  Value* oldValue = NULL;
+  extensionPrefStore->prefs()->Get(pref_key, &oldValue);
+  bool changed = !subtle::equalValues(winningPrefValue, oldValue);
+
+  if (winningPrefValue) {
+    extensionPrefStore->prefs()->Set(pref_key, winningPrefValue->DeepCopy());
+  } else {
+    extensionPrefStore->prefs()->Remove(pref_key, NULL);
+  }
+
+  if (changed)
+    pref_service()->pref_notifier()->OnPreferenceSet(
+        pref_key.c_str(), PrefNotifier::EXTENSION_STORE);

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

hm, 1103 to here really looks like it should be a method on the pref store.
Problem with that is the store doesn't have access to the pref_notifier. Maybe
we should really provide the stores with the pref_notifier on construction.
WDYT? Put a TODO for now and fix it in a separate patch?

[battre (please use the other), 2010/11/19 16:03:18]

I agree. Added a TODO.

+}
+
+void ExtensionPrefs::SetExtensionControlledPref(const std::string& extension_id,
+                                                const std::string& pref_key,
+                                                Value* value) {
+  DictionaryValue* extensionPreferences =
+      GetExtensionControlledPrefs(extension_id);
+
+  Value* oldValue = NULL;
+  extensionPreferences->GetWithoutPathExpansion(pref_key, &oldValue);
+  bool modified = subtle::equalValues(oldValue, value);

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

if (!modified)
  return;

[battre (please use the other), 2010/11/19 16:03:18]

Done. Also inserted the missing !.

+
+  if (value == NULL)
+    extensionPreferences->RemoveWithoutPathExpansion(pref_key, NULL);
+  else
+    extensionPreferences->SetWithoutPathExpansion(pref_key, value);
+
+  if (modified)
+    pref_service()->ScheduleSavePersistentPrefs();

[Mattias Nissler (ping if slow), 2010/11/19 10:36:12]

You don't need to do that explicitly, the browser will on shutdown.

[battre (please use the other), 2010/11/19 16:03:18]

I think is it better to be crash save. WDYT?

+
+  UpdateWinningPref(pref_key);
+}
+
+void ExtensionPrefs::GetExtensionControlledPrefKeys(
+    const std::string& extension_id, std::vector<std::string> *out) const {
+  DCHECK(out != NULL);
+  DictionaryValue* extPrefs = GetExtensionControlledPrefs(extension_id);
+  if (extPrefs) {
+    for (DictionaryValue::key_iterator i = extPrefs->begin_keys();
+         i != extPrefs->end_keys(); ++i) {
+      out->push_back(*i);
+    }
+  }
+}
+
 // static
 void ExtensionPrefs::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterDictionaryPref(kExtensionsPref);