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..982310f0bc224997f795c810258143329db95fc3 100644
--- a/chrome/browser/extensions/extension_settings.cc
+++ b/chrome/browser/extensions/extension_settings.cc
@@ -4,76 +4,94 @@
 
 #include "chrome/browser/extensions/extension_settings.h"
 
-#include "base/bind.h"
 #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) {}
 
 ExtensionSettings::~ExtensionSettings() {
-  std::map<std::string, ExtensionSettingsStorage*>::iterator it;
-  for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
-    BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, it->second);
-  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+}
+
+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;
+  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) {
+    // 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 =
-      storage_objs_.find(extension_id);
+SyncableExtensionSettingsStorage* ExtensionSettings::GetOrCreateAndInitStorage(
+    ExtensionSettingsStorage::Type type,
+    bool cached,
+    const std::string& extension_id,
+    const DictionaryValue& initial_sync_data) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  StorageObjMap::iterator existing = storage_objs_.find(extension_id);
   if (existing != storage_objs_.end()) {
-    return existing->second;
+    return existing->second.get();
   }
-
-  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_EQ(0u, storage_objs_.count(extension_id));
   ExtensionSettingsStorage* storage = NULL;
   switch (type) {
     case ExtensionSettingsStorage::NOOP:
@@ -86,8 +104,138 @@ 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] =
+      linked_ptr<SyncableExtensionSettingsStorage>(synced_storage);
+  return synced_storage;
+}
+
+SyncDataList ExtensionSettings::GetAllSyncData(
+    syncable::ModelType type) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK_EQ(type, syncable::EXTENSION_SETTINGS);
+
+  // 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;
+  std::vector<std::string> extension_ids(

[akalin, 2011/09/17 08:44:47]

Wait, this will include apps, too, right?  Surely we must filter those out.

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

Apps are fine, no reason to filter them out?

Anyway, no longer needed anyway.

[akalin, 2011/09/19 07:49:16]

Well, you can turn extension sync and app sync on and off independently.  So if
we tie extensions setting sync to extension sync but also sync apps settings,
then that's surprising if a user turned off app sync.

As I mentioned before, we have to figure out the story with apps.

[not at google - send to devlin, 2011/09/19 08:42:19]

Ah right, that's what you meant.  Interesting.  This is something that
ExtensionSettings will need to be aware of since it's responsible for
interpreting the settings data type per-extension.

Doesn't sound hard, but hopefully something for a follow-up patch, this one is
big enough.

+      extension_service_->GetAllExtensionIds());

[akalin, 2011/09/17 08:44:47]

hmm, problem.  I'm pretty sure ExtensionService lives on the UI thread.  So we
either have to make GetAllExtensionIds explicitly thread-safe, or figure
something else out.

Actually, why do we have to grab the current list of extensions and then loop
over that, possibly creating new storage objects?  If the storage lives on the
FILE thread, shouldn't there be a way to access all currently-existing storage
objects without having to consult the extension service?  I think it's okay if
there are some extensions without settings, yet.

This'll remove the dependency on ExtensionService, too.

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

Yeah, this cleans up a whole lot of stuff.  See comment I made about this
earlier.

+
+  for (std::vector<std::string>::iterator it = extension_ids.begin();

[akalin, 2011/09/17 08:44:47]

const_iterator

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

Done.

+      it != extension_ids.end(); ++it) {
+    ExtensionSettingsStorage::Result maybe_settings = GetStorage(*it)->Get();
+    if (maybe_settings.HasError()) {
+      LOG(WARNING) << "Failed to get settings for " << *it << ": " <<
+          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 = NULL;
+      settings->GetWithoutPathExpansion(*key_it, &value);
+      all_sync_data.push_back(
+          extension_settings_sync_util::CreateData(*it, *key_it, *value));
+    }
+  }
+
+  return all_sync_data;
+}
+
+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);
+    linked_ptr<DictionaryValue> sync_data =

[akalin, 2011/09/17 08:44:47]

Didn't address my previous comment:

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:47]

Why here?  Unlike below where you can avoid unnecessary construction, the
linked_ptr will be created on the next line anyway.  In terms of efficiency I
don't know whether the construction of a map iterator is lighter than the
assignment operation of a linked_ptr, but either way, it's hardly an important
consideration.  On the other hand using find() would be more verbose than this
NULL-checking way.

[akalin, 2011/09/19 19:24:11]

You're right, I think this is fine.

+        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 (StorageObjMap::iterator it = storage_objs_.begin();
+      it != storage_objs_.end(); ++it) {
+    linked_ptr<DictionaryValue> sync_data = grouped_sync_data[it->first];

[akalin, 2011/09/17 08:44:47]

use find() here, too

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

Done.

+    if (sync_data.get()) {
+      it->second->StartSyncing(*sync_data, sync_processor);
+    } else {
+      DictionaryValue empty;
+      it->second->StartSyncing(empty, sync_processor);
+    }
+    grouped_sync_data.erase(it->first);
+  }
+
+  // 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);
+    grouped_sync_data[data.extension_id()].push_back(data);
+  }
+
+  DictionaryValue empty;
+  for (std::map<std::string, ExtensionSettingSyncDataList>::iterator
+      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;
+
+  for (StorageObjMap::iterator it = storage_objs_.begin();
+      it != storage_objs_.end(); ++it) {
+    it->second->StopSyncing();
+  }
 }