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

chrome/browser/extensions/extension_settings_api.cc

Index: chrome/browser/extensions/extension_settings_api.cc
diff --git a/chrome/browser/extensions/extension_settings_api.cc b/chrome/browser/extensions/extension_settings_api.cc
index 5da832fa3a359593364aae9814d008e68bf765d0..c779b4a665044daa2d6a13445ce47ab5608e7d07 100644
--- a/chrome/browser/extensions/extension_settings_api.cc
+++ b/chrome/browser/extensions/extension_settings_api.cc
@@ -25,7 +25,7 @@ bool SettingsFunction::RunImpl() {
 void SettingsFunction::RunWithBackendOnFileThread(
     ExtensionSettingsBackend* backend) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  bool success = RunWithStorage(backend->GetStorage(extension_id()));
+  bool success = RunWithStorage(backend, backend->GetStorage(extension_id()));
   BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
@@ -33,13 +33,36 @@ void SettingsFunction::RunWithBackendOnFileThread(
 }
 
 bool SettingsFunction::UseResult(
+    ExtensionSettingsBackend* backend,
     const ExtensionSettingsStorage::Result& storage_result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (storage_result.HasError()) {
     error_ = storage_result.GetError();
     return false;
   }
+
   DictionaryValue* settings = storage_result.GetSettings();
   result_.reset(settings == NULL ? NULL : settings->DeepCopy());
+
+  std::set<std::string>* changed_keys = storage_result.GetChangedKeys();
+  if (changed_keys && !changed_keys->empty()) {
+    ExtensionSettingsObserver::ChangeList change_event;
+    for (std::set<std::string>::iterator it = changed_keys->begin();
+        it != changed_keys->end(); ++it) {
+      Value* old_value = NULL;
+      if (storage_result.GetOldValue(*it, &old_value) && old_value) {
+        old_value = old_value->DeepCopy();
+      }
+      Value* new_value = NULL;
+      if (storage_result.GetNewValue(*it, &new_value) && new_value) {
+        new_value = new_value->DeepCopy();
+      }
+      change_event.AppendChange(*it, old_value, new_value);
+    }
+    backend->TriggerOnSettingsChanged(

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

It seems weird to leave it up to the consumer of ExtensionSettingsStorage to
trigger the onChanged event. Why not have ExtensionSettingStorage itself trigger
it? Maybe Set() and Remove() could have implementations, and call through to
SetImpl/RemoveImpl virtual methods, and fire the onChanged event afterwards.

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

I figured it wasn't so bad since there's only 2 sources of changes, sync (needs
to notify all background pages) and set/remove from one background page to the
other.  There will actually be a 3rd at some point (enterprise policy) and it
might get more complicated when content scripts get settings, whenever that is.

I thought about how to do this within the storage objects, since conceptually
it's nicer, as you say.  I don't really want to have to propagate a SetImpl and
RemoveImpl all the way through the interface of ExtensionSettingsStorage... what
would that mean for the storage implementations that don't notify?  That Set and
SetImpl (etc) do the same thing?

The reason I decided to do it this way is because the notifications logic needs
access to the Profile* for controlling which profile the notification goes to,
so this would need to be passed through the Set/Remove methods.  Probably with a
wrapping struct, but still.

My preferred way of doing this would be to add
SyncableExtensionSettingsStorage-only methods called like
SetAndNotify/RemoveAndNotify.  SyncableExtensionSettingStorage then becomes an
inaccurate name, it's more like the outer storage object which handles
side-effectful operations, but, whatever.

Which is why the simpler option I figured was this one.

Thoughts?

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

After thinking about it more, having the notification be separate from
Set/Remove allows you to aggregate several changes into 1 event, which looks
useful for sync. I'm OK with separating them this way.

+        profile(), extension_id(), change_event.GetEventJson());
+  }
+
   return true;
 }
 
@@ -58,47 +81,57 @@ static void AddAllStringValues(
 }
 
 bool GetSettingsFunction::RunWithStorage(
+    ExtensionSettingsBackend* backend,
     ExtensionSettingsStorage* storage) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   Value *input;
   EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input));
   std::string as_string;
   ListValue* as_list;
   if (input->IsType(Value::TYPE_NULL)) {
-    return UseResult(storage->Get());
+    return UseResult(backend, storage->Get());
   } else if (input->GetAsString(&as_string)) {
-    return UseResult(storage->Get(as_string));
+    return UseResult(backend, storage->Get(as_string));
   } else if (input->GetAsList(&as_list)) {
     std::vector<std::string> string_list;
     AddAllStringValues(*as_list, &string_list);
-    return UseResult(storage->Get(string_list));
+    return UseResult(backend, storage->Get(string_list));
   }
-  return UseResult(ExtensionSettingsStorage::Result(kUnsupportedArgumentType));
+  return UseResult(
+      backend, ExtensionSettingsStorage::Result(kUnsupportedArgumentType));
 }
 
 bool SetSettingsFunction::RunWithStorage(
+    ExtensionSettingsBackend* backend,
     ExtensionSettingsStorage* storage) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DictionaryValue *input;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &input));
-  return UseResult(storage->Set(*input));
+  return UseResult(backend, storage->Set(*input));
 }
 
 bool RemoveSettingsFunction::RunWithStorage(
+    ExtensionSettingsBackend* backend,
     ExtensionSettingsStorage* storage) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   Value *input;
   EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input));
   std::string as_string;
   ListValue* as_list;
   if (input->GetAsString(&as_string)) {
-    return UseResult(storage->Remove(as_string));
+    return UseResult(backend, storage->Remove(as_string));
   } else if (input->GetAsList(&as_list)) {
     std::vector<std::string> string_list;
     AddAllStringValues(*as_list, &string_list);
-    return UseResult(storage->Remove(string_list));
+    return UseResult(backend, storage->Remove(string_list));
   }
-  return UseResult(ExtensionSettingsStorage::Result(kUnsupportedArgumentType));
+  return UseResult(
+      backend, ExtensionSettingsStorage::Result(kUnsupportedArgumentType));
 }
 
 bool ClearSettingsFunction::RunWithStorage(
+    ExtensionSettingsBackend* backend,
     ExtensionSettingsStorage* storage) {
-  return UseResult(storage->Clear());
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  return UseResult(backend, storage->Clear());
 }