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..1cb2fd7b21e41ca03d4da98d68a83e2a083bd7a3 100644
--- a/chrome/browser/extensions/syncable_extension_settings_storage.cc
+++ b/chrome/browser/extensions/syncable_extension_settings_storage.cc
@@ -11,8 +11,13 @@
 #include "content/browser/browser_thread.h"
 
 SyncableExtensionSettingsStorage::SyncableExtensionSettingsStorage(
-    std::string extension_id, ExtensionSettingsStorage* delegate)
-    : extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) {
+    ObserverListThreadSafe<ExtensionSettingsObserver>* observers,
+    std::string extension_id,
+    ExtensionSettingsStorage* delegate)
+    : observers_(observers),
+      extension_id_(extension_id),
+      delegate_(delegate),
+      sync_processor_(NULL) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 }
 
@@ -225,25 +230,95 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges(
 
   std::vector<SyncError> errors;
 
+  // Change event to build then send to onChanged listeners of the settings API.
+  ExtensionSettingsObserver::ChangeList change_event;
+
   for (ExtensionSettingSyncDataList::const_iterator it = sync_changes.begin();
       it != sync_changes.end(); ++it) {
     DCHECK_EQ(extension_id_, it->extension_id());
+
+    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 ") +

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

use StringPrintf

[not at google - send to devlin, 2011/10/13 06:40:43]

...

[akalin, 2011/10/13 20:40:41]

Ah, so that's why it happened.  Why? StringPrintf is less typesafe than the
alternatives.  Better typesafe and slightly less readable than readable and not
typesafe.

[Matt Perry, 2011/10/13 23:20:24]

I disagree. StringPrintf is perfectly typesafe if you get the types right :).
Which is easily verifiable.

We have StringPrintf in the codebase because we think it's valuable to use. It
makes the code easier to read.

That said, it's a style issue, so I'm OK with it either way.

+                extension_id_ + "/" + it->key() + ": " +
+                maybe_settings.GetError(),
+            syncable::EXTENSION_SETTINGS));
+        continue;
+      }
+      if (maybe_settings.GetSettings() != NULL) {
+        Value* value_ownership = NULL;
+        maybe_settings.GetSettings()->RemoveWithoutPathExpansion(
+            it->key(), &value_ownership);
+        current_value.reset(value_ownership);
+      }
+    }
+
     switch (it->change_type()) {
-      case SyncChange::ACTION_ADD:
+      case SyncChange::ACTION_ADD: {
+        if (current_value.get()) {
+          errors.push_back(SyncError(

[akalin, 2011/10/07 21:06:23]

i don't know if this should be an error -- it's possible that someone could add
a key locally and an ADD could come in from sync immediately after, right?

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

Probably... maybe?  If somebody adds a key then it will be sent to sync
immediately; what does sync do if it gets a remote sync add after a local sync
add forn the same key?  Does it swallow the add?  Turn it into an update?  Just
send the add anyway?

+              FROM_HERE,
+              std::string("Got add from sync for existing setting ") +
+                  extension_id_ + "/" + it->key(),
+              syncable::EXTENSION_SETTINGS));
+          break;
+        }
+
+        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 add to local settings: ") +
+                  result.GetError(),
+              syncable::EXTENSION_SETTINGS));
+          break;
+        }
+
+        change_event.AppendChange(it->key(), NULL, it->value().DeepCopy());
+        break;
+      }
+
       case SyncChange::ACTION_UPDATE: {
+        if (!current_value.get()) {

[akalin, 2011/10/07 21:06:23]

same here -- local delete, update from sync.  maybe just drop and warn?  or may
be apply anyway. (prefer the latter as it doesn't destroy data)

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

Ditto.

Btw assuming sync *does* just send the add anyway, I'll apply it anyway and use
the result of changed_keys to determine whether a change should be appended to
change_event.

+          errors.push_back(SyncError(
+              FROM_HERE,
+              std::string("Got update from sync for nonexistent setting ") +
+                  extension_id_ + "/" + it->key(),
+              syncable::EXTENSION_SETTINGS));
+          break;
+        }
+
         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: ") +
+              std::string("Error pushing sync update to local settings: ") +
                   result.GetError(),
               syncable::EXTENSION_SETTINGS));
+          break;
         }
+
+        change_event.AppendChange(
+            it->key(), current_value.release(), it->value().DeepCopy());
         break;
       }
 
       case SyncChange::ACTION_DELETE: {
+        if (!current_value.get()) {

[akalin, 2011/10/07 21:06:23]

yeah, this should be a warning, too

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

Ditto.

+          errors.push_back(SyncError(
+              FROM_HERE,
+              std::string("Got delete from sync for nonexistent setting ") +
+                  extension_id_ + "/" + it->key(),
+              syncable::EXTENSION_SETTINGS));
+          break;
+        }
+
         synced_keys_.erase(it->key());
         Result result = delegate_->Remove(it->key());
         if (result.HasError()) {
@@ -252,7 +327,10 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges(
               std::string("Error removing sync change from local settings: ") +
                   result.GetError(),
               syncable::EXTENSION_SETTINGS));
+          break;
         }
+
+        change_event.AppendChange(it->key(), current_value.release(), NULL);
         break;
       }
 
@@ -261,6 +339,12 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges(
     }
   }
 
+  observers_->Notify(
+      &ExtensionSettingsObserver::OnSettingsChanged,
+      static_cast<Profile*>(NULL),
+      extension_id_,
+      change_event.GetEventJson());
+
   return errors;
 }