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

chrome/browser/extensions/syncable_extension_settings_storage.cc

Index: chrome/browser/extensions/syncable_extension_settings_storage.cc
diff --git a/chrome/browser/extensions/syncable_extension_settings_storage.cc b/chrome/browser/extensions/syncable_extension_settings_storage.cc
index f9820f44a4cd7118c0b5dab4bf368db5de8df7a4..c3503e76479fc0716fcebb03416c0260961e7d13 100644
--- a/chrome/browser/extensions/syncable_extension_settings_storage.cc
+++ b/chrome/browser/extensions/syncable_extension_settings_storage.cc
@@ -11,8 +11,14 @@
 #include "content/browser/browser_thread.h"
 
 SyncableExtensionSettingsStorage::SyncableExtensionSettingsStorage(
-    std::string extension_id, ExtensionSettingsStorage* delegate)
-    : extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) {
+    const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >&
+        observers,
+    const std::string& extension_id,
+    ExtensionSettingsStorage* delegate)
+    : observers_(observers),
+      extension_id_(extension_id),
+      delegate_(delegate),
+      sync_processor_(NULL) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 }
 
@@ -118,7 +124,7 @@ SyncError SyncableExtensionSettingsStorage::StartSyncing(
         syncable::EXTENSION_SETTINGS);
   }
 
-  DictionaryValue* settings = maybe_settings.GetSettings();
+  const DictionaryValue* settings = maybe_settings.GetSettings();
   if (sync_state.empty()) {
     return SendLocalSettingsToSync(*settings);
   }
