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

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

 // Copyright 2014 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/api/storage/sync_storage_backend.h"
 
 #include "base/files/file_enumerator.h"
 #include "base/logging.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_processor.h"
 #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
@@ skipping 10 matching lines @@
 void AddAllSyncData(const std::string& extension_id,
                     const base::DictionaryValue& src,
                     syncer::ModelType type,
                     syncer::SyncDataList* dst) {
   for (base::DictionaryValue::Iterator it(src); !it.IsAtEnd(); it.Advance()) {
     dst->push_back(settings_sync_util::CreateData(
         extension_id, it.key(), it.value(), type));
   }
 }
 
+scoped_ptr<base::DictionaryValue> EmptyDictionaryValue() {
+  return make_scoped_ptr(new base::DictionaryValue());
+}
+
 }  // namespace
 
 SyncStorageBackend::SyncStorageBackend(
     const scoped_refptr<SettingsStorageFactory>& storage_factory,
     const base::FilePath& base_path,
     const SettingsStorageQuotaEnforcer::Limits& quota,
     const scoped_refptr<SettingsObserverList>& observers,
     syncer::ModelType sync_type,
     const syncer::SyncableService::StartSyncFlare& flare)
     : storage_factory_(storage_factory),
       base_path_(base_path),
       quota_(quota),
       observers_(observers),
       sync_type_(sync_type),
       flare_(flare) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DCHECK(sync_type_ == syncer::EXTENSION_SETTINGS ||
          sync_type_ == syncer::APP_SETTINGS);
 }
 
 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);
   if (maybe_storage != storage_objs_.end()) {
     return maybe_storage->second.get();
   }
 
   scoped_ptr<SettingsStorageQuotaEnforcer> storage(
       new SettingsStorageQuotaEnforcer(
           quota_, storage_factory_->Create(base_path_, extension_id)));
 
   // It's fine to create the quota enforcer underneath the sync layer, since
   // sync will only go ahead if each underlying storage operation succeeds.
   linked_ptr<SyncableSettingsStorage> syncable_storage(
       new SyncableSettingsStorage(
           observers_, extension_id, storage.release(), sync_type_, flare_));
   storage_objs_[extension_id] = syncable_storage;
 
   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();
 }
 
 void SyncStorageBackend::DeleteStorage(const std::string& extension_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 
   // Clear settings when the extension is uninstalled.  Leveldb implementations
   // will also delete the database from disk when the object is destroyed as a
   // result of being removed from |storage_objs_|.
   //
   // TODO(kalman): always GetStorage here (rather than only clearing if it
   // exists) since the storage area may have been unloaded, but we still want
   // to clear the data from disk.
   // However, this triggers http://crbug.com/111072.
   StorageObjMap::iterator maybe_storage = storage_objs_.find(extension_id);
   if (maybe_storage == storage_objs_.end())
     return;
   maybe_storage->second->Clear();
   storage_objs_.erase(extension_id);
 }
 
 std::set<std::string> SyncStorageBackend::GetKnownExtensionIDs() const {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   std::set<std::string> result;
 
   // 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_|.
   base::FileEnumerator extension_dirs(
       base_path_, false, base::FileEnumerator::DIRECTORIES);
   while (!extension_dirs.Next().empty()) {
     base::FilePath extension_dir = extension_dirs.GetInfo().GetName();
     DCHECK(!extension_dir.IsAbsolute());
     // Extension IDs are created as std::strings so they *should* be ASCII.
     std::string maybe_as_ascii(extension_dir.MaybeAsASCII());
@@ skipping 14 matching lines @@
 
   // For all extensions, get all their settings.  This has the effect
   // of bringing in the entire state of extension settings in memory; sad.
   syncer::SyncDataList all_sync_data;
   std::set<std::string> known_extension_ids(GetKnownExtensionIDs());
 
   for (std::set<std::string>::const_iterator it = known_extension_ids.begin();
        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;
       continue;
     }
     AddAllSyncData(*it, maybe_settings->settings(), type, &all_sync_data);
   }
 
   return all_sync_data;
 }
 
 syncer::SyncMergeResult SyncStorageBackend::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
     scoped_ptr<syncer::SyncErrorFactory> sync_error_factory) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   DCHECK_EQ(sync_type_, type);
   DCHECK(!sync_processor_.get());
   DCHECK(sync_processor.get());
   DCHECK(sync_error_factory.get());
 
   sync_processor_ = sync_processor.Pass();
   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());
