Enable sync for the settings from the Extension Settings API.

chrome/browser/extensions/syncable_extension_settings_storage.cc

Index: chrome/browser/extensions/syncable_extension_settings_storage.cc
diff --git a/chrome/browser/extensions/syncable_extension_settings_storage.cc b/chrome/browser/extensions/syncable_extension_settings_storage.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b8df311d8139c4b449bca404c4c3dcedfb44bdb4
--- /dev/null
+++ b/chrome/browser/extensions/syncable_extension_settings_storage.cc
@@ -0,0 +1,311 @@
+// 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 "chrome/browser/extensions/syncable_extension_settings_storage.h"
+
+#include "base/bind.h"

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

no need for bind

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

Done.

+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"

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

no need for message loop or task either

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

Done.

+#include "base/task.h"
+#include "chrome/browser/extensions/extension_settings_sync_util.h"
+#include "chrome/browser/sync/api/sync_data.h"
+#include "chrome/browser/sync/protocol/extension_setting_specifics.pb.h"
+#include "content/browser/browser_thread.h"
+
+SyncableExtensionSettingsStorage::SyncableExtensionSettingsStorage(
+    std::string extension_id, ExtensionSettingsStorage* delegate)
+    : extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) {}

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

add DCHECKs in all methods for FILE thread

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

Done.

+
+SyncableExtensionSettingsStorage::~SyncableExtensionSettingsStorage() {}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get(
+    const std::string& key) {
+  return delegate_->Get(key);
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get(
+    const std::vector<std::string>& keys) {
+  return delegate_->Get(keys);
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get() {
+  return delegate_->Get();
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Set(
+    const std::string& key, const Value& value) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  Result result = delegate_->Set(key, value);
+  if (result.HasError()) {
+    return result;
+  }
+  if (sync_processor_) {
+    SendAddsOrUpdatesToSync(*result.GetSettings());
+  }
+  return result;
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Set(
+    const DictionaryValue& values) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  Result result = delegate_->Set(values);
+  if (result.HasError()) {
+    return result;
+  }
+  if (sync_processor_) {
+    SendAddsOrUpdatesToSync(*result.GetSettings());
+  }
+  return result;
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Remove(
+    const std::string& key) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  Result result = delegate_->Remove(key);
+  if (result.HasError()) {
+    return result;
+  }
+  if (sync_processor_) {
+    std::vector<std::string> keys;
+    keys.push_back(key);
+    SendDeletesToSync(keys);
+  }
+  return result;
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Remove(
+    const std::vector<std::string>& keys) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  Result result = delegate_->Remove(keys);
+  if (result.HasError()) {
+    return result;
+  }
+  if (sync_processor_) {
+    SendDeletesToSync(keys);
+  }
+  return result;
+}
+
+ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Clear() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  Result result = delegate_->Clear();
+  if (result.HasError()) {
+    return result;
+  }
+  if (sync_processor_) {
+    std::vector<std::string> keys;

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

i think you can do:

std::vector<...> keys(synced_keys_.begin(), ....end());

directly

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

Done.

+    keys.insert(keys.end(), synced_keys_.begin(), synced_keys_.end());
+    SendDeletesToSync(keys);
+  }
+  return result;
+}
+
+// Sync-related methods.
+
+void SyncableExtensionSettingsStorage::StartSyncing(
+    const DictionaryValue& sync_state, SyncChangeProcessor* sync_processor) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!sync_processor_);
+  DCHECK(synced_keys_.empty());
+  sync_processor_ = sync_processor;
+  for (DictionaryValue::key_iterator it = sync_state.begin_keys();

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

i think you can do something like:

synced_keys_.insert(sync_state.begin_keys(), sync_state.end_keys());

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

It would be nice, and GCC/clang are fine with it, but something about the
implementation of DictionaryValue::key_iterator disagrees with visual c++ and it
doesn't compile.

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

I see.  Add comment and/or decomp into another function?

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

Done.

+      it != sync_state.end_keys(); ++it) {
+    synced_keys_.insert(*it);
+  }
+
+  Result maybe_settings = delegate_->Get();
+  if (maybe_settings.HasError()) {
+    LOG(WARNING) << "Failed to get local settings to send to sync: " <<
+        maybe_settings.GetError();
+    return;

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

I think we should propagate the error up and let the caller handle it

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

Didn't have time to do this before my flight, in fact, it's board now.  What can
the caller do though?  I presume... this can return the Result and the caller
log the error rather than the error being logged here.  But... it seems wrong to
return a Result from this, those Result objects are for storage operations.

And a bool wouldn't really be all that useful.

Anyway, I'll deal with it / think about it later.

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

Suggest wrapping any error message from maybe_settings here into a SyncError and
returning that.  Seems more useful than a bool, and you wouldn't expose the
Result object.  WDYT?

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

I looked at this some more.  I think it makes the most sense to return a
SyncError() since that's basically what it seems a SyncError is for.  However,
the ExtensionSettings which calls these StartSyncing methods can't really do
much about it, since it just means that this single extension failed to start;
others may have worked.

Which implies two problems: Firstly, the docs for SyncError say that they are
"unrecoverable" but what does this mean for partial recoverability?  Secondly,
if ExtensionSettings should propagate the error further up to its return value,
what should it be for the combination of all errors?

The same issue exists with the return value from ProcessSyncChanges, along with
the additional problem of there being multiple failable operations within the
method itself (as you already said).

Anyway, for now I've made this method return a SyncError (which will do the
logging itself, on construction of an error), but not done anything about it at
the ExtensionSettings level.

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