@@ -224,43 +230,86 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges(
   DCHECK(!sync_changes.empty()) << "No sync changes for " << extension_id_;
 
   std::vector<SyncError> errors;
+  ExtensionSettingChanges::Builder changes;
 
   for (ExtensionSettingSyncDataList::const_iterator it = sync_changes.begin();
       it != sync_changes.end(); ++it) {
     DCHECK_EQ(extension_id_, it->extension_id());
+
+    const std::string& key = it->key();
+    const Value& value = it->value();
+
+    scoped_ptr<Value> current_value;
+    {
+      Result maybe_settings = Get(it->key());
+      if (maybe_settings.HasError()) {
+        errors.push_back(SyncError(
+            FROM_HERE,
+            std::string("Error getting current sync state for ") +
+                extension_id_ + "/" + key + ": " + maybe_settings.GetError(),
+            syncable::EXTENSION_SETTINGS));
+        continue;
+      }
+      const DictionaryValue* settings = maybe_settings.GetSettings();
+      if (settings) {
+        Value* value = NULL;
+        if (settings->GetWithoutPathExpansion(key, &value)) {
+          current_value.reset(value->DeepCopy());
+        }
+      }
+    }
+
+    SyncError error;
+
     switch (it->change_type()) {
       case SyncChange::ACTION_ADD:
-      case SyncChange::ACTION_UPDATE: {
-        synced_keys_.insert(it->key());
-        Result result = delegate_->Set(it->key(), it->value());
-        if (result.HasError()) {
-          errors.push_back(SyncError(
-              FROM_HERE,
-              std::string("Error pushing sync change to local settings: ") +
-                  result.GetError(),
-              syncable::EXTENSION_SETTINGS));
+        if (!current_value.get()) {
+          error = OnSyncAdd(key, value, &changes);
+        } else {
+          // Already a value; hopefully a local change has beaten sync in a
+          // race and it's not a bug, so pretend it's an update.
+          LOG(WARNING) << "Got add from sync for existing setting " <<
+              extension_id_ << "/" << key;
+          error = OnSyncUpdate(key, *current_value, value, &changes);
         }
         break;
-      }
 
-      case SyncChange::ACTION_DELETE: {
-        synced_keys_.erase(it->key());
-        Result result = delegate_->Remove(it->key());
-        if (result.HasError()) {
-          errors.push_back(SyncError(
-              FROM_HERE,
-              std::string("Error removing sync change from local settings: ") +
-                  result.GetError(),
-              syncable::EXTENSION_SETTINGS));
+      case SyncChange::ACTION_UPDATE:
+        if (current_value.get()) {
+          error = OnSyncUpdate(key, *current_value, value, &changes);
+        } else {
+          // Similarly, pretend it's an add.
+          LOG(WARNING) << "Got update from sync for nonexistent setting" <<
+              extension_id_ << "/" << key;
+          error = OnSyncAdd(key, value, &changes);
+        }
+        break;
+
+      case SyncChange::ACTION_DELETE:
+        if (current_value.get()) {
+          error = OnSyncDelete(key, *current_value, &changes);

[akalin, 2011/10/17 17:37:38]

make OnSyncDelete take a Value* and pass current_value.release().  Similarly for
other functions.

[not at google - send to devlin, 2011/10/17 23:05:44]

Done.

+        } else {
+          // Similarly, ignore it.
+          LOG(WARNING) << "Got delete from sync for nonexistent setting " <<
+              extension_id_ << "/" << key;
         }
         break;
-      }
 
       default:
         NOTREACHED();
     }
+
+    if (error.IsSet()) {
+      errors.push_back(error);
+    }
   }
 
+  observers_->Notify(
+      &ExtensionSettingsObserver::OnSettingsChanged,
+      static_cast<Profile*>(NULL),
+      extension_id_,
+      changes.Build());
+
   return errors;
 }
 
@@ -307,11 +356,18 @@ void SyncableExtensionSettingsStorage::SendDeletesToSync(
   SyncChangeList changes;
   for (std::set<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
-    DCHECK(synced_keys_.count(*it));
+    if (!synced_keys_.count(*it)) {
+      LOG(WARNING) << "Deleted " << *it << " but no entry in synced_keys";
+      continue;
+    }
     changes.push_back(
         extension_settings_sync_util::CreateDelete(extension_id_, *it));
   }
 
+  if (changes.empty()) {
+    return;
+  }
+
   SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
   if (error.IsSet()) {
     LOG(WARNING) << "Failed to send changes to sync: " << error.message();
@@ -323,3 +379,54 @@ void SyncableExtensionSettingsStorage::SendDeletesToSync(
     synced_keys_.erase(*it);
   }
 }
+
+SyncError SyncableExtensionSettingsStorage::OnSyncAdd(
+    const std::string& key,
+    const Value& new_value,
+    ExtensionSettingChanges::Builder* changes) {
+  synced_keys_.insert(key);
+  Result result = delegate_->Set(key, new_value);
+  if (result.HasError()) {
+    return SyncError(
+        FROM_HERE,
+        std::string("Error pushing sync add to local settings: ") +
+            result.GetError(),
+        syncable::EXTENSION_SETTINGS);
+  }
+  changes->AppendChange(key, NULL, new_value.DeepCopy());
+  return SyncError();
+}
+
+SyncError SyncableExtensionSettingsStorage::OnSyncUpdate(
+    const std::string& key,
+    const Value& old_value,
+    const Value& new_value,
+    ExtensionSettingChanges::Builder* changes) {
+  Result result = delegate_->Set(key, new_value);
+  if (result.HasError()) {
+    return SyncError(
+        FROM_HERE,
+        std::string("Error pushing sync update to local settings: ") +
+            result.GetError(),
+        syncable::EXTENSION_SETTINGS);
+  }
+  changes->AppendChange(key, old_value.DeepCopy(), new_value.DeepCopy());
+  return SyncError();
+}
+
+SyncError SyncableExtensionSettingsStorage::OnSyncDelete(
+    const std::string& key,
+    const Value& old_value,
+    ExtensionSettingChanges::Builder* changes) {
+  synced_keys_.erase(key);
+  Result result = delegate_->Remove(key);
+  if (result.HasError()) {
+    return SyncError(
+        FROM_HERE,
+        std::string("Error pushing sync remove to local settings: ") +
+            result.GetError(),
+        syncable::EXTENSION_SETTINGS);
+  }
+  changes->AppendChange(key, old_value.DeepCopy(), NULL);
+  return SyncError();
+}