+  std::map<std::string, base::DictionaryValue*> grouped_sync_data;

[Devlin, 2015/05/15 21:59:07]

This is scary - easy leaking if we ever early-return, which, in a function of
this size, is not entirely unlikely.  At the same time, linked_ptrs are evil, so
I like getting rid of them.

I ran an idea past Jeffrey, and he says it's not totally crazy.  So, what if we
have something like a scoped assert, which yells if the function early-returns
(and the assert isn't reset)?

It would be something like:
std::map<string, dictionary> grouped_sync_data;
ScopedAssert grouped_sync_data_guard;  // Make sure we don't leak anything.
<...all code....>
for (const auto& group : grouped_sync_data)
  GetOrCreateStorageWithSyncData(group.first, make_scoped_ptr(group.second));
grouped_sync_data_guard.Reset();  // All data safely passed.

That would ensure that we never leak anything (or, if we do, we CHECK).

For a ScopedAssert, you could make your own object (which would be really
trivial - about 10 - 20 lines, including boilerplate) or use the
base::ScopedClosureRunner.

WDYT?

[not at google - send to devlin, 2015/05/15 23:28:30]

I don't really see the point. The reason to have RAII is to get compile time
safety, not to get some runtime error. Changing this back to linked pointer
seems far easier, it's just a PITA to work with, especially the conversions
between linked and scoped which this CL would introduce.

[Devlin, 2015/05/15 23:58:18]

Meh, fine then. :P  At the very least, can you document why it's safe?

[not at google - send to devlin, 2015/05/19 19:43:57]

Documented.

+  for (const syncer::SyncData& sync_data : initial_sync_data) {
+    SettingSyncData data(sync_data);
+    base::DictionaryValue*& settings = grouped_sync_data[data.extension_id()];

[Devlin, 2015/05/15 21:59:07]

This is kinda cool, but honestly, it might be worth a comment to say "No, this
isn't as crazy as it looks."

[not at google - send to devlin, 2015/05/15 23:28:30]

haha ok.

+    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();
+
     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 (grouped_sync_data.count(extension_id)) {
+      error = storage->StartSyncing(
+          make_scoped_ptr(grouped_sync_data[extension_id]),

[Devlin, 2015/05/15 21:59:07]

this does result in a trip lookup (one in the if on line 198, one here, and one
on line 208 (that was always there)), which is probably why we had the find
before.  I think it's probably best to keep it like that, and just clean it up a
touch.

auto iter = grouped_sync_data.find(extension_id);
if (iter != grouped_sync_data.end()) {
  error = ...
  grouped_sync_data.erase(iter);
}

[not at google - send to devlin, 2015/05/15 23:28:30]

Done.

+          CreateSettingsSyncProcessor(extension_id).Pass());
+      grouped_sync_data.erase(extension_id);
     } 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);
 }
 
 syncer::SyncError SyncStorageBackend::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& sync_changes) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   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);
+  std::map<std::string, SettingSyncDataList*> grouped_sync_data;

[Devlin, 2015/05/15 21:59:07]

ditto here, though this one's a little less scary since it's a much smaller
function.

[not at google - send to devlin, 2015/05/19 19:43:57]

Documented.

+  for (const syncer::SyncChange& change : sync_changes) {
+    SettingSyncData data(change);
+    SettingSyncDataList*& group = grouped_sync_data[data.extension_id()];
+    if (!group)
+      group = new SettingSyncDataList();
+    group->push_back(data);
   }
 
   // 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();
   }
 
   return syncer::SyncError();
 }
 
 void SyncStorageBackend::StopSyncing(syncer::ModelType type) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   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();
   sync_error_factory_.reset();
 }
 
 scoped_ptr<SettingsSyncProcessor>
 SyncStorageBackend::CreateSettingsSyncProcessor(const std::string& extension_id)
     const {
   CHECK(sync_processor_.get());
   return scoped_ptr<SettingsSyncProcessor>(new SettingsSyncProcessor(
       extension_id, sync_type_, sync_processor_.get()));
 }
 
 }  // namespace extensions