Convert some parts of sync/ to use of scoped_ptr instead of bare ptrs

for base::Value handling.

BUG=none

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

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

8 files changed, 58 insertions, 37 deletions

diff --git a/sync/internal_api/change_record.cc b/sync/internal_api/change_record.cc
index 4894b3c..d6c7d36 100644
--- a/sync/internal_api/change_record.cc
+++ b/sync/internal_api/change_record.cc
@@ -17,8 +17,8 @@ ChangeRecord::ChangeRecord()
 
 ChangeRecord::~ChangeRecord() {}
 
-base::DictionaryValue* ChangeRecord::ToValue() const {
-  base::DictionaryValue* value = new base::DictionaryValue();
+scoped_ptr<base::DictionaryValue> ChangeRecord::ToValue() const {
+  scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue());
   std::string action_str;
   switch (action) {
     case ACTION_ADD:
@@ -55,7 +55,8 @@ ExtraPasswordChangeRecordData::ExtraPasswordChangeRecordData(
 
 ExtraPasswordChangeRecordData::~ExtraPasswordChangeRecordData() {}
 
-base::DictionaryValue* ExtraPasswordChangeRecordData::ToValue() const {
+scoped_ptr<base::DictionaryValue> ExtraPasswordChangeRecordData::ToValue()
+    const {
   return PasswordSpecificsDataToValue(unencrypted_);
 }
 
diff --git a/sync/internal_api/public/base/model_type.h b/sync/internal_api/public/base/model_type.h
index 2bba5d5..0639181 100644
--- a/sync/internal_api/public/base/model_type.h
+++ b/sync/internal_api/public/base/model_type.h
@@ -14,6 +14,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/base/enum_set.h"
 
@@ -292,8 +293,8 @@ SYNC_EXPORT std::string ModelTypeSetToString(ModelTypeSet model_types);
 SYNC_EXPORT ModelTypeSet ModelTypeSetFromString(
     const std::string& model_type_string);
 
-// Caller takes ownership of returned list.
-SYNC_EXPORT base::ListValue* ModelTypeSetToValue(ModelTypeSet model_types);
+SYNC_EXPORT scoped_ptr<base::ListValue> ModelTypeSetToValue(
+    ModelTypeSet model_types);
 
 SYNC_EXPORT ModelTypeSet ModelTypeSetFromValue(const base::ListValue& value);
 
diff --git a/sync/internal_api/public/change_record.h b/sync/internal_api/public/change_record.h
index 062d6fa..9f51737 100644
--- a/sync/internal_api/public/change_record.h
+++ b/sync/internal_api/public/change_record.h
@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
 #include "sync/base/sync_export.h"
 #include "sync/internal_api/public/util/immutable.h"
 #include "sync/protocol/password_specifics.pb.h"
@@ -29,8 +30,7 @@ class SYNC_EXPORT ExtraPasswordChangeRecordData {
       const sync_pb::PasswordSpecificsData& data);
   virtual ~ExtraPasswordChangeRecordData();
 
-  // Transfers ownership of the DictionaryValue to the caller.
-  virtual base::DictionaryValue* ToValue() const;
+  virtual scoped_ptr<base::DictionaryValue> ToValue() const;
 
   const sync_pb::PasswordSpecificsData& unencrypted() const;
  private:
@@ -50,8 +50,7 @@ struct SYNC_EXPORT_PRIVATE ChangeRecord {
   ChangeRecord();
   ~ChangeRecord();
 
-  // Transfers ownership of the DictionaryValue to the caller.
-  base::DictionaryValue* ToValue() const;
+  scoped_ptr<base::DictionaryValue> ToValue() const;
 
   int64 id;
   Action action;
diff --git a/sync/internal_api/public/change_record_unittest.cc b/sync/internal_api/public/change_record_unittest.cc
index 3d532d6..f12974e 100644
--- a/sync/internal_api/public/change_record_unittest.cc
+++ b/sync/internal_api/public/change_record_unittest.cc
@@ -53,7 +53,7 @@ void CheckChangeRecordValue(
   if (record.action == ChangeRecord::ACTION_DELETE) {
     scoped_ptr<base::DictionaryValue> expected_extra_value;
     if (record.extra.get()) {
-      expected_extra_value.reset(record.extra->ToValue());
+      expected_extra_value = record.extra->ToValue();
     }
     const base::Value* extra_value = NULL;
     EXPECT_EQ(record.extra.get() != NULL,
@@ -67,10 +67,32 @@ void CheckChangeRecordValue(
   }
 }
 
-class MockExtraChangeRecordData
-    : public ExtraPasswordChangeRecordData {
+class TestExtraChangeRecordData : public ExtraPasswordChangeRecordData {
  public:
-  MOCK_CONST_METHOD0(ToValue, base::DictionaryValue*());
+  TestExtraChangeRecordData()
+      : to_value_calls_(0), expected_to_value_calls_(0) {}
+
+  ~TestExtraChangeRecordData() override {
+    EXPECT_EQ(expected_to_value_calls_, to_value_calls_);
+  }
+
+  scoped_ptr<base::DictionaryValue> ToValue() const override {
+    const_cast<TestExtraChangeRecordData*>(this)->to_value_calls_++;
+    return dict_->CreateDeepCopy();
+  }
+
+  void set_dictionary_value(scoped_ptr<base::DictionaryValue> dict) {
+    dict_ = dict.Pass();
+  }
+
+  void set_expected_to_value_calls(size_t expectation) {
+    expected_to_value_calls_ = expectation;
+  }
+
+ private:
+  scoped_ptr<base::DictionaryValue> dict_;
+  size_t to_value_calls_;
+  size_t expected_to_value_calls_;
 };
 
 TEST_F(ChangeRecordTest, ChangeRecordToValue) {
@@ -87,7 +109,7 @@ TEST_F(ChangeRecordTest, ChangeRecordToValue) {
     record.action = ChangeRecord::ACTION_ADD;
     record.id = kTestId;
     record.specifics = old_specifics;
-    record.extra.reset(new StrictMock<MockExtraChangeRecordData>());
+    record.extra.reset(new TestExtraChangeRecordData());
     scoped_ptr<base::DictionaryValue> value(record.ToValue());
     CheckChangeRecordValue(record, *value);
   }
@@ -98,7 +120,7 @@ TEST_F(ChangeRecordTest, ChangeRecordToValue) {
     record.action = ChangeRecord::ACTION_UPDATE;
     record.id = kTestId;
     record.specifics = old_specifics;
-    record.extra.reset(new StrictMock<MockExtraChangeRecordData>());
+    record.extra.reset(new TestExtraChangeRecordData());
     scoped_ptr<base::DictionaryValue> value(record.ToValue());
     CheckChangeRecordValue(record, *value);
   }
@@ -120,12 +142,12 @@ TEST_F(ChangeRecordTest, ChangeRecordToValue) {
     record.id = kTestId;
     record.specifics = old_specifics;
 
-    base::DictionaryValue extra_value;
-    extra_value.SetString("foo", "bar");
-    scoped_ptr<StrictMock<MockExtraChangeRecordData> > extra(
-        new StrictMock<MockExtraChangeRecordData>());
-    EXPECT_CALL(*extra, ToValue()).Times(2).WillRepeatedly(
-        Invoke(&extra_value, &base::DictionaryValue::DeepCopy));
+    scoped_ptr<base::DictionaryValue> extra_value(new base::DictionaryValue());
+    extra_value->SetString("foo", "bar");
+    scoped_ptr<TestExtraChangeRecordData> extra(
+        new TestExtraChangeRecordData());
+    extra->set_dictionary_value(extra_value.Pass());
+    extra->set_expected_to_value_calls(2U);
 
     record.extra.reset(extra.release());
     scoped_ptr<base::DictionaryValue> value(record.ToValue());
diff --git a/sync/internal_api/public/engine/model_safe_worker.cc b/sync/internal_api/public/engine/model_safe_worker.cc
index ea8bd4b..9692a78 100644
--- a/sync/internal_api/public/engine/model_safe_worker.cc
+++ b/sync/internal_api/public/engine/model_safe_worker.cc
@@ -11,9 +11,9 @@
 
 namespace syncer {
 
-base::DictionaryValue* ModelSafeRoutingInfoToValue(
+scoped_ptr<base::DictionaryValue> ModelSafeRoutingInfoToValue(
     const ModelSafeRoutingInfo& routing_info) {
-  base::DictionaryValue* dict = new base::DictionaryValue();
+  scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   for (ModelSafeRoutingInfo::const_iterator it = routing_info.begin();
        it != routing_info.end(); ++it) {
     dict->SetString(ModelTypeToString(it->first),
@@ -24,8 +24,8 @@ base::DictionaryValue* ModelSafeRoutingInfoToValue(
 
 std::string ModelSafeRoutingInfoToString(
     const ModelSafeRoutingInfo& routing_info) {
-  scoped_ptr<base::DictionaryValue> dict(
-      ModelSafeRoutingInfoToValue(routing_info));
+  scoped_ptr<base::DictionaryValue> dict =
+      ModelSafeRoutingInfoToValue(routing_info);
   std::string json;
   base::JSONWriter::Write(dict.get(), &json);
   return json;
diff --git a/sync/internal_api/public/engine/model_safe_worker.h b/sync/internal_api/public/engine/model_safe_worker.h
index ad84d49..e2d04c2 100644
--- a/sync/internal_api/public/engine/model_safe_worker.h
+++ b/sync/internal_api/public/engine/model_safe_worker.h
@@ -11,6 +11,7 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
@@ -148,8 +149,8 @@ class SYNC_EXPORT ModelSafeWorker
 typedef std::map<ModelType, ModelSafeGroup> ModelSafeRoutingInfo;
 
 // Caller takes ownership of return value.
-SYNC_EXPORT_PRIVATE base::DictionaryValue* ModelSafeRoutingInfoToValue(
-    const ModelSafeRoutingInfo& routing_info);
+SYNC_EXPORT_PRIVATE scoped_ptr<base::DictionaryValue>
+ModelSafeRoutingInfoToValue(const ModelSafeRoutingInfo& routing_info);
 
 SYNC_EXPORT std::string ModelSafeRoutingInfoToString(
     const ModelSafeRoutingInfo& routing_info);
diff --git a/sync/internal_api/public/sessions/sync_session_snapshot.cc b/sync/internal_api/public/sessions/sync_session_snapshot.cc
index 4e7a2b1..3380882 100644
--- a/sync/internal_api/public/sessions/sync_session_snapshot.cc
+++ b/sync/internal_api/public/sessions/sync_session_snapshot.cc
@@ -54,7 +54,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
 
 SyncSessionSnapshot::~SyncSessionSnapshot() {}
 
-base::DictionaryValue* SyncSessionSnapshot::ToValue() const {
+scoped_ptr<base::DictionaryValue> SyncSessionSnapshot::ToValue() const {
   scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue());
   value->SetInteger("numSuccessfulCommits",
                     model_neutral_state_.num_successful_commits);
@@ -71,7 +71,7 @@ base::DictionaryValue* SyncSessionSnapshot::ToValue() const {
   value->SetInteger("numServerOverwrites",
                     model_neutral_state_.num_server_overwrites);
   value->Set("downloadProgressMarkers",
-             ProgressMarkerMapToValue(download_progress_markers_).release());
+             ProgressMarkerMapToValue(download_progress_markers_));
   value->SetBoolean("isSilenced", is_silenced_);
   // We don't care too much if we lose precision here, also.
   value->SetInteger("numEncryptionConflicts",
@@ -96,16 +96,14 @@ base::DictionaryValue* SyncSessionSnapshot::ToValue() const {
     const std::string model_type = ModelTypeToString(static_cast<ModelType>(i));
     counter_entries->Set(model_type, type_entries.release());
   }
-  value->Set("counter_entries", counter_entries.release());
-  return value.release();
+  value->Set("counter_entries", counter_entries.Pass());
+  return value;
 }
 
 std::string SyncSessionSnapshot::ToString() const {
-  scoped_ptr<base::DictionaryValue> value(ToValue());
   std::string json;
-  base::JSONWriter::WriteWithOptions(value.get(),
-                                     base::JSONWriter::OPTIONS_PRETTY_PRINT,
-                                     &json);
+  base::JSONWriter::WriteWithOptions(
+      ToValue().get(), base::JSONWriter::OPTIONS_PRETTY_PRINT, &json);
   return json;
 }
 
diff --git a/sync/internal_api/public/sessions/sync_session_snapshot.h b/sync/internal_api/public/sessions/sync_session_snapshot.h
index a764de1..8641fbd 100644
--- a/sync/internal_api/public/sessions/sync_session_snapshot.h
+++ b/sync/internal_api/public/sessions/sync_session_snapshot.h
@@ -44,8 +44,7 @@ class SYNC_EXPORT SyncSessionSnapshot {
       sync_pb::GetUpdatesCallerInfo::GetUpdatesSource legacy_updates_source);
   ~SyncSessionSnapshot();
 
-  // Caller takes ownership of the returned dictionary.
-  base::DictionaryValue* ToValue() const;
+  scoped_ptr<base::DictionaryValue> ToValue() const;
 
   std::string ToString() const;