Remove a bunch of DeepCopy() calls in the chrome.storage API.

DeepCopy() is expensive and we're doing a lot of it, transferring ownership is
better. I also tidied up some code here and there, like C++11 niceties,
removing unnecessary linked_ptrs, and better commenting.

R=rdevlin.cronin@chromium.org

Review URL: https://codereview.chromium.org/1141963002

Cr-Commit-Position: refs/heads/master@{#330597}

chrome/browser/extensions/api/storage/setting_sync_data.cc

@@ -13,90 +13,51 @@
 
 namespace extensions {
 
-SettingSyncData::SettingSyncData(
-    const syncer::SyncChange& sync_change) {
-  Init(sync_change.change_type(), sync_change.sync_data());
+SettingSyncData::SettingSyncData(const syncer::SyncChange& sync_change)
+    : change_type_(sync_change.change_type()) {
+  ExtractSyncData(sync_change.sync_data());
 }
 
-SettingSyncData::SettingSyncData(
-    const syncer::SyncData& sync_data) {
-  Init(syncer::SyncChange::ACTION_INVALID, sync_data);
+SettingSyncData::SettingSyncData(const syncer::SyncData& sync_data)
+    : change_type_(syncer::SyncChange::ACTION_INVALID) {
+  ExtractSyncData(sync_data);
 }
 
-void SettingSyncData::Init(
-    syncer::SyncChange::SyncChangeType change_type,
-    const syncer::SyncData& sync_data) {
-  DCHECK(!internal_.get());
-  sync_pb::EntitySpecifics specifics = sync_data.GetSpecifics();
-  // The data must only be either extension or app specfics.
-  DCHECK_NE(specifics.has_extension_setting(),
-            specifics.has_app_setting());
-  if (specifics.has_extension_setting()) {
-    InitFromExtensionSettingSpecifics(
-        change_type,
-        specifics.extension_setting());
-  } else if (specifics.has_app_setting()) {
-    InitFromExtensionSettingSpecifics(
-        change_type,
-        specifics.app_setting().extension_setting());
-  }
-}
-
-void SettingSyncData::InitFromExtensionSettingSpecifics(
-    syncer::SyncChange::SyncChangeType change_type,
-    const sync_pb::ExtensionSettingSpecifics& specifics) {
-  DCHECK(!internal_.get());
-  scoped_ptr<base::Value> value(
-      base::JSONReader::Read(specifics.value()));
-  if (!value.get()) {
-    LOG(WARNING) << "Specifics for " << specifics.extension_id() << "/" <<
-        specifics.key() << " had bad JSON for value: " << specifics.value();
-    value.reset(new base::DictionaryValue());
-  }
-  internal_ = new Internal(
-      change_type,
-      specifics.extension_id(),
-      specifics.key(),
-      value.Pass());
+SettingSyncData::SettingSyncData(syncer::SyncChange::SyncChangeType change_type,
+                                 const std::string& extension_id,
+                                 const std::string& key,
+                                 scoped_ptr<base::Value> value)
+    : change_type_(change_type),
+      extension_id_(extension_id),
+      key_(key),
+      value_(value.Pass()) {
 }
 
-SettingSyncData::SettingSyncData(
-    syncer::SyncChange::SyncChangeType change_type,
-    const std::string& extension_id,
-    const std::string& key,
-    scoped_ptr<base::Value> value)
-    : internal_(new Internal(change_type, extension_id, key, value.Pass())) {}
-
 SettingSyncData::~SettingSyncData() {}
 
-syncer::SyncChange::SyncChangeType SettingSyncData::change_type() const {
-  return internal_->change_type_;
-}
-
-const std::string& SettingSyncData::extension_id() const {
-  return internal_->extension_id_;
+scoped_ptr<base::Value> SettingSyncData::PassValue() {
+  DCHECK(value_) << "value has already been Pass()ed";
+  return value_.Pass();
 }
 
-const std::string& SettingSyncData::key() const {
-  return internal_->key_;
-}
-
-const base::Value& SettingSyncData::value() const {
-  return *internal_->value_;
-}
-
-SettingSyncData::Internal::Internal(
-    syncer::SyncChange::SyncChangeType change_type,
-    const std::string& extension_id,
-    const std::string& key,
-    scoped_ptr<base::Value> value)
-    : change_type_(change_type),
-      extension_id_(extension_id),
-      key_(key),
-      value_(value.Pass()) {
-  DCHECK(value_.get());
+void SettingSyncData::ExtractSyncData(const syncer::SyncData& sync_data) {
+  sync_pb::EntitySpecifics specifics = sync_data.GetSpecifics();
+  // The specifics are exclusively either extension or app settings.
+  DCHECK_NE(specifics.has_extension_setting(), specifics.has_app_setting());
+  const sync_pb::ExtensionSettingSpecifics& extension_specifics =
+      specifics.has_extension_setting()
+          ? specifics.extension_setting()
+          : specifics.app_setting().extension_setting();
+
+  extension_id_ = extension_specifics.extension_id();
+  key_ = extension_specifics.key();
+  value_.reset(base::JSONReader::Read(extension_specifics.value()));
+
+  if (!value_) {
+    LOG(WARNING) << "Specifics for " << extension_id_ << "/" << key_
+                 << " had bad JSON for value: " << extension_specifics.value();
+    value_.reset(new base::DictionaryValue());
+  }
 }
 
-SettingSyncData::Internal::~Internal() {}
-
 }  // namespace extensions

chrome/browser/extensions/api/storage/setting_sync_data.h

@@ -5,8 +5,11 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_STORAGE_SETTING_SYNC_DATA_H_
 #define CHROME_BROWSER_EXTENSIONS_API_STORAGE_SETTING_SYNC_DATA_H_
 
-#include "base/memory/ref_counted.h"
+#include <string>
+
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "base/values.h"
 #include "sync/api/sync_change.h"
 
@@ -39,52 +42,34 @@ class SettingSyncData {
 
   ~SettingSyncData();
 
-  // Returns the type of the sync change; may be ACTION_INVALID.
-  syncer::SyncChange::SyncChangeType change_type() const;
-
-  // Returns the extension id the setting is for.
-  const std::string& extension_id() const;
-
-  // Returns the settings key.
-  const std::string& key() const;
+  // May return ACTION_INVALID if this object represents sync data that isn't
+  // associated with a sync operation.
+  syncer::SyncChange::SyncChangeType change_type() const {
+    return change_type_;
+  }
+  const std::string& extension_id() const { return extension_id_; }
+  const std::string& key() const { return key_; }
+  // value() cannot be called if PassValue() has been called.
+  const base::Value& value() const { return *value_; }
 
-  // Returns the value of the setting.
-  const base::Value& value() const;
+  // Releases ownership of the value to the caller. Neither value() nor
+  // PassValue() can be after this.
+  scoped_ptr<base::Value> PassValue();
 
  private:
-  // Ref-counted container for the data.
-  // TODO(kalman): Use browser_sync::Immutable<Internal>.
-  class Internal : public base::RefCountedThreadSafe<Internal> {
-   public:
-    Internal(
-      syncer::SyncChange::SyncChangeType change_type,
-      const std::string& extension_id,
-      const std::string& key,
-      scoped_ptr<base::Value> value);
-
-    syncer::SyncChange::SyncChangeType change_type_;
-    std::string extension_id_;
-    std::string key_;
-    scoped_ptr<base::Value> value_;
-
-   private:
-    friend class base::RefCountedThreadSafe<Internal>;
-    ~Internal();
-  };
+  // Populates the extension ID, key, and value from |sync_data|. This will be
+  // either an extension or app settings data type.
+  void ExtractSyncData(const syncer::SyncData& sync_data);
 
-  // Initializes internal_ from sync data for an extension or app setting.
-  void Init(syncer::SyncChange::SyncChangeType change_type,
-            const syncer::SyncData& sync_data);
-
-  // Initializes internal_ from extension specifics.
-  void InitFromExtensionSettingSpecifics(
-      syncer::SyncChange::SyncChangeType change_type,
-      const sync_pb::ExtensionSettingSpecifics& specifics);
+  syncer::SyncChange::SyncChangeType change_type_;
+  std::string extension_id_;
+  std::string key_;
+  scoped_ptr<base::Value> value_;
 
-  scoped_refptr<Internal> internal_;
+  DISALLOW_COPY_AND_ASSIGN(SettingSyncData);
 };
 
-typedef std::vector<SettingSyncData> SettingSyncDataList;
+typedef ScopedVector<SettingSyncData> SettingSyncDataList;
 
 }  // namespace extensions
 

chrome/browser/extensions/api/storage/sync_storage_backend.cc

@@ -28,6 +28,10 @@ void AddAllSyncData(const std::string& extension_id,
   }
 }
 
+scoped_ptr<base::DictionaryValue> EmptyDictionaryValue() {
+  return make_scoped_ptr(new base::DictionaryValue());
+}
+
 }  // namespace
 
 SyncStorageBackend::SyncStorageBackend(
@@ -52,13 +56,12 @@ SyncStorageBackend::~SyncStorageBackend() {}
 
 ValueStore* SyncStorageBackend::GetStorage(const std::string& extension_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  base::DictionaryValue empty;
-  return GetOrCreateStorageWithSyncData(extension_id, empty);
+  return GetOrCreateStorageWithSyncData(extension_id, EmptyDictionaryValue());
 }
 
 SyncableSettingsStorage* SyncStorageBackend::GetOrCreateStorageWithSyncData(
     const std::string& extension_id,
-    const base::DictionaryValue& sync_data) const {
+    scoped_ptr<base::DictionaryValue> sync_data) const {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   StorageObjMap::iterator maybe_storage = storage_objs_.find(extension_id);
@@ -79,9 +82,9 @@ SyncableSettingsStorage* SyncStorageBackend::GetOrCreateStorageWithSyncData(
 
   if (sync_processor_.get()) {
     syncer::SyncError error = syncable_storage->StartSyncing(
-        sync_data, CreateSettingsSyncProcessor(extension_id).Pass());
+        sync_data.Pass(), CreateSettingsSyncProcessor(extension_id).Pass());
     if (error.IsSet())
-      syncable_storage.get()->StopSyncing();
+      syncable_storage->StopSyncing();
   }
   return syncable_storage.get();
 }
@@ -110,10 +113,8 @@ std::set<std::string> SyncStorageBackend::GetKnownExtensionIDs() const {
 
   // Storage areas can be in-memory as well as on disk. |storage_objs_| will
   // contain all that are in-memory.
-  for (StorageObjMap::iterator it = storage_objs_.begin();
-       it != storage_objs_.end();
-       ++it) {
-    result.insert(it->first);
+  for (const auto& storage_obj : storage_objs_) {
+    result.insert(storage_obj.first);
   }
 
   // Leveldb databases are directories inside |base_path_|.
@@ -148,7 +149,7 @@ syncer::SyncDataList SyncStorageBackend::GetAllSyncData(syncer::ModelType type)
        it != known_extension_ids.end();
        ++it) {
     ValueStore::ReadResult maybe_settings =
-        GetOrCreateStorageWithSyncData(*it, base::DictionaryValue())->Get();
+        GetOrCreateStorageWithSyncData(*it, EmptyDictionaryValue())->Get();
     if (maybe_settings->HasError()) {
       LOG(WARNING) << "Failed to get settings for " << *it << ": "
                    << maybe_settings->error().message;
@@ -175,54 +176,50 @@ syncer::SyncMergeResult SyncStorageBackend::MergeDataAndStartSyncing(
   sync_error_factory_ = sync_error_factory.Pass();
 
   // Group the initial sync data by extension id.
-  std::map<std::string, linked_ptr<base::DictionaryValue> > grouped_sync_data;
-  for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
-       it != initial_sync_data.end();
-       ++it) {
-    SettingSyncData data(*it);
-    linked_ptr<base::DictionaryValue> sync_data =
-        grouped_sync_data[data.extension_id()];
-    if (!sync_data.get()) {
-      sync_data =
-          linked_ptr<base::DictionaryValue>(new base::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->SetWithoutPathExpansion(data.key(), data.value().DeepCopy());
+  // The raw pointers are safe because ownership of each item is passed to
+  // storage->StartSyncing or GetOrCreateStorageWithSyncData.
+  std::map<std::string, base::DictionaryValue*> grouped_sync_data;
+
+  for (const syncer::SyncData& sync_data : initial_sync_data) {
+    SettingSyncData data(sync_data);
+    // Yes this really is a reference to a pointer.
+    base::DictionaryValue*& settings = grouped_sync_data[data.extension_id()];
+    if (!settings)
+      settings = new base::DictionaryValue();
+    DCHECK(!settings->HasKey(data.key())) << "Duplicate settings for "
+                                          << data.extension_id() << "/"
+                                          << data.key();
+    settings->SetWithoutPathExpansion(data.key(), data.PassValue());
   }
 
   // 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) {
-    std::map<std::string, linked_ptr<base::DictionaryValue> >::iterator
-        maybe_sync_data = grouped_sync_data.find(it->first);
+  for (const auto& storage_obj : storage_objs_) {
+    const std::string& extension_id = storage_obj.first;
+    SyncableSettingsStorage* storage = storage_obj.second.get();
+
+    auto group = grouped_sync_data.find(extension_id);
     syncer::SyncError error;
-    if (maybe_sync_data != grouped_sync_data.end()) {
-      error = it->second->StartSyncing(
-          *maybe_sync_data->second,
-          CreateSettingsSyncProcessor(it->first).Pass());
-      grouped_sync_data.erase(it->first);
+    if (group != grouped_sync_data.end()) {
+      error = storage->StartSyncing(
+          make_scoped_ptr(group->second),
+          CreateSettingsSyncProcessor(extension_id).Pass());
+      grouped_sync_data.erase(group);
     } else {
-      base::DictionaryValue empty;
-      error = it->second->StartSyncing(
-          empty, CreateSettingsSyncProcessor(it->first).Pass());
+      error = storage->StartSyncing(
+          EmptyDictionaryValue(),
+          CreateSettingsSyncProcessor(extension_id).Pass());
     }
+
     if (error.IsSet())
-      it->second->StopSyncing();
+      storage->StopSyncing();
   }
 
   // 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<base::DictionaryValue> >::iterator it =
-           grouped_sync_data.begin();
-       it != grouped_sync_data.end();
-       ++it) {
-    GetOrCreateStorageWithSyncData(it->first, *it->second);
+  for (const auto& group : grouped_sync_data) {
+    GetOrCreateStorageWithSyncData(group.first, make_scoped_ptr(group.second));
   }
 
   return syncer::SyncMergeResult(type);
@@ -235,23 +232,24 @@ syncer::SyncError SyncStorageBackend::ProcessSyncChanges(
   DCHECK(sync_processor_.get());
 
   // Group changes by extension, to pass all changes in a single method call.
-  std::map<std::string, SettingSyncDataList> grouped_sync_data;
-  for (syncer::SyncChangeList::const_iterator it = sync_changes.begin();
-       it != sync_changes.end();
-       ++it) {
-    SettingSyncData data(*it);
-    grouped_sync_data[data.extension_id()].push_back(data);
+  // The raw pointers are safe because ownership of each item is passed to
+  // storage->ProcessSyncChanges.
+  std::map<std::string, SettingSyncDataList*> grouped_sync_data;
+
+  for (const syncer::SyncChange& change : sync_changes) {
+    scoped_ptr<SettingSyncData> data(new SettingSyncData(change));
+    SettingSyncDataList*& group = grouped_sync_data[data->extension_id()];
+    if (!group)
+      group = new SettingSyncDataList();
+    group->push_back(data.Pass());
   }
 
   // Create any storage areas that don't exist yet but have sync data.
-  base::DictionaryValue empty;
-  for (std::map<std::string, SettingSyncDataList>::iterator it =
-           grouped_sync_data.begin();
-       it != grouped_sync_data.end();
-       ++it) {
+  for (const auto& group : grouped_sync_data) {
     SyncableSettingsStorage* storage =
-        GetOrCreateStorageWithSyncData(it->first, empty);
-    syncer::SyncError error = storage->ProcessSyncChanges(it->second);
+        GetOrCreateStorageWithSyncData(group.first, EmptyDictionaryValue());
+    syncer::SyncError error =
+        storage->ProcessSyncChanges(make_scoped_ptr(group.second));
     if (error.IsSet())
       storage->StopSyncing();
   }
@@ -264,12 +262,10 @@ void SyncStorageBackend::StopSyncing(syncer::ModelType type) {
   DCHECK(type == syncer::EXTENSION_SETTINGS || type == syncer::APP_SETTINGS);
   DCHECK_EQ(sync_type_, type);
 
-  for (StorageObjMap::iterator it = storage_objs_.begin();
-       it != storage_objs_.end();
-       ++it) {
+  for (const auto& storage_obj : storage_objs_) {
     // Some storage areas may have already stopped syncing if they had areas
     // and syncing was disabled, but StopSyncing is safe to call multiple times.
-    it->second->StopSyncing();
+    storage_obj.second->StopSyncing();
   }
 
   sync_processor_.reset();

chrome/browser/extensions/api/storage/sync_storage_backend.h

@@ -67,7 +67,7 @@ class SyncStorageBackend : public syncer::SyncableService {
   // initializing sync with some initial data if sync enabled.
   SyncableSettingsStorage* GetOrCreateStorageWithSyncData(
       const std::string& extension_id,
-      const base::DictionaryValue& sync_data) const;
+      scoped_ptr<base::DictionaryValue> sync_data) const;
 
   // Gets all extension IDs known to extension settings.  This may not be all
   // installed extensions.

chrome/browser/extensions/api/storage/syncable_settings_storage.cc

@@ -12,10 +12,10 @@
 #include "sync/api/sync_data.h"
 #include "sync/protocol/extension_setting_specifics.pb.h"
 
-namespace extensions {
-
 using content::BrowserThread;
 
+namespace extensions {
+
 SyncableSettingsStorage::SyncableSettingsStorage(
     const scoped_refptr<ObserverListThreadSafe<SettingsObserver> >&
         observers,
@@ -164,102 +164,97 @@ void SyncableSettingsStorage::SyncResultIfEnabled(
 // Sync-related methods.
 
 syncer::SyncError SyncableSettingsStorage::StartSyncing(
-    const base::DictionaryValue& sync_state,
+    scoped_ptr<base::DictionaryValue> sync_state,
     scoped_ptr<SettingsSyncProcessor> sync_processor) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(sync_state);
   DCHECK(!sync_processor_.get());
 
   sync_processor_ = sync_processor.Pass();
-  sync_processor_->Init(sync_state);
+  sync_processor_->Init(*sync_state);
 
   ReadResult maybe_settings = delegate_->Get();
   if (maybe_settings->HasError()) {
     return syncer::SyncError(
-        FROM_HERE,
-        syncer::SyncError::DATATYPE_ERROR,
+        FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
         base::StringPrintf("Failed to get settings: %s",
-            maybe_settings->error().message.c_str()),
+                           maybe_settings->error().message.c_str()),
         sync_processor_->type());
   }
 
-  const base::DictionaryValue& settings = maybe_settings->settings();
-  return sync_state.empty() ?
-      SendLocalSettingsToSync(settings) :
-      OverwriteLocalSettingsWithSync(sync_state, settings);
+  scoped_ptr<base::DictionaryValue> current_settings =
+      maybe_settings->PassSettings();
+  return sync_state->empty() ? SendLocalSettingsToSync(current_settings.Pass())
+                             : OverwriteLocalSettingsWithSync(
+                                   sync_state.Pass(), current_settings.Pass());
 }
 
 syncer::SyncError SyncableSettingsStorage::SendLocalSettingsToSync(
-    const base::DictionaryValue& settings) {
+    scoped_ptr<base::DictionaryValue> local_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
+  if (local_state->empty())
+    return syncer::SyncError();
+
+  // Transform the current settings into a list of sync changes.
   ValueStoreChangeList changes;
-  for (base::DictionaryValue::Iterator i(settings); !i.IsAtEnd(); i.Advance()) {
-    changes.push_back(ValueStoreChange(i.key(), NULL, i.value().DeepCopy()));
+  while (!local_state->empty()) {
+    // It's not possible to iterate over a DictionaryValue and modify it at the
+    // same time, so hack around that restriction.
+    std::string key = base::DictionaryValue::Iterator(*local_state).key();
+    scoped_ptr<base::Value> value;
+    local_state->RemoveWithoutPathExpansion(key, &value);
+    changes.push_back(ValueStoreChange(key, nullptr, value.release()));
   }
 
-  if (changes.empty())
-    return syncer::SyncError();
-
   syncer::SyncError error = sync_processor_->SendChanges(changes);
   if (error.IsSet())
     StopSyncing();
-
   return error;
 }
 
 syncer::SyncError SyncableSettingsStorage::OverwriteLocalSettingsWithSync(
-    const base::DictionaryValue& sync_state,
-    const base::DictionaryValue& settings) {
+    scoped_ptr<base::DictionaryValue> sync_state,
+    scoped_ptr<base::DictionaryValue> local_state) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  // Treat this as a list of changes to sync and use ProcessSyncChanges.
-  // This gives notifications etc for free.
-  scoped_ptr<base::DictionaryValue> new_sync_state(sync_state.DeepCopy());
+  // This is implemented by building up a list of sync changes then sending
+  // those to ProcessSyncChanges. This generates events like onStorageChanged.
+  scoped_ptr<SettingSyncDataList> changes(new SettingSyncDataList());
 
-  SettingSyncDataList changes;
-  for (base::DictionaryValue::Iterator it(settings);
-       !it.IsAtEnd(); it.Advance()) {
+  for (base::DictionaryValue::Iterator it(*local_state); !it.IsAtEnd();
+       it.Advance()) {
     scoped_ptr<base::Value> sync_value;
-    if (new_sync_state->RemoveWithoutPathExpansion(it.key(), &sync_value)) {
+    if (sync_state->RemoveWithoutPathExpansion(it.key(), &sync_value)) {
       if (sync_value->Equals(&it.value())) {
         // Sync and local values are the same, no changes to send.
       } else {
         // Sync value is different, update local setting with new value.
-        changes.push_back(
-            SettingSyncData(
-                syncer::SyncChange::ACTION_UPDATE,
-                extension_id_,
-                it.key(),
-                sync_value.Pass()));
+        changes->push_back(
+            new SettingSyncData(syncer::SyncChange::ACTION_UPDATE,
+                                extension_id_, it.key(), sync_value.Pass()));
       }
     } else {
       // Not synced, delete local setting.
-      changes.push_back(
-          SettingSyncData(
-              syncer::SyncChange::ACTION_DELETE,
-              extension_id_,
-              it.key(),
-              scoped_ptr<base::Value>(new base::DictionaryValue())));
+      changes->push_back(new SettingSyncData(
+          syncer::SyncChange::ACTION_DELETE, extension_id_, it.key(),
+          scoped_ptr<base::Value>(new base::DictionaryValue())));
     }
   }
 
   // Add all new settings to local settings.
-  while (!new_sync_state->empty()) {
-    base::DictionaryValue::Iterator first_entry(*new_sync_state);
-    std::string key = first_entry.key();
+  while (!sync_state->empty()) {
+    // It's not possible to iterate over a DictionaryValue and modify it at the
+    // same time, so hack around that restriction.
+    std::string key = base::DictionaryValue::Iterator(*sync_state).key();
     scoped_ptr<base::Value> value;
-    CHECK(new_sync_state->RemoveWithoutPathExpansion(key, &value));
-    changes.push_back(
-        SettingSyncData(
-            syncer::SyncChange::ACTION_ADD,
-            extension_id_,
-            key,
-            value.Pass()));
+    CHECK(sync_state->RemoveWithoutPathExpansion(key, &value));
+    changes->push_back(new SettingSyncData(syncer::SyncChange::ACTION_ADD,
+                                           extension_id_, key, value.Pass()));
   }
 
-  if (changes.empty())
+  if (changes->empty())
     return syncer::SyncError();
-
-  return ProcessSyncChanges(changes);
+  return ProcessSyncChanges(changes.Pass());
 }
 
 void SyncableSettingsStorage::StopSyncing() {
@@ -268,9 +263,9 @@ void SyncableSettingsStorage::StopSyncing() {
 }
 
 syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges(
-    const SettingSyncDataList& sync_changes) {
+    scoped_ptr<SettingSyncDataList> sync_changes) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  DCHECK(!sync_changes.empty()) << "No sync changes for " << extension_id_;
+  DCHECK(!sync_changes->empty()) << "No sync changes for " << extension_id_;
 
   if (!sync_processor_.get()) {
     return syncer::SyncError(
@@ -283,16 +278,15 @@ syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges(
   std::vector<syncer::SyncError> errors;
   ValueStoreChangeList changes;
 
-  for (SettingSyncDataList::const_iterator it = sync_changes.begin();
-      it != sync_changes.end(); ++it) {
-    DCHECK_EQ(extension_id_, it->extension_id());
-
-    const std::string& key = it->key();
-    const base::Value& value = it->value();
+  for (SettingSyncDataList::iterator it = sync_changes->begin();
+       it != sync_changes->end(); ++it) {
+    DCHECK_EQ(extension_id_, (*it)->extension_id());
+    const std::string& key = (*it)->key();
+    scoped_ptr<base::Value> change_value = (*it)->PassValue();
 
     scoped_ptr<base::Value> current_value;
     {
-      ReadResult maybe_settings = Get(it->key());
+      ReadResult maybe_settings = Get(key);
       if (maybe_settings->HasError()) {
         errors.push_back(syncer::SyncError(
             FROM_HERE,
@@ -309,29 +303,29 @@ syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges(
 
     syncer::SyncError error;
 
-    switch (it->change_type()) {
+    switch ((*it)->change_type()) {
       case syncer::SyncChange::ACTION_ADD:
         if (!current_value.get()) {
-          error = OnSyncAdd(key, value.DeepCopy(), &changes);
+          error = OnSyncAdd(key, change_value.release(), &changes);
         } else {
           // Already a value; hopefully a local change has beaten sync in a
-          // race and it's not a bug, so pretend it's an update.
+          // race and change's not a bug, so pretend change's an update.
           LOG(WARNING) << "Got add from sync for existing setting " <<
               extension_id_ << "/" << key;
-          error = OnSyncUpdate(
-              key, current_value.release(), value.DeepCopy(), &changes);
+          error = OnSyncUpdate(key, current_value.release(),
+                               change_value.release(), &changes);
         }
         break;
 
       case syncer::SyncChange::ACTION_UPDATE:
         if (current_value.get()) {
-          error = OnSyncUpdate(
-              key, current_value.release(), value.DeepCopy(), &changes);
+          error = OnSyncUpdate(key, current_value.release(),
+                               change_value.release(), &changes);
         } else {
-          // Similarly, pretend it's an add.
+          // Similarly, pretend change's an add.
           LOG(WARNING) << "Got update from sync for nonexistent setting" <<
               extension_id_ << "/" << key;
-          error = OnSyncAdd(key, value.DeepCopy(), &changes);
+          error = OnSyncAdd(key, change_value.release(), &changes);
         }
         break;
 
@@ -339,7 +333,7 @@ syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges(
         if (current_value.get()) {
           error = OnSyncDelete(key, current_value.release(), &changes);
         } else {
-          // Similarly, ignore it.
+          // Similarly, ignore change.
           LOG(WARNING) << "Got delete from sync for nonexistent setting " <<
               extension_id_ << "/" << key;
         }

chrome/browser/extensions/api/storage/syncable_settings_storage.h

@@ -55,31 +55,41 @@ class SyncableSettingsStorage : public ValueStore {
   // ExtensionSettings), but with looser guarantees about when the methods
   // can be called.
 
-  // Must only be called if sync isn't already active.
+  // Starts syncing this storage area. Must only be called if sync isn't
+  // already active.
+  // |sync_state| is the current state of the extension settings in sync.
+  // |sync_processor| is used to write out any changes.
+  // Returns any error when trying to sync, or an empty error on success.
   syncer::SyncError StartSyncing(
-      const base::DictionaryValue& sync_state,
+      scoped_ptr<base::DictionaryValue> sync_state,
       scoped_ptr<SettingsSyncProcessor> sync_processor);
 
-  // May be called at any time (idempotent).
+  // Stops syncing this storage area. May be called at any time (idempotent).
   void StopSyncing();
 
-  // May be called at any time; changes will be ignored if sync isn't active.
-  syncer::SyncError ProcessSyncChanges(const SettingSyncDataList& sync_changes);
+  // Pushes a list of sync changes into this storage area. May be called at any
+  // time, changes will be ignored if sync isn't active.
+  // Returns any error when trying to sync, or an empty error on success.
+  syncer::SyncError ProcessSyncChanges(
+      scoped_ptr<SettingSyncDataList> sync_changes);
 
  private:
   // Sends the changes from |result| to sync if it's enabled.
   void SyncResultIfEnabled(const ValueStore::WriteResult& result);
 
-  // Sends all local settings to sync (synced settings assumed to be empty).
+  // Sends all local settings to sync. This assumes that there are no settings
+  // in sync yet.
+  // Returns any error when trying to sync, or an empty error on success.
   syncer::SyncError SendLocalSettingsToSync(
-      const base::DictionaryValue& settings);
+      scoped_ptr<base::DictionaryValue> local_state);
 
   // Overwrites local state with sync state.
+  // Returns any error when trying to sync, or an empty error on success.
   syncer::SyncError OverwriteLocalSettingsWithSync(
-      const base::DictionaryValue& sync_state,
-      const base::DictionaryValue& settings);
+      scoped_ptr<base::DictionaryValue> sync_state,
+      scoped_ptr<base::DictionaryValue> local_state);
 
-  // Called when an Add/Update/Remove comes from sync.  Ownership of Value*s
+  // Called when an Add/Update/Remove comes from sync. Ownership of Value*s
   // are taken.
   syncer::SyncError OnSyncAdd(
       const std::string& key,