Enable sync for the settings from the Extension Settings API.

chrome/browser/extensions/extension_settings.cc

Index: chrome/browser/extensions/extension_settings.cc
diff --git a/chrome/browser/extensions/extension_settings.cc b/chrome/browser/extensions/extension_settings.cc
index 0d855b95c9acb3747cc00bc25d3195fa4f2ac936..bd3feddcc84e3b92e3a180bc6580da78d5d21a04 100644
--- a/chrome/browser/extensions/extension_settings.cc
+++ b/chrome/browser/extensions/extension_settings.cc
@@ -8,72 +8,95 @@
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
+#include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
-#include "content/browser/browser_thread.h"
 #include "chrome/browser/extensions/extension_settings_leveldb_storage.h"
 #include "chrome/browser/extensions/extension_settings_noop_storage.h"
 #include "chrome/browser/extensions/extension_settings_storage_cache.h"
+#include "chrome/browser/extensions/extension_settings_sync_util.h"
+#include "chrome/common/extensions/extension.h"
+#include "content/browser/browser_thread.h"
 #include "third_party/leveldatabase/src/include/leveldb/iterator.h"
 #include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
 
 ExtensionSettings::ExtensionSettings(const FilePath& base_path)
-    : base_path_(base_path) {}
+    : base_path_(base_path),
+      extension_service_(NULL),
+      sync_processor_(NULL) {}

[akalin, 2011/09/15 19:56:44]

add DCHECK for FILE thread

[not at google - send to devlin, 2011/09/16 05:18:59]

... except it doesn't need to be constructed on the FILE thread, and it would be
kind of a pain to need to do that.  Well, probably not *that* much of a pain,
but it would make the code which constructs these need to be asynchronous, and
then refcount or something because they need to be injected elsewhere
immediately on construction... ugh.

[akalin, 2011/09/16 15:36:40]

An object which is constructed/destroyed on one thread but used on another
doesn't make sense by itself.  It may make sense in context of the whole
program, where you know how it's used, but it's much more preferable to have
objects that can be understood by themselves.

Yeah, it's more complex to do things correctly.  But the reason why the simple
way is simple is because it's wrong. :-) I'd have to take a look at the code
that uses this class, but I bet there are some race conditions lurking in there,
with probably some leaks (using DeleteOn*Thread with a ref-counted object is
basically a recipe for a shutdown leak).

So, I'd much prefer this class to live completely on the FILE thread.  If
necessary, you can push up the ref-counted/delete-on-file-thread grossness to an
inner struct of the owner of this class.

[not at google - send to devlin, 2011/09/19 07:10:46]

I don't see why it can't be understood by itself.  Methods that need to run on
the FILE thread are DCHECKed as such.  This is all methods apart from the
constructor, a very lightweight operation that just sets a pointer to NULL and
constructs an empty map.

Particularly now that there's a clear owner (see below), it's a pain to arrange
it any other way.
Not sure this comment still applies given the changes described below, but let
me know if it does.
Ok, so I'll summarise all the changes regarding this and just say "see comment"
for the other related comments.

You're right -- there's no reason why the ExtensionSettings object needs to be
constructed outside the ExtensionService that I can see.  I originally based the
structure of the code on ExtensionPreferences, but now that I'm more familiar
with the code it looks like that's unnecessarily complicated.

So now the ExtensionService just owns and constructs its own ExtensionSettings. 
This is much clearer.  The thread restriction on construction is whatever the
ExtensionService's restriction is (since there are no restrictions), this allows
simple/synchronous construction, I can't see why now there'd be any other way
that makes sense.  Then the service ensures that it's destroyed on the FILE
thread (DeleteSoon without any ref counting).

 
 ExtensionSettings::~ExtensionSettings() {
-  std::map<std::string, ExtensionSettingsStorage*>::iterator it;
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator it;

[akalin, 2011/09/15 19:56:44]

add DCHECK for FILE thread

[not at google - send to devlin, 2011/09/16 05:18:59]

Ditto above comment, which is why they're deleted with
BrowserThread::DeleteSoon().

Although come to think of it, destruction of this still isn't thread safe, since
storage_objs_ isn't thread safe.  Bummer.  I'll push the
BrowserThread::DeleteSoon call up the stack.  Which means I can make the
SyncableExtensionSettingsStorage* a
linked_ptr<SyncableExtensionSettingsStorage>.  Yay, 80 character long iterator
name!

[akalin, 2011/09/16 15:36:40]

See earlier comment.  If this class lives completely on the FILE thread, there's
no need for DeleteSoon.  linked_ptr<> may be a good idea for storage_objs_. 
Keep in mind that linked_ptr<> is *not* thread-safe.

[not at google - send to devlin, 2011/09/19 07:10:46]

Yep, I made this change in the version I uploaded before.

   for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
     BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, it->second);
   }
 }
 