Yeah, I'll have to think about "recoverable" errors like these.
Yeah, I think that's fine for now.

+  }
+  DictionaryValue* settings = maybe_settings.GetSettings();
+
+  if (sync_state.empty()) {
+    SendLocalSettingsToSync(*settings);
+  } else {
+    OverwriteLocalSettingsWithSync(sync_state, *settings);
+  }
+}
+
+void SyncableExtensionSettingsStorage::SendLocalSettingsToSync(
+    const DictionaryValue& settings) {
+  SyncChangeList changes;
+  for (DictionaryValue::key_iterator it = settings.begin_keys();
+      it != settings.end_keys(); ++it) {
+    Value* value;
+    settings.GetWithoutPathExpansion(*it, &value);
+    changes.push_back(
+        extension_settings_sync_util::CreateAdd(extension_id_, *it, *value));
+  }
+
+  if (changes.empty()) {
+    return;
+  }
+
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  if (error.IsSet()) {
+    LOG(WARNING) << "Failed to send changes to sync: " << error.message();
+    return;
+  }
+
+  for (DictionaryValue::key_iterator it = settings.begin_keys();

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

use insert(begin, end) here, too

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

Ditto problem.

+      it != settings.end_keys(); ++it) {
+    synced_keys_.insert(*it);
+  }
+}
+
+void SyncableExtensionSettingsStorage::OverwriteLocalSettingsWithSync(
+    const DictionaryValue& sync_state, const DictionaryValue& settings) {
+  // Treat this as a list of changes to sync and use ProcessSyncChanges.
+  // This gives notifications etc for free.
+  scoped_ptr<DictionaryValue> new_sync_state(sync_state.DeepCopy());
+
+  ExtensionSettingSyncDataList changes;
+  for (DictionaryValue::key_iterator it = settings.begin_keys();
+      it != settings.end_keys(); ++it) {
+    Value* sync_value;

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

 = NULL

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

Done.

+    if (new_sync_state->RemoveWithoutPathExpansion(*it, &sync_value)) {
+      Value* local_value;

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

here too

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

Done.

+      settings.GetWithoutPathExpansion(*it, &local_value);
+      if (!local_value->Equals(sync_value)) {
+        // Sync value is different, update local setting with new value.
+        changes.push_back(
+            ExtensionSettingSyncData(
+                SyncChange::ACTION_UPDATE, "", *it, sync_value));

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

"" for extension ID?  Is this correct?  even if it doesn't matter, rather just
use extension_id_ for correctness

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

Done.

+      }
+    } else {
+      // Not synced, delete local setting.
+      changes.push_back(
+          ExtensionSettingSyncData(
+              SyncChange::ACTION_DELETE, "", *it, new DictionaryValue()));
+    }
+  }
+
+  // Add all new settings to local settings.
+  while (!new_sync_state->empty()) {
+    std::string key = *new_sync_state->begin_keys();
+    Value* value;
+    new_sync_state->RemoveWithoutPathExpansion(key, &value);
+    changes.push_back(
+        ExtensionSettingSyncData(SyncChange::ACTION_ADD, "", key, value));
+  }
+
+  if (!changes.empty()) {
+    ProcessSyncChanges(changes);
+  }
+}
+
+void SyncableExtensionSettingsStorage::StopSyncing() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+  sync_processor_ = NULL;
+  synced_keys_.clear();
+}
+
+void SyncableExtensionSettingsStorage::ProcessSyncChanges(
+    const ExtensionSettingSyncDataList& sync_changes) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+  DCHECK(!sync_changes.empty()) << "No sync changes for " << extension_id_;
+
+  for (ExtensionSettingSyncDataList::const_iterator it = sync_changes.begin();
+      it != sync_changes.end(); ++it) {
+    switch (it->change_type()) {
+      case SyncChange::ACTION_ADD:
+      case SyncChange::ACTION_UPDATE: {
+        synced_keys_.insert(it->key());
+        Result result = delegate_->Set(it->key(), *it->value());
+        if (result.HasError()) {

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

prefer propagating errors up, although i guess you'd have to return a vector of
errors or something

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

Ditto.

+          LOG(WARNING) << "Error pushing sync change to local settings: " <<
+              result.GetError();
+        }
+        break;
+      }
+
+      case SyncChange::ACTION_DELETE: {
+        synced_keys_.erase(it->key());
+        Result result = delegate_->Remove(it->key());
+        if (result.HasError()) {
+          LOG(WARNING) << "Error removing sync change from local settings: " <<
+                result.GetError();
+        }
+        break;
+      }
+
+      default:
+        NOTREACHED();
+    }
+  }
+}
+
+void SyncableExtensionSettingsStorage::SendAddsOrUpdatesToSync(
+    const DictionaryValue& settings) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+
+  SyncChangeList changes;
+  for (DictionaryValue::key_iterator it = settings.begin_keys();
+      it != settings.end_keys(); ++it) {
+    Value* value = NULL;
+    settings.GetWithoutPathExpansion(*it, &value);
+    DCHECK(value);
+    if (synced_keys_.count(*it)) {
+      // Key is synced, send ACTION_UPDATE to sync.
+      changes.push_back(
+          extension_settings_sync_util::CreateUpdate(
+              extension_id_, *it, *value));
+    } else {
+      // Key is not synced, send ACTION_ADD to sync.
+      changes.push_back(
+          extension_settings_sync_util::CreateAdd(extension_id_, *it, *value));
+    }
+  }
+
+  if (changes.empty()) {
+    return;
+  }
+
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  if (error.IsSet()) {
+    LOG(WARNING) << "Failed to send changes to sync: " << error.message();
+    return;
+  }
+
+  for (DictionaryValue::key_iterator it = settings.begin_keys();

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

use insert(begin, end)

probably should decomp this out into a utility method, though, since it's used
so much

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

Done.

+      it != settings.end_keys(); ++it) {
+    synced_keys_.insert(*it);
+  }
+}
+
+void SyncableExtensionSettingsStorage::SendDeletesToSync(
+    const std::vector<std::string>& keys) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(sync_processor_);
+
+  SyncChangeList changes;
+  for (std::vector<std::string>::const_iterator it = keys.begin();
+      it != keys.end(); ++it) {
+    // Only remove from sync if it's actually synced.
+    if (synced_keys_.count(*it)) {
+      changes.push_back(
+          extension_settings_sync_util::CreateDelete(extension_id_, *it));
+    }
+  }
+
+  if (changes.empty()) {
+    return;
+  }
+
+  SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  if (error.IsSet()) {
+    LOG(WARNING) << "Failed to send changes to sync: " << error.message();
+    return;
+  }
+
+  for (std::vector<std::string>::const_iterator it = keys.begin();
+      it != keys.end(); ++it) {
+    synced_keys_.erase(*it);

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

might be an erase(begin, end) method, too

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

Heh, it's funny because I tried all these things a while ago and ran into crazy
bugs.  The weird windows compiler one was just a puzzling compile error, but if
you want some fun trivia, read on:

std::set does have an erase(begin, end) method.  Unlike .insert though, it
assumes that the iterators are to *itself*.  If you try to do
std::set.erase(keys.start(), keys.end()) it puts the container in some invalid
state... I can't really remember the symptoms of the problem, but it manifested
itself later on and it took me a little while to figure out that this was the
cause.

+  }
+}