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 eb8f6e18a70b17df3d201d682d83bc07fc042104..dc90bfe16e664677e7305ed7f3c835d2f478b0ff 100644
--- a/chrome/browser/extensions/extension_settings.cc
+++ b/chrome/browser/extensions/extension_settings.cc
@@ -13,15 +13,14 @@
 #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 "third_party/leveldatabase/src/include/leveldb/iterator.h"
-#include "third_party/leveldatabase/src/include/leveldb/write_batch.h"
+#include "chrome/browser/extensions/syncable_extension_settings_storage.h"
 
 ExtensionSettings::ExtensionSettings(const FilePath& base_path)
-    : base_path_(base_path) {
+    : base_path_(base_path), sync_processor_(NULL) {
 }
 
 ExtensionSettings::~ExtensionSettings() {
-  std::map<std::string, ExtensionSettingsStorage*>::iterator it;
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator it;
   for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
     it->second->DeleteSoon();
   }
@@ -52,14 +51,14 @@ void ExtensionSettings::GetStorageForTesting(
 
 bool ExtensionSettings::GetExistingStorage(
     const std::string& extension_id, const Callback& callback) {
-  std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator existing =
       storage_objs_.find(extension_id);
   if (existing == storage_objs_.end()) {
     // No existing storage.
     return false;
   }
   // Existing storage.  Reply with that.
-  ExtensionSettingsStorage* storage = existing->second;
+  SyncableExtensionSettingsStorage* storage = existing->second;
   DCHECK(storage != NULL);
   MessageLoop::current()->PostTask(
       FROM_HERE,
@@ -72,7 +71,7 @@ bool ExtensionSettings::GetExistingStorage(
 }
 
 void ExtensionSettings::RunWithStorage(
-    Callback* callback, ExtensionSettingsStorage* storage) {
+    Callback* callback, SyncableExtensionSettingsStorage* storage) {
   callback->Run(storage);
   delete callback;
 }
@@ -103,7 +102,8 @@ void ExtensionSettings::CreateStorageOnFileThread(
     bool cached,
     const Callback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  ExtensionSettingsStorage* storage = CreateStorage(extension_id, type, cached);
+  SyncableExtensionSettingsStorage* storage =
+      CreateStorage(extension_id, type, cached);
   if (storage == NULL && fallback_type != type) {
     storage = CreateStorage(extension_id, fallback_type, cached);
   }
@@ -119,7 +119,7 @@ void ExtensionSettings::CreateStorageOnFileThread(
           callback));
 }
 
-ExtensionSettingsStorage* ExtensionSettings::CreateStorage(
+SyncableExtensionSettingsStorage* ExtensionSettings::CreateStorage(
     const std::string& extension_id,
     ExtensionSettingsStorage::Type type,
     bool cached) {
@@ -136,20 +136,26 @@ ExtensionSettingsStorage* ExtensionSettings::CreateStorage(
     default:
       NOTREACHED();
   }
-  if (storage != NULL && cached) {
+  if (storage == NULL) {
+    return NULL;
+  }
+  if (cached) {
     storage = new ExtensionSettingsStorageCache(storage);
   }
-  return storage;
+  // Always send changes to sync.  The merge algorithm is such that local data
+  // will be overwritten by sync data, so even if it's purely in-memory storage
+  // the settings will be consistent across browser restarts.
+  return new SyncableExtensionSettingsStorage(extension_id, storage);
 }
 
 void ExtensionSettings::EndCreationOfStorage(
     const std::string& extension_id,
-    ExtensionSettingsStorage* storage,
+    SyncableExtensionSettingsStorage* storage,
     const Callback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Cache the result now.  To avoid a race condition, check again to see
   // whether a storage has been created already; if so, use that one.
-  std::map<std::string, ExtensionSettingsStorage*>::iterator existing =
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator existing =
       storage_objs_.find(extension_id);
   if (existing == storage_objs_.end()) {
     storage_objs_[extension_id] = storage;
@@ -158,5 +164,147 @@ void ExtensionSettings::EndCreationOfStorage(
     storage = existing->second;
     DCHECK(storage != NULL);
   }
+  if (sync_processor_ != NULL) {
+    StartSyncingStorage(extension_id, storage);
+  }
   callback.Run(storage);
 }
+
+SyncDataList ExtensionSettings::GetAllSyncData(
+    syncable::ModelType type) const {
+  // Not possible to do this synchronously, or while satisfying const, but only

[akalin, 2011/08/31 07:49:34]

Explain to me again why the sync methods can't live on the native thread (i.e.
the FILE thread)?  You're breaking a lot of assumptions sync makes by subverting
it this way, and in general it's hard to reason clearly about classes that
thread-jump like this.

Btw, one of the use cases for GetAllSyncData() is (eventually) DCHECKing that
local_service->GetAllSyncData() == remote_service->GetAllSyncData() after every
call to ProcessSyncChanges.  So having this returning nothing is already a
non-starter.

[not at google - send to devlin, 2011/09/02 05:00:40]

The extension settings code already thread jumps because it needs to go to the
FILE thread for the leveldb stuff, but the extension API handling code lives on
the UI thread.

As for why the sync code lives on the UI thread, it mostly comes down to not
realising that it needs to be on the FILE thread (fwiw it runs fine?).  Also the
way I wrote the code, to avoid bloating up a single file with all the settings
functionality, was to implement the cache/sync/quota(eventually) layers as
wrappers i.e. decorator pattern.  Putting the sync code on the FILE thread would
require entirely subverting this.
Hm ok.  Well I was thinking about this and there are two ways to fix it.
1. pre-load all the extension settings in memory so that this function can
access some cache and return the correct data.  Kind of a bad solution since it
makes startup more complicated, plus there might be race condition issues.
2. assume that GetAllSyncData is run on the FILE thread (could check this now
but will write comment instead, it's mostly a moot point) and read everything
from leveldb synchronously.

The latter is more palatable, but it all depends on where the code needs to go.

+  // used for debugging so fine to ignore.
+  return SyncDataList();
+}
+
+SyncError ExtensionSettings::MergeDataAndStartSyncing(
+    syncable::ModelType type,
+    const SyncDataList& initial_sync_data,
+    SyncChangeProcessor* sync_processor) {
+  DCHECK(type == syncable::EXTENSION_SETTINGS);
+  DCHECK(sync_processor_ == NULL);
+  sync_processor_ = sync_processor;
+
+  // Unmerged sync data could conceivably be non-empty if this method is called
+  // in quick succession, before the storage creation methods return (clearing
+  // the data).  Clearing the unmerged data here is safe, it will just get
+  // repopulated with the new and correct data.
+  std::map<std::string, DictionaryValue*>::iterator unmerged_it;
+  for (unmerged_it = unmerged_sync_data_.begin();
+      unmerged_it != unmerged_sync_data_.end(); ++unmerged_it) {
+    delete unmerged_it->second;
+  }
+  unmerged_sync_data_.clear();
+
+  // Merge algorithm when starting to sync: for each extension, if there are no
+  // sycned settings yet, send local settings to sync.  Otherwise, completely
+  // overwrite local settings with those from sync.
+  //
+  // Firstly group the initial data per extension.
+  for (SyncDataList::const_iterator it = initial_sync_data.begin();
+      it != initial_sync_data.end(); ++it) {
+    ExtensionSettingSyncData data(*it);
+    if (data.value() == NULL) {
+      LOG(WARNING) << "NULL value in sync data";
+      continue;
+    }
+    DictionaryValue* sync_data = unmerged_sync_data_[data.extension_id()];
+    if (sync_data == NULL) {
+      sync_data = new DictionaryValue();
+      unmerged_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());
+  }
+
+  // Immediately start syncing the settings of existing storage areas.
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator storage_it;
+  for (storage_it = storage_objs_.begin();
+      storage_it != storage_objs_.end(); ++storage_it) {
+    StartSyncingStorage(storage_it->first, storage_it->second);
+  }
+
+  // Asynchronously start syncing the remaining settings.
+  for (unmerged_it = unmerged_sync_data_.begin();
+      unmerged_it != unmerged_sync_data_.end(); ++unmerged_it) {
+    // StartSyncingStorage will be called from EndCreationOfStorage, so the
+    // actual callback from getting the storage doesn't need to do anything.
+    GetStorage(
+        unmerged_it->first,
+        base::Bind(
+            &ExtensionSettings::AssertNoSyncData,
+            this,
+            unmerged_it->first));
+  }
+
+  return SyncError();
+}
+
+void ExtensionSettings::AssertNoSyncData(
+    const std::string& extension_id,
+    SyncableExtensionSettingsStorage* storage) {
+  DCHECK_EQ(0u, unmerged_sync_data_.count(extension_id));
+}
+
+void ExtensionSettings::StartSyncingStorage(
+    const std::string& extension_id,
+    SyncableExtensionSettingsStorage* storage) {
+  DCHECK(sync_processor_ != NULL);
+  scoped_ptr<DictionaryValue> sync_data(unmerged_sync_data_[extension_id]);
+  unmerged_sync_data_.erase(extension_id);
+  if (sync_data.get() == NULL) {
+    DictionaryValue no_data;
+    storage->StartSyncing(no_data, sync_processor_);
+  } else {
+    storage->StartSyncing(*sync_data, sync_processor_);
+  }
+}
+
+static void CallProcessSyncChanges(
+    ExtensionSettingSyncDataList* sync_changes,
+    SyncableExtensionSettingsStorage* storage) {
+  scoped_ptr<ExtensionSettingSyncDataList> changes_ownership(sync_changes);
+  storage->ProcessSyncChanges(*sync_changes);
+}
+
+SyncError ExtensionSettings::ProcessSyncChanges(
+    const tracked_objects::Location& from_here,
+    const SyncChangeList& sync_changes) {
+  DCHECK(sync_processor_ != NULL);
+
+  // Gather changes per extension, so that all the processing can be done from a
+  // single GetStorage() call.
+  std::map<std::string, ExtensionSettingSyncDataList> all_sync_data;
+  for (SyncChangeList::const_iterator it = sync_changes.begin();
+      it != sync_changes.end(); ++it) {
+    ExtensionSettingSyncData data(*it);
+    if (data.value() == NULL) {
+      LOG(WARNING) << "NULL value in sync data";
+      continue;
+    }
+    all_sync_data[data.extension_id()].push_back(data);
+  }
+
+  std::map<std::string, ExtensionSettingSyncDataList>::iterator it;
+  for (it = all_sync_data.begin(); it != all_sync_data.end(); ++it) {
+    GetStorage(
+        it->first,
+        base::Bind(
+            &CallProcessSyncChanges,
+            new ExtensionSettingSyncDataList(it->second)));

[akalin, 2011/08/31 07:49:34]

this may leak.  Just pass by value.

[not at google - send to devlin, 2011/09/02 05:00:40]

How can it leak?

But passing by value works, so, done.

+  }
+
+  return SyncError();
+}
+
+void ExtensionSettings::StopSyncing(syncable::ModelType type) {
+  DCHECK(type == syncable::EXTENSION_SETTINGS);
+  DCHECK(sync_processor_ != NULL);
+  sync_processor_ = NULL;
+
+  std::map<std::string, SyncableExtensionSettingsStorage*>::iterator it;
+  for (it = storage_objs_.begin(); it != storage_objs_.end(); ++it) {
+    it->second->StopSyncing();
+  }
+}