+void ExtensionSettings::SetExtensionService(
+    ExtensionServiceInterface* extension_service) {
+  extension_service_ = extension_service;
+}
+
 ExtensionSettingsStorage* ExtensionSettings::GetStorage(
-    const std::string& extension_id) {
+    const std::string& extension_id) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DictionaryValue empty;

[akalin, 2011/09/15 19:56:44]

i prefer just inlining this, e.g.

return GetOrCreate(..., DictionaryValue())

but it's up to you

[not at google - send to devlin, 2011/09/16 05:18:59]

Ah right.  Yes, I would do that but DictionaryValue() has no copy constructor so
C++ doesn't like it.

[akalin, 2011/09/16 15:36:40]

Right, I forgot about that. Ugh.

+  return GetOrCreateStorageWithSyncData(extension_id, empty);
+}
 
-  std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
-      storage_objs_.find(extension_id);
-  if (existing != storage_objs_.end()) {
-    return existing->second;
-  }
-
-  ExtensionSettingsStorage* new_storage =
-      CreateStorage(extension_id, ExtensionSettingsStorage::LEVELDB, true);
-  if (new_storage == NULL) {
-    // Failed to create a leveldb storage for some reason.  Use an in memory
-    // storage area (no-op wrapped in a cache) instead.
-    new_storage =
-        CreateStorage(extension_id, ExtensionSettingsStorage::NOOP, true);
-    DCHECK(new_storage != NULL);
+SyncableExtensionSettingsStorage*
+ExtensionSettings::GetOrCreateStorageWithSyncData(
+    const std::string& extension_id, const DictionaryValue& sync_data) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  SyncableExtensionSettingsStorage* storage = GetOrCreateAndInitStorage(
+      ExtensionSettingsStorage::LEVELDB,
+      true,
+      extension_id,
+      sync_data);
+  if (!storage) {

[akalin, 2011/09/15 19:56:44]

What happens in this case?  That means that the settings won't be written to
disk, but will be in sync?  This isn't very comment is it?  Add a comment
explaining how this works.

[not at google - send to devlin, 2011/09/16 05:18:59]

I thought about this earlier, and I think it's fine.  If the leveldb storage
doesn't initialise and the in-memory version is active and syncing, then it just
means that the next time the browser starts it (presumably) won't be in sync. 
But that's fine... the sync initialisation logic is to overwrite local settings
with those from sync.

Comment added.

+    // Fall back to an in-memory storage area (cached NOOP).
+    storage = GetOrCreateAndInitStorage(
+        ExtensionSettingsStorage::NOOP,
+        true,
+        extension_id,
+        sync_data);
+    DCHECK(storage);
   }
-
-  storage_objs_[extension_id] = new_storage;
-  return new_storage;
+  return storage;
 }
 
 ExtensionSettingsStorage* ExtensionSettings::GetStorageForTesting(
     ExtensionSettingsStorage::Type type,
     bool cached,
-    const std::string& extension_id) {
+    const std::string& extension_id) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DictionaryValue empty;
+  ExtensionSettingsStorage* storage =
+      GetOrCreateAndInitStorage(type, cached, extension_id, empty);
+  DCHECK(storage);
+  return storage;
+}
 
-  std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
+SyncableExtensionSettingsStorage* ExtensionSettings::GetOrCreateAndInitStorage(
+    ExtensionSettingsStorage::Type type,
+    bool cached,
+    const std::string& extension_id,
+    const DictionaryValue& initial_sync_data) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator existing =
       storage_objs_.find(extension_id);
   if (existing != storage_objs_.end()) {
     return existing->second;
   }
-
-  ExtensionSettingsStorage* new_storage =
-      CreateStorage(extension_id, type, cached);
-  DCHECK(new_storage != NULL);
-  storage_objs_[extension_id] = new_storage;
-  return new_storage;
+  return CreateAndInitStorage(type, cached, extension_id, initial_sync_data);
 }
 
