Add onChanged events to the extension settings API, both from sync and between

chrome/browser/extensions/extension_settings_leveldb_storage.cc

Index: chrome/browser/extensions/extension_settings_leveldb_storage.cc
diff --git a/chrome/browser/extensions/extension_settings_leveldb_storage.cc b/chrome/browser/extensions/extension_settings_leveldb_storage.cc
index 1a717ae717af64731ea505291ce7acabd73e465f..6819453533bd795ee3ca2a15d83db1a0143c7b04 100644
--- a/chrome/browser/extensions/extension_settings_leveldb_storage.cc
+++ b/chrome/browser/extensions/extension_settings_leveldb_storage.cc
@@ -87,7 +87,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get(
   if (setting.get()) {
     settings->SetWithoutPathExpansion(key, setting.release());
   }
-  return Result(settings, NULL);
+  return Result(settings, NULL, NULL);
 }
 
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get(
@@ -111,7 +111,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get(
     }
   }
 
-  return Result(settings.release(), NULL);
+  return Result(settings.release(), NULL, NULL);
 }
 
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() {
@@ -141,7 +141,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() {
     return Result(kGenericOnFailureMessage);
   }
 
-  return Result(settings.release(), NULL);
+  return Result(settings.release(), NULL, NULL);
 }
 
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set(
@@ -155,24 +155,29 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set(
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set(
     const DictionaryValue& settings) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<DictionaryValue> old_settings(new DictionaryValue());
   scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>());
   std::string value_as_json;
   leveldb::WriteBatch batch;
 
   for (DictionaryValue::key_iterator it = settings.begin_keys();
       it != settings.end_keys(); ++it) {
-    scoped_ptr<Value> original_value;
-    if (!ReadFromDb(leveldb::ReadOptions(), *it, &original_value)) {
+    scoped_ptr<Value> old_value;
+    if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) {
       return Result(kGenericOnFailureMessage);
     }
 
     Value* new_value = NULL;
     settings.GetWithoutPathExpansion(*it, &new_value);
-    if (!original_value.get() || !original_value->Equals(new_value)) {
+    if (!old_value.get() || !old_value->Equals(new_value)) {
       changed_keys->insert(*it);
       base::JSONWriter::Write(new_value, false, &value_as_json);
       batch.Put(*it, value_as_json);
     }
+
+    if (old_value.get()) {
+      old_settings->SetWithoutPathExpansion(*it, old_value.release());
+    }
   }
 
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
@@ -181,7 +186,8 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set(
     return Result(kGenericOnFailureMessage);
   }
 
-  return Result(settings.DeepCopy(), changed_keys.release());
+  return Result(
+      settings.DeepCopy(), old_settings.release(), changed_keys.release());
 }
 
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove(
@@ -195,18 +201,20 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove(
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove(
     const std::vector<std::string>& keys) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<DictionaryValue> old_settings(new DictionaryValue());
   scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>());
 
   leveldb::WriteBatch batch;
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
-    scoped_ptr<Value> original_value;
-    if (!ReadFromDb(leveldb::ReadOptions(), *it, &original_value)) {
+    scoped_ptr<Value> old_value;
+    if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) {
       return Result(kGenericOnFailureMessage);
     }
 
-    if (original_value.get()) {
+    if (old_value.get()) {
       changed_keys->insert(*it);
+      old_settings->SetWithoutPathExpansion(*it, old_value.release());
       batch.Delete(*it);
     }
   }
@@ -217,11 +225,12 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove(
     return Result(kGenericOnFailureMessage);
   }
 
-  return Result(NULL, changed_keys.release());
+  return Result(NULL, old_settings.release(), changed_keys.release());
 }
 
 ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  scoped_ptr<DictionaryValue> old_settings(new DictionaryValue());
   scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>());
   leveldb::ReadOptions options;
   // All interaction with the db is done on the same thread, so snapshotting
@@ -232,8 +241,18 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() {
   options.snapshot = snapshot.get();
   scoped_ptr<leveldb::Iterator> it(db_->NewIterator(options));
   for (it->SeekToFirst(); it->Valid(); it->Next()) {
-    changed_keys->insert(it->key().ToString());
-    batch.Delete(it->key());
+    const std::string key = it->key().ToString();
+    const std::string old_value_as_json = it->value().ToString();
+    changed_keys->insert(key);
+    Value* old_value =
+        base::JSONReader().JsonToValue(old_value_as_json, false, false);

[Matt Perry, 2011/10/07 22:39:52]

I don't understand this conversion to/from json. What's wrong with DeepCopy?

[not at google - send to devlin, 2011/10/10 01:00:16]

I don't understand... what conversion?

[Matt Perry, 2011/10/10 20:37:15]

old_value is the same as it->value()->DeepCopy(), but you convert it to and from
json first.

[not at google - send to devlin, 2011/10/10 22:57:53]

Still puzzled, "it" is a leveldb iterator, it->value() isn't a Value but rather
some leveldb thing?

[Matt Perry, 2011/10/10 23:01:17]

Oops, my mistake! Reading comprehension fail... Ignore me.

[not at google - send to devlin, 2011/10/11 03:58:48]

np

+    if (old_value) {
+      old_settings->SetWithoutPathExpansion(key, old_value);
+    } else {
+      // TODO(kalman): clear the offending non-JSON value from the database.
+      LOG(ERROR) << "Invalid JSON in database: " << old_value_as_json;
+    }
+    batch.Delete(key);
   }
 
   if (!it->status().ok()) {
@@ -247,7 +266,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() {
     return Result(kGenericOnFailureMessage);
   }
 
-  return Result(NULL, changed_keys.release());
+  return Result(NULL, old_settings.release(), changed_keys.release());
 }
 
 bool ExtensionSettingsLeveldbStorage::ReadFromDb(