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

chrome/browser/extensions/extension_settings_api.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_settings_api.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/browser/browser_thread.h"
 
 namespace {
 const char* kUnsupportedArgumentType = "Unsupported argument type";
 }  // namespace
 
 // SettingsFunction
 
 bool SettingsFunction::RunImpl() {
   profile()->GetExtensionService()->extension_settings_frontend()->
       RunWithBackend(
           base::Bind(&SettingsFunction::RunWithBackendOnFileThread, this));
   return true;
 }
 
 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,
       base::Bind(&SettingsFunction::SendResponse, this, success));
 }
 
 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;
 }
 
 // Concrete settings functions
 
 // Adds all StringValues from a ListValue to a vector of strings.
 static void AddAllStringValues(
     const ListValue& from, std::vector<std::string>* to) {
   DCHECK(to->empty());
   std::string as_string;
   for (ListValue::const_iterator it = from.begin(); it != from.end(); ++it) {
     if ((*it)->GetAsString(&as_string)) {
       to->push_back(as_string);
     }
   }
 }
 
 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());
 }