-ExtensionSettingsStorage* ExtensionSettings::CreateStorage(
-    const std::string& extension_id,
+SyncableExtensionSettingsStorage* ExtensionSettings::CreateAndInitStorage(
     ExtensionSettingsStorage::Type type,
-    bool cached) {
+    bool cached,
+    const std::string& extension_id,
+    const DictionaryValue& initial_sync_data) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(storage_objs_.count(extension_id) == 0);

[akalin, 2011/09/15 19:56:44]

DCHECK_EQ

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

   ExtensionSettingsStorage* storage = NULL;
   switch (type) {
     case ExtensionSettingsStorage::NOOP:
@@ -86,8 +109,154 @@ ExtensionSettingsStorage* ExtensionSettings::CreateStorage(
     default:
       NOTREACHED();
   }
-  if (storage != NULL && cached) {
+  if (!storage) {
+    return NULL;
+  }
+  if (cached) {
     storage = new ExtensionSettingsStorageCache(storage);
   }
-  return storage;
+
+  SyncableExtensionSettingsStorage* synced_storage =
+      new SyncableExtensionSettingsStorage(extension_id, storage);
+  if (sync_processor_) {
+    synced_storage->StartSyncing(initial_sync_data, sync_processor_);
+  }
+  storage_objs_[extension_id] = synced_storage;
+  return synced_storage;
+}
+
+SyncDataList ExtensionSettings::GetAllSyncData(
+    syncable::ModelType type) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK_EQ(type, syncable::EXTENSION_SETTINGS);
+  DCHECK(extension_service_);
+
+  // For all extensions, get all their settings.
+  // This has the effect of bringing in the entire state of extension settings
+  // in memory; sad.
+  SyncDataList all_sync_data;

[akalin, 2011/09/15 19:56:44]

I think the logic that gathers all the extensions should be in ExtensionService,
as adding another extension type would silently break this.  Maybe add a
GetAllExtensions() method to it and use that?  Or since you only really need the
ID, maybe GetAllExtensionIDs()

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+  AppendAllSyncData(&all_sync_data, *extension_service_->extensions());
+  AppendAllSyncData(&all_sync_data,
+      *extension_service_->disabled_extensions());
+  AppendAllSyncData(&all_sync_data,
+      *extension_service_->terminated_extensions());
+  return all_sync_data;
+}
+
+void ExtensionSettings::AppendAllSyncData(
+    SyncDataList* sync_data, const ExtensionList& extensions) const {
+  for (ExtensionList::const_iterator it = extensions.begin();

[akalin, 2011/09/15 19:56:44]

DCHECK, file thread

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+      it != extensions.end(); ++it) {
+    std::string extension_id = (*it)->id();
+    ExtensionSettingsStorage::Result maybe_settings =
+        GetStorage(extension_id)->Get();
+    if (maybe_settings.HasError()) {
+      LOG(WARNING) << "Failed to get settings for " << extension_id << ": " <<
+          maybe_settings.GetError();
+      continue;
+    }
+
+    DictionaryValue* settings = maybe_settings.GetSettings();
+    for (DictionaryValue::key_iterator key_it = settings->begin_keys();
+        key_it != settings->end_keys(); ++key_it) {
+      Value *value;

[akalin, 2011/09/15 19:56:44]

 = NULL

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+      settings->GetWithoutPathExpansion(*key_it, &value);
+      sync_data->push_back(
+          extension_settings_sync_util::CreateData(
+              extension_id, *key_it, *value));
+    }
+  }
+}
+
+SyncError ExtensionSettings::MergeDataAndStartSyncing(
+    syncable::ModelType type,
+    const SyncDataList& initial_sync_data,
+    SyncChangeProcessor* sync_processor) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK_EQ(type, syncable::EXTENSION_SETTINGS);
+  DCHECK(!sync_processor_);
+  sync_processor_ = sync_processor;
+
+  // Group the initial sync data by extension id.
+  std::map<std::string, linked_ptr<DictionaryValue> > grouped_sync_data;
+  for (SyncDataList::const_iterator it = initial_sync_data.begin();
+      it != initial_sync_data.end(); ++it) {
+    ExtensionSettingSyncData data(*it);
+    if (!data.value()) {
+      LOG(WARNING) << "NULL value in sync data";

[akalin, 2011/09/15 19:56:44]

prefer

LOG(WARNING) << "Could not parse setting: " <<
it->GetSpecifics().GetExtension(...).key() << " " << ...value();

[not at google - send to devlin, 2011/09/16 05:18:59]

Changing Value* to Value& made this moot.  Which is a good thing.

+      continue;
+    }
+    linked_ptr<DictionaryValue> sync_data =
+        grouped_sync_data[data.extension_id()];
+    if (!sync_data.get()) {
+      sync_data = linked_ptr<DictionaryValue>(new DictionaryValue());
+      grouped_sync_data[data.extension_id()] = sync_data;
+    }
+    DCHECK(!sync_data->HasKey(data.key())) <<
+        "Duplicate settings for " << data.extension_id() << "/" << data.key();
+    sync_data->Set(data.key(), data.value()->DeepCopy());
+  }
+
+  // Start syncing all existing storage areas.  Any storage areas created in
+  // the future will start being synced as part of the creation process.
+  for (std::map<std::string, SyncableExtensionSettingsStorage*>::iterator it =
+      storage_objs_.begin(); it != storage_objs_.end(); ++it) {
+    linked_ptr<DictionaryValue> sync_data = grouped_sync_data[it->first];

[akalin, 2011/09/16 15:36:40]

prefer using group_sync_data.find() so that you don't auto-create a value if it
doesn't exist.

[not at google - send to devlin, 2011/09/19 07:10:46]

Done.

+    if (sync_data.get()) {
+      it->second->StartSyncing(*sync_data, sync_processor);
+    } else {

[akalin, 2011/09/15 19:56:44]

from the logic above, I don't think sync_data should ever be NULL.  add
DCHECK(sync_data.get())

[not at google - send to devlin, 2011/09/16 05:18:59]

I think it can be NULL if sync data exists for storage areas that haven't been
seen locally yet.  These would appear in grouped_sync_data but not
storage_objs_.

[akalin, 2011/09/16 15:36:40]

Ah, I see, right.

+      DictionaryValue empty;
+      it->second->StartSyncing(empty, sync_processor);
+    }
+    grouped_sync_data.erase(it->first);

[akalin, 2011/09/15 19:56:44]

no need for this, and in fact it's dangerous given that you're invaliding it and
then incrementing it

[not at google - send to devlin, 2011/09/16 05:18:59]

Should be safe, I'm incrementing an iterator to storage_objs_ not
grouped_sync_data?

Or am I confused?

Is there some idiomatic way to make this clearer?

[akalin, 2011/09/16 15:36:40]

My mistake, I didn't see you were iterating over storage_objs_.

May as well move this into the if statement (see earlier comment about using
group_sync_data.find())

[not at google - send to devlin, 2011/09/19 07:10:46]

Done.

+  }
+
+  // Eagerly create and init the rest of the storage areas that have sync data.
+  // Under normal circumstances (i.e. not first-time sync) this will be all of
+  // them.
+  for (std::map<std::string, linked_ptr<DictionaryValue> >::iterator it =
+      grouped_sync_data.begin(); it != grouped_sync_data.end(); ++it) {
+    GetOrCreateStorageWithSyncData(it->first, *it->second);
+  }
+
+  return SyncError();
+}
+
+SyncError ExtensionSettings::ProcessSyncChanges(
+    const tracked_objects::Location& from_here,
+    const SyncChangeList& sync_changes) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+
+  // Group changes by extension, to pass all changes in a single method call.
+  std::map<std::string, ExtensionSettingSyncDataList> grouped_sync_data;
+  for (SyncChangeList::const_iterator it = sync_changes.begin();
+      it != sync_changes.end(); ++it) {
+    ExtensionSettingSyncData data(*it);
+    if (!data.value()) {
+      LOG(WARNING) << "NULL value in sync data";

[akalin, 2011/09/15 19:56:44]

revamp error message as above (possibly decomp into utility method)

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+      continue;
+    }
+    grouped_sync_data[data.extension_id()].push_back(data);
+  }
+
+  std::map<std::string, ExtensionSettingSyncDataList>::iterator it;

[akalin, 2011/09/15 19:56:44]

move into for loop

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+  DictionaryValue empty;
+  for (it = grouped_sync_data.begin(); it != grouped_sync_data.end(); ++it) {
+    GetOrCreateStorageWithSyncData(it->first, empty)->
+        ProcessSyncChanges(it->second);
+  }
+
+  return SyncError();
+}
+
+void ExtensionSettings::StopSyncing(syncable::ModelType type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+  sync_processor_ = NULL;
+
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator it;

[akalin, 2011/09/15 19:56:44]

fold into for loop

[not at google - send to devlin, 2011/09/16 05:18:59]

Done.

+  for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
+    it->second->StopSyncing();
+  }
 }