Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base

base/json/json_parser.cc

Index: base/json/json_parser.cc
diff --git a/base/json/json_parser.cc b/base/json/json_parser.cc
index 801c84a24cad7d344e6266cd2ccf309b4c976c06..c1bcf4a92758ed6dc6dfcbb332ffa4cd3c7a0bee 100644
--- a/base/json/json_parser.cc
+++ b/base/json/json_parser.cc
@@ -5,10 +5,11 @@
 #include "base/json/json_parser.h"
 
 #include <cmath>
-#include <memory>
+#include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -27,16 +28,19 @@ const int kStackMaxDepth = 100;
 
 const int32_t kExtendedASCIIStart = 0x80;
 
-// This and the class below are used to own the JSON input string for when
-// string tokens are stored as StringPiece instead of std::string. This
-// optimization avoids about 2/3rds of string memory copies. The constructor
-// takes ownership of the input string. The real root value is Swap()ed into
-// the new instance.
+// DictionaryHiddenRootValue and ListHiddenRootValue are used in conjunction
+// with JSONStringValue as an optimization for reducing the number of string
+// copies. When this optimization is active, the parser uses a hidden root to
+// keep the original JSON input string live and creates JSONStringValue children
+// holding StringPiece references to the input string, avoiding about 2/3rds of
+// string memory copies. The real root value is Swap()ed into the new instance.
 class DictionaryHiddenRootValue : public DictionaryValue {
  public:
-  DictionaryHiddenRootValue(std::string* json, Value* root) : json_(json) {
+  DictionaryHiddenRootValue(std::unique_ptr<std::string> json,
+                            std::unique_ptr<Value> root)
+      : json_(std::move(json)) {
     DCHECK(root->IsType(Value::TYPE_DICTIONARY));
-    DictionaryValue::Swap(static_cast<DictionaryValue*>(root));
+    DictionaryValue::Swap(static_cast<DictionaryValue*>(root.get()));

[danakj, 2016/04/28 17:39:37]

This should be passing the unique_ptr. Can you TODO somewhere?

[dcheng, 2016/04/28 18:03:23]

Should it though? It's not really moving/taking ownership of it so much as
swapping the contents: after all, things like std::vector::swap() don't require
passing in an rvalue reference.

[danakj, 2016/04/28 18:09:19]

Oh I guess you're right yah, it's swapping stuff into this Value here. ok.

   }
 
   void Swap(DictionaryValue* other) override {
@@ -44,7 +48,7 @@ class DictionaryHiddenRootValue : public DictionaryValue {
 
     // First deep copy to convert JSONStringValue to std::string and swap that
     // copy with |other|, which contains the new contents of |this|.
-    std::unique_ptr<DictionaryValue> copy(DeepCopy());
+    std::unique_ptr<DictionaryValue> copy(CreateDeepCopy());
     copy->Swap(other);
 
     // Then erase the contents of the current dictionary and swap in the
@@ -71,7 +75,7 @@ class DictionaryHiddenRootValue : public DictionaryValue {
     if (!DictionaryValue::RemoveWithoutPathExpansion(key, &out_owned))
       return false;
 
-    out->reset(out_owned->DeepCopy());
+    *out = out_owned->CreateDeepCopy();
 
     return true;
   }
@@ -84,9 +88,11 @@ class DictionaryHiddenRootValue : public DictionaryValue {
 
 class ListHiddenRootValue : public ListValue {
  public:
-  ListHiddenRootValue(std::string* json, Value* root) : json_(json) {
+  ListHiddenRootValue(std::unique_ptr<std::string> json,
+                      std::unique_ptr<Value> root)
+      : json_(std::move(json)) {
     DCHECK(root->IsType(Value::TYPE_LIST));
-    ListValue::Swap(static_cast<ListValue*>(root));
+    ListValue::Swap(static_cast<ListValue*>(root.get()));

[danakj, 2016/04/28 17:39:37]

ditto

[dcheng, 2016/04/28 18:03:23]

See previous.

   }
 
   void Swap(ListValue* other) override {
@@ -94,7 +100,7 @@ class ListHiddenRootValue : public ListValue {
 
     // First deep copy to convert JSONStringValue to std::string and swap that
     // copy with |other|, which contains the new contents of |this|.
-    std::unique_ptr<ListValue> copy(DeepCopy());
+    std::unique_ptr<ListValue> copy(CreateDeepCopy());
     copy->Swap(other);
 
     // Then erase the contents of the current list and swap in the new contents,
@@ -117,7 +123,7 @@ class ListHiddenRootValue : public ListValue {
     if (!ListValue::Remove(index, &out_owned))

[danakj, 2016/04/28 17:39:37]

heh, we should just have this return std::unique_ptr and null means the same as
false currently

[dcheng, 2016/04/28 18:03:23]

Acknowledged.

       return false;
 
-    out->reset(out_owned->DeepCopy());
+    *out = out_owned->CreateDeepCopy();
 
     return true;
   }
@@ -133,10 +139,8 @@ class ListHiddenRootValue : public ListValue {
 // otherwise the referenced string will not be guaranteed to outlive it.
 class JSONStringValue : public Value {
  public:
-  explicit JSONStringValue(const StringPiece& piece)
-      : Value(TYPE_STRING),
-        string_piece_(piece) {
-  }
+  explicit JSONStringValue(StringPiece piece)
+      : Value(TYPE_STRING), string_piece_(piece) {}

[danakj, 2016/04/28 17:39:37]

nit: move piece? (i know its the same as copy atm)

[dcheng, 2016/04/28 18:03:23]

Why though? copy on a StringPiece will be cheaper than move anyway.

[jbroman, 2016/04/28 18:06:18]

nit-nit: base::StringPiece explicitly promises in its header that it's
essentially free to copy and consists only of a pointer + length. Moving it feel
like writing using std::move on an int, gfx::Point, or similar.

[danakj, 2016/04/28 18:09:19]

Ah I see I see. #newtostrings. LGTM

 
   // Overridden from Value:
   bool GetAsString(std::string* out_value) const override {
@@ -203,13 +207,13 @@ JSONParser::JSONParser(int options)
 JSONParser::~JSONParser() {
 }
 
-Value* JSONParser::Parse(const StringPiece& input) {
+std::unique_ptr<Value> JSONParser::Parse(StringPiece input) {
   std::unique_ptr<std::string> input_copy;
   // If the children of a JSON root can be detached, then hidden roots cannot
   // be used, so do not bother copying the input because StringPiece will not
   // be used anywhere.
   if (!(options_ & JSON_DETACHABLE_CHILDREN)) {
-    input_copy.reset(new std::string(input.as_string()));
+    input_copy = WrapUnique(new std::string(input.as_string()));
     start_pos_ = input_copy->data();
   } else {
     start_pos_ = input.data();
@@ -236,14 +240,14 @@ Value* JSONParser::Parse(const StringPiece& input) {
 
   // Parse the first and any nested tokens.
   std::unique_ptr<Value> root(ParseNextToken());
-  if (!root.get())
-    return NULL;
+  if (!root)
+    return nullptr;
 
   // Make sure the input stream is at an end.
   if (GetNextToken() != T_END_OF_INPUT) {
     if (!CanConsume(1) || (NextChar() && GetNextToken() != T_END_OF_INPUT)) {
       ReportError(JSONReader::JSON_UNEXPECTED_DATA_AFTER_ROOT, 1);
-      return NULL;
+      return nullptr;
     }
   }
 
@@ -251,19 +255,21 @@ Value* JSONParser::Parse(const StringPiece& input) {
   // hidden root.
   if (!(options_ & JSON_DETACHABLE_CHILDREN)) {
     if (root->IsType(Value::TYPE_DICTIONARY)) {
-      return new DictionaryHiddenRootValue(input_copy.release(), root.get());
+      return WrapUnique(new DictionaryHiddenRootValue(std::move(input_copy),
+                                                      std::move(root)));
     } else if (root->IsType(Value::TYPE_LIST)) {
-      return new ListHiddenRootValue(input_copy.release(), root.get());
+      return WrapUnique(
+          new ListHiddenRootValue(std::move(input_copy), std::move(root)));
     } else if (root->IsType(Value::TYPE_STRING)) {
       // A string type could be a JSONStringValue, but because there's no
       // corresponding HiddenRootValue, the memory will be lost. Deep copy to
       // preserve it.
-      return root->DeepCopy();
+      return root->CreateDeepCopy();
     }
   }
 
   // All other values can be returned directly.
-  return root.release();
+  return root;
 }
 
 JSONReader::JsonParseError JSONParser::error_code() const {