Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal store.

base/values.h

Index: base/values.h
diff --git a/base/values.h b/base/values.h
index e2506cc14f5515ab9fba9a76c1995fe8575dc9d5..e65cf1cdd4de8f657ae5b3ebbd5de3646c12f940 100644
--- a/base/values.h
+++ b/base/values.h
@@ -42,9 +42,6 @@ class ListValue;
 class StringValue;
 class Value;
 
-typedef std::vector<Value*> ValueVector;
-typedef std::map<std::string, Value*> ValueMap;
-
 // The Value class is the base class for Values. A Value can be instantiated
 // via the Create*Value() factory methods, or by directly creating instances of
 // the subclasses.
@@ -210,6 +207,7 @@ class BASE_EXPORT BinaryValue: public Value {
 // are |std::string|s and should be UTF-8 encoded.
 class BASE_EXPORT DictionaryValue : public Value {
  public:
+  using Storage = std::map<std::string, std::unique_ptr<Value>>;
   // Returns |value| if it is a dictionary, nullptr otherwise.
   static std::unique_ptr<DictionaryValue> From(std::unique_ptr<Value> value);
 
@@ -372,7 +370,7 @@ class BASE_EXPORT DictionaryValue : public Value {
 
    private:
     const DictionaryValue& target_;
-    ValueMap::const_iterator it_;
+    Storage::const_iterator it_;
   };
 
   // Overridden from Value:
@@ -382,7 +380,7 @@ class BASE_EXPORT DictionaryValue : public Value {
   bool Equals(const Value* other) const override;
 
  private:
-  ValueMap dictionary_;
+  Storage dictionary_;
 
   DISALLOW_COPY_AND_ASSIGN(DictionaryValue);
 };
@@ -390,8 +388,20 @@ class BASE_EXPORT DictionaryValue : public Value {
 // This type of Value represents a list of other Value values.
 class BASE_EXPORT ListValue : public Value {
  public:
-  typedef ValueVector::iterator iterator;
-  typedef ValueVector::const_iterator const_iterator;
+  using Storage = std::vector<std::unique_ptr<Value>>;
+  class iterator : public Storage::iterator {

[dcheng, 2016/05/20 19:49:47]

There's ~160 dependencies on iterator wrapping a raw pointer right now, which
makes it a little annoying to change ListValue and DictionaryValue to hold
std::unique_ptr<Value>'s internally rather than raw Value pointers.

This is a shim so the external consumers still iterate over the raw pointer
version. The use of std::unique_ptr internally feels like an implementation
detail, so it might be nice to avoid exposing it.

But even if we do want to expose it, it's probably best to separate it into a
separate patch given the number of changes.

[danakj, 2016/05/20 20:51:38]

The subclassing part is weird. It'd be better if you made it a member I think
and just expose the API you want to expose.

[dcheng, 2016/05/20 22:41:55]

I could do that, but it makes it a bit more painful. I'll have to double up the
implementations of:
- operator++
- operator*
- operator->
- operator==
- operator!=
- in addition, operator== and operator!= need to be explicitly made comparable
now.
- the iterator conversion constructor and the iterator copy constructor both
need to be out-of-lined since the clang plugin will now complain
- for STL friendliness, both should derive from
std::iterator<std::forward_iterator_tag, Value> as well.

Finally, I need to expose a way to poke at the internal iterator for code in
values.cc, but this is going to be publicly visible outside base::Value anyway.

In short, there's quite a bit of overhead from not subclassing and it actually
gets a bit harder to read.

With the current subclassing, it's pretty straightforward: we want the exact
same behavior as the wrapped iterator, except for unary operator*.

+   public:
+    iterator(const Storage::iterator& it) : Storage::iterator(it) {}
+    Value* operator*() { return Storage::iterator::operator*().get(); }
+  };
+  class const_iterator : public Storage::const_iterator {
+   public:
+    const_iterator(const Storage::const_iterator& it)
+        : Storage::const_iterator(it) {}
+    const Value* operator*() {

[dcheng, 2016/05/20 19:49:47]

I believe const_iterator just returns Value* here. I could make it match.

+      return Storage::const_iterator::operator*().get();
+    }
+  };
 
   // Returns |value| if it is a list, nullptr otherwise.
   static std::unique_ptr<ListValue> From(std::unique_ptr<Value> value);
@@ -508,7 +518,7 @@ class BASE_EXPORT ListValue : public Value {
   std::unique_ptr<ListValue> CreateDeepCopy() const;
 
  private:
-  ValueVector list_;
+  Storage list_;
 
   DISALLOW_COPY_AND_ASSIGN(ListValue);
 };