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

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

Index: chrome/browser/extensions/api/storage/setting_sync_data.cc
diff --git a/chrome/browser/extensions/api/storage/setting_sync_data.cc b/chrome/browser/extensions/api/storage/setting_sync_data.cc
index 212f5c7abb1bd78d97cac81c2410b4d69c88fab9..c577909ac2f3380e5846eb02a80896e3f01b0f4f 100644
--- a/chrome/browser/extensions/api/storage/setting_sync_data.cc
+++ b/chrome/browser/extensions/api/storage/setting_sync_data.cc
@@ -13,90 +13,52 @@
 
 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()) {
+  Init(sync_change.sync_data());

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

nit: The fact that this is called Init() but isn't called from all constructors
almost seems like a bug at first glance.  Can we rename it to something like
InitFromSyncData or something?  Or, if you wanted, you could use delegated
constructors between these two.

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

I looked into changing this to something better factored. Unfortunately
delegated constructors won't work because the difference is unwrapping either
|sync_change| or |sync_data| into its components - and there isn't away to
express that. Even if there was it would be a little verbose needing to declare
all 4 real constructor arguments, have some function "PullApart()" which assigns
to each of them, then "delegate" construction.

I can rename from "Init" because I agree it's an overloaded term.

 }
 
-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) {
+  Init(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::Init(const syncer::SyncData& sync_data) {
+  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());
+  const sync_pb::ExtensionSettingSpecifics* extension_specifics = nullptr;
+  if (specifics.has_extension_setting())

[Devlin, 2015/05/19 15:41:54]

nit: Ternary?  Up to you.

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

hah I do love ternary :)

+    extension_specifics = &specifics.extension_setting();
+  else
+    extension_specifics = &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