Make IDMap respect C++ rule of three,

i.e. when the dtor is defined, copy ctor
and assignment operator should also be defined.

We can't use DISALLOW_COPY_AND_ASSIGN because IDMap
is being copied/assigned in the codebase.

BUG=none
TEST=none


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

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

2 files changed, 100 insertions, 3 deletions

diff --git a/base/id_map.h b/base/id_map.h
index d3f23aa..bb9f05b 100644
--- a/base/id_map.h
+++ b/base/id_map.h
@@ -110,6 +110,12 @@ class IDMap : public base::NonThreadSafe {
     return data_.size() - removed_ids_.size();
   }
 
+#if defined(UNIT_TEST)
+  int iteration_depth() const {
+    return iteration_depth_;
+  }
+#endif  // defined(UNIT_TEST)
+
   // It is safe to remove elements from the map during iteration. All iterators
   // will remain valid.
   template<class ReturnType>
@@ -118,13 +124,29 @@ class IDMap : public base::NonThreadSafe {
     Iterator(IDMap<T, OS>* map)
         : map_(map),
           iter_(map_->data_.begin()) {
-      DCHECK(map->CalledOnValidThread());
-      ++map_->iteration_depth_;
-      SkipRemovedEntries();
+      Init();
+    }
+
+    Iterator(const Iterator& iter)
+        : map_(iter.map_),
+          iter_(iter.iter_) {
+      Init();
+    }
+
+    const Iterator& operator=(const Iterator& iter) {
+      map_ = iter.map;
+      iter_ = iter.iter;
+      Init();
+      return *this;
     }
 
     ~Iterator() {
       DCHECK(map_->CalledOnValidThread());
+
+      // We're going to decrement iteration depth. Make sure it's greater than
+      // zero so that it doesn't become negative.
+      DCHECK_LT(0, map_->iteration_depth_);
+
       if (--map_->iteration_depth_ == 0)
         map_->Compact();
     }
@@ -151,6 +173,12 @@ class IDMap : public base::NonThreadSafe {
     }
 
    private:
+    void Init() {
+      DCHECK(map_->CalledOnValidThread());
+      ++map_->iteration_depth_;
+      SkipRemovedEntries();
+    }
+
     void SkipRemovedEntries() {
       while (iter_ != map_->data_.end() &&
              map_->removed_ids_.find(iter_->first) !=
diff --git a/base/id_map_unittest.cc b/base/id_map_unittest.cc
index feb150b..c4838ee 100644
--- a/base/id_map_unittest.cc
+++ b/base/id_map_unittest.cc
@@ -54,6 +54,8 @@ TEST_F(IDMapTest, Basic) {
   map.AddWithID(&obj2, 2);
   EXPECT_EQ(&obj1, map.Lookup(1));
   EXPECT_EQ(&obj2, map.Lookup(2));
+
+  EXPECT_EQ(0, map.iteration_depth());
 }
 
 TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingCurrentElement) {
@@ -69,6 +71,9 @@ TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingCurrentElement) {
 
   {
     IDMap<TestObject>::const_iterator iter(&map);
+
+    EXPECT_EQ(1, map.iteration_depth());
+
     while (!iter.IsAtEnd()) {
       map.Remove(iter.GetCurrentKey());
       iter.Advance();
@@ -82,6 +87,8 @@ TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingCurrentElement) {
 
   EXPECT_TRUE(map.IsEmpty());
   EXPECT_EQ(0U, map.size());
+
+  EXPECT_EQ(0, map.iteration_depth());
 }
 
 TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingOtherElements) {
@@ -97,6 +104,8 @@ TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingOtherElements) {
   int counter = 0;
   for (IDMap<TestObject>::const_iterator iter(&map);
        !iter.IsAtEnd(); iter.Advance()) {
+    EXPECT_EQ(1, map.iteration_depth());
+
     switch (counter) {
       case 0:
         EXPECT_EQ(ids[0], iter.GetCurrentKey());
@@ -120,6 +129,66 @@ TEST_F(IDMapTest, IteratorRemainsValidWhenRemovingOtherElements) {
 
     counter++;
   }
+
+  EXPECT_EQ(0, map.iteration_depth());
+}
+
+TEST_F(IDMapTest, CopyIterator) {
+  IDMap<TestObject> map;
+
+  TestObject obj1;
+  TestObject obj2;
+  TestObject obj3;
+
+  map.Add(&obj1);
+  map.Add(&obj2);
+  map.Add(&obj3);
+
+  EXPECT_EQ(0, map.iteration_depth());
+
+  {
+    IDMap<TestObject>::const_iterator iter1(&map);
+    EXPECT_EQ(1, map.iteration_depth());
+
+    // Make sure that copying the iterator correctly increments
+    // map's iteration depth.
+    IDMap<TestObject>::const_iterator iter2(iter1);
+    EXPECT_EQ(2, map.iteration_depth());
+  }
+
+  // Make sure after destroying all iterators the map's iteration depth
+  // returns to initial state.
+  EXPECT_EQ(0, map.iteration_depth());
+}
+
+TEST_F(IDMapTest, AssignIterator) {
+  IDMap<TestObject> map;
+
+  TestObject obj1;
+  TestObject obj2;
+  TestObject obj3;
+
+  map.Add(&obj1);
+  map.Add(&obj2);
+  map.Add(&obj3);
+
+  EXPECT_EQ(0, map.iteration_depth());
+
+  {
+    IDMap<TestObject>::const_iterator iter1(&map);
+    EXPECT_EQ(1, map.iteration_depth());
+
+    IDMap<TestObject>::const_iterator iter2(&map);
+    EXPECT_EQ(2, map.iteration_depth());
+
+    // Make sure that assigning the iterator correctly updates
+    // map's iteration depth (-1 for destruction, +1 for assignment).
+    EXPECT_EQ(2, map.iteration_depth());
+  }
+
+  // Make sure after destroying all iterators the map's iteration depth
+  // returns to initial state.
+  EXPECT_EQ(0, map.iteration_depth());
 }
 
 TEST_F(IDMapTest, OwningPointersDeletesThemOnRemove) {