Make element removal methods in DictionaryValue and ListValue take scoped_ptr's as outparams.

TBR=pneubeck@chromium.org,scottbyer@chromium.org
BUG=263894

Review URL: https://chromiumcodereview.appspot.com/21030009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215885 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 47 insertions, 53 deletions

diff --git a/base/debug/trace_event_unittest.cc b/base/debug/trace_event_unittest.cc
index 5beea37..ea295c0 100644
--- a/base/debug/trace_event_unittest.cc
+++ b/base/debug/trace_event_unittest.cc
@@ -149,9 +149,9 @@ void TraceEventTestFixture::OnTraceDataCollected(
 
   // Move items into our aggregate collection
   while (root_list->GetSize()) {
-    Value* item = NULL;
+    scoped_ptr<Value> item;
     root_list->Remove(0, &item);
-    trace_parsed_.Append(item);
+    trace_parsed_.Append(item.release());
   }
 }
 
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 4eb9e9a..f1f4333 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -56,7 +56,7 @@ class DictionaryHiddenRootValue : public base::DictionaryValue {
   // the method below.
 
   virtual bool RemoveWithoutPathExpansion(const std::string& key,
-                                          Value** out) OVERRIDE {
+                                          scoped_ptr<Value>* out) OVERRIDE {
     // If the caller won't take ownership of the removed value, just call up.
     if (!out)
       return DictionaryValue::RemoveWithoutPathExpansion(key, out);
@@ -65,12 +65,11 @@ class DictionaryHiddenRootValue : public base::DictionaryValue {
 
     // Otherwise, remove the value while its still "owned" by this and copy it
     // to convert any JSONStringValues to std::string.
-    Value* out_owned = NULL;
+    scoped_ptr<Value> out_owned;
     if (!DictionaryValue::RemoveWithoutPathExpansion(key, &out_owned))
       return false;
 
-    *out = out_owned->DeepCopy();
-    delete out_owned;
+    out->reset(out_owned->DeepCopy());
 
     return true;
   }
@@ -103,7 +102,7 @@ class ListHiddenRootValue : public base::ListValue {
     ListValue::Swap(copy.get());
   }
 
-  virtual bool Remove(size_t index, Value** out) OVERRIDE {
+  virtual bool Remove(size_t index, scoped_ptr<Value>* out) OVERRIDE {
     // If the caller won't take ownership of the removed value, just call up.
     if (!out)
       return ListValue::Remove(index, out);
@@ -112,12 +111,11 @@ class ListHiddenRootValue : public base::ListValue {
 
     // Otherwise, remove the value while its still "owned" by this and copy it
     // to convert any JSONStringValues to std::string.
-    Value* out_owned = NULL;
+    scoped_ptr<Value> out_owned;
     if (!ListValue::Remove(index, &out_owned))
       return false;
 
-    *out = out_owned->DeepCopy();
-    delete out_owned;
+    out->reset(out_owned->DeepCopy());
 
     return true;
   }
diff --git a/base/json/json_reader_unittest.cc b/base/json/json_reader_unittest.cc
index 41650b4..527cb97 100644
--- a/base/json/json_reader_unittest.cc
+++ b/base/json/json_reader_unittest.cc
@@ -559,9 +559,12 @@ TEST(JSONReaderTest, ReadFromFile) {
 // Tests that the root of a JSON object can be deleted safely while its
 // children outlive it.
 TEST(JSONReaderTest, StringOptimizations) {
-  Value* dict_literals[2] = {0};
-  Value* dict_strings[2] = {0};
-  Value* list_values[2] = {0};
+  scoped_ptr<Value> dict_literal_0;
+  scoped_ptr<Value> dict_literal_1;
+  scoped_ptr<Value> dict_string_0;
+  scoped_ptr<Value> dict_string_1;
+  scoped_ptr<Value> list_value_0;
+  scoped_ptr<Value> list_value_1;
 
   {
     scoped_ptr<Value> root(JSONReader::Read(
@@ -588,43 +591,36 @@ TEST(JSONReaderTest, StringOptimizations) {
     ASSERT_TRUE(root_dict->GetDictionary("test", &dict));
     ASSERT_TRUE(root_dict->GetList("list", &list));
 
-    EXPECT_TRUE(dict->Remove("foo", &dict_literals[0]));
-    EXPECT_TRUE(dict->Remove("bar", &dict_literals[1]));
-    EXPECT_TRUE(dict->Remove("baz", &dict_strings[0]));
-    EXPECT_TRUE(dict->Remove("moo", &dict_strings[1]));
+    EXPECT_TRUE(dict->Remove("foo", &dict_literal_0));
+    EXPECT_TRUE(dict->Remove("bar", &dict_literal_1));
+    EXPECT_TRUE(dict->Remove("baz", &dict_string_0));
+    EXPECT_TRUE(dict->Remove("moo", &dict_string_1));
 
     ASSERT_EQ(2u, list->GetSize());
-    EXPECT_TRUE(list->Remove(0, &list_values[0]));
-    EXPECT_TRUE(list->Remove(0, &list_values[1]));
+    EXPECT_TRUE(list->Remove(0, &list_value_0));
+    EXPECT_TRUE(list->Remove(0, &list_value_1));
   }
 
   bool b = false;
   double d = 0;
   std::string s;
 
-  EXPECT_TRUE(dict_literals[0]->GetAsBoolean(&b));
+  EXPECT_TRUE(dict_literal_0->GetAsBoolean(&b));
   EXPECT_TRUE(b);
 
-  EXPECT_TRUE(dict_literals[1]->GetAsDouble(&d));
+  EXPECT_TRUE(dict_literal_1->GetAsDouble(&d));
   EXPECT_EQ(3.14, d);
 
-  EXPECT_TRUE(dict_strings[0]->GetAsString(&s));
+  EXPECT_TRUE(dict_string_0->GetAsString(&s));
   EXPECT_EQ("bat", s);
 
-  EXPECT_TRUE(dict_strings[1]->GetAsString(&s));
+  EXPECT_TRUE(dict_string_1->GetAsString(&s));
   EXPECT_EQ("cow", s);
 
-  EXPECT_TRUE(list_values[0]->GetAsString(&s));
+  EXPECT_TRUE(list_value_0->GetAsString(&s));
   EXPECT_EQ("a", s);
-  EXPECT_TRUE(list_values[1]->GetAsString(&s));
+  EXPECT_TRUE(list_value_1->GetAsString(&s));
   EXPECT_EQ("b", s);
-
-  delete dict_literals[0];
-  delete dict_literals[1];
-  delete dict_strings[0];
-  delete dict_strings[1];
-  delete list_values[0];
-  delete list_values[1];
 }
 
 // A smattering of invalid JSON designed to test specific portions of the
diff --git a/base/values.cc b/base/values.cc
index 712fff39..adfb980 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -719,7 +719,8 @@ bool DictionaryValue::GetListWithoutPathExpansion(const std::string& key,
           const_cast<const ListValue**>(out_value));
 }
 
-bool DictionaryValue::Remove(const std::string& path, Value** out_value) {
+bool DictionaryValue::Remove(const std::string& path,
+                             scoped_ptr<Value>* out_value) {
   DCHECK(IsStringUTF8(path));
   std::string current_path(path);
   DictionaryValue* current_dictionary = this;
@@ -736,7 +737,7 @@ bool DictionaryValue::Remove(const std::string& path, Value** out_value) {
 }
 
 bool DictionaryValue::RemoveWithoutPathExpansion(const std::string& key,
-                                                 Value** out_value) {
+                                                 scoped_ptr<Value>* out_value) {
   DCHECK(IsStringUTF8(key));
   ValueMap::iterator entry_iterator = dictionary_.find(key);
   if (entry_iterator == dictionary_.end())
@@ -744,7 +745,7 @@ bool DictionaryValue::RemoveWithoutPathExpansion(const std::string& key,
 
   Value* entry = entry_iterator->second;
   if (out_value)
-    *out_value = entry;
+    out_value->reset(entry);
   else
     delete entry;
   dictionary_.erase(entry_iterator);
@@ -958,12 +959,12 @@ bool ListValue::GetList(size_t index, ListValue** out_value) {
       const_cast<const ListValue**>(out_value));
 }
 
-bool ListValue::Remove(size_t index, Value** out_value) {
+bool ListValue::Remove(size_t index, scoped_ptr<Value>* out_value) {
   if (index >= list_.size())
     return false;
 
   if (out_value)
-    *out_value = list_[index];
+    out_value->reset(list_[index]);
   else
     delete list_[index];
 
@@ -986,9 +987,10 @@ bool ListValue::Remove(const Value& value, size_t* index) {
   return false;
 }
 
-ListValue::iterator ListValue::Erase(iterator iter, Value** out_value) {
+ListValue::iterator ListValue::Erase(iterator iter,
+                                     scoped_ptr<Value>* out_value) {
   if (out_value)
-    *out_value = *iter;
+    out_value->reset(*iter);
   else
     delete *iter;
 
diff --git a/base/values.h b/base/values.h
index 06e9458..4025c75 100644
--- a/base/values.h
+++ b/base/values.h
@@ -311,16 +311,16 @@ class BASE_EXPORT DictionaryValue : public Value {
 
   // Removes the Value with the specified path from this dictionary (or one
   // of its child dictionaries, if the path is more than just a local key).
-  // If |out_value| is non-NULL, the removed Value AND ITS OWNERSHIP will be
-  // passed out via out_value.  If |out_value| is NULL, the removed value will
-  // be deleted.  This method returns true if |path| is a valid path; otherwise
-  // it will return false and the DictionaryValue object will be unchanged.
-  virtual bool Remove(const std::string& path, Value** out_value);
+  // If |out_value| is non-NULL, the removed Value will be passed out via
+  // |out_value|.  If |out_value| is NULL, the removed value will be deleted.
+  // This method returns true if |path| is a valid path; otherwise it will
+  // return false and the DictionaryValue object will be unchanged.
+  virtual bool Remove(const std::string& path, scoped_ptr<Value>* out_value);
 
   // Like Remove(), but without special treatment of '.'.  This allows e.g. URLs
   // to be used as paths.
   virtual bool RemoveWithoutPathExpansion(const std::string& key,
-                                          Value** out_value);
+                                          scoped_ptr<Value>* out_value);
 
   // Makes a copy of |this| but doesn't include empty dictionaries and lists in
   // the copy.  This never returns NULL, even if |this| itself is empty.
@@ -414,7 +414,7 @@ class BASE_EXPORT ListValue : public Value {
   // passed out via |out_value|.  If |out_value| is NULL, the removed value will
   // be deleted.  This method returns true if |index| is valid; otherwise
   // it will return false and the ListValue object will be unchanged.
-  virtual bool Remove(size_t index, Value** out_value);
+  virtual bool Remove(size_t index, scoped_ptr<Value>* out_value);
 
   // Removes the first instance of |value| found in the list, if any, and
   // deletes it. |index| is the location where |value| was found. Returns false
@@ -425,7 +425,7 @@ class BASE_EXPORT ListValue : public Value {
   // deleted, otherwise ownership of the value is passed back to the caller.
   // Returns an iterator pointing to the location of the element that
   // followed the erased element.
-  iterator Erase(iterator iter, Value** out_value);
+  iterator Erase(iterator iter, scoped_ptr<Value>* out_value);
 
   // Appends a Value to the end of the list.
   void Append(Value* in_value);
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 8776c10..733c485 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -203,7 +203,7 @@ TEST(ValuesTest, ListDeletion) {
 
 TEST(ValuesTest, ListRemoval) {
   bool deletion_flag = true;
-  Value* removed_item = NULL;
+  scoped_ptr<Value> removed_item;
 
   {
     ListValue list;
@@ -218,8 +218,7 @@ TEST(ValuesTest, ListRemoval) {
     EXPECT_EQ(0U, list.GetSize());
   }
   EXPECT_FALSE(deletion_flag);
-  delete removed_item;
-  removed_item = NULL;
+  removed_item.reset();
   EXPECT_TRUE(deletion_flag);
 
   {
@@ -275,7 +274,7 @@ TEST(ValuesTest, DictionaryDeletion) {
 TEST(ValuesTest, DictionaryRemoval) {
   std::string key = "test";
   bool deletion_flag = true;
-  Value* removed_item = NULL;
+  scoped_ptr<Value> removed_item;
 
   {
     DictionaryValue dict;
@@ -288,8 +287,7 @@ TEST(ValuesTest, DictionaryRemoval) {
     ASSERT_TRUE(removed_item);
   }
   EXPECT_FALSE(deletion_flag);
-  delete removed_item;
-  removed_item = NULL;
+  removed_item.reset();
   EXPECT_TRUE(deletion_flag);
 
   {