Simplified ObserverListBase.

Made ObserverListBase's iterator and const_iterator the same type. ObserverListBase::iterator doesn't allow to modify the collection anyway. There was no good reason for const_iterator and iterator to be different types. Making them the same type just makes the code simpler.
Inlined ObserverListBase methods.
Removed unnecessary friend declarations.
Removed old MSVC workarounds.

In the end: less code to do exactly the same thing.

Change-Id: I0448a8a52f904635a2e30d094bcc2b48780e4d64
Reviewed-on: https://chromium-review.googlesource.com/773758
Commit-Queue: François Degros <fdegros@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517775}

base/observer_list.h

@@ -42,7 +42,7 @@
 //       observer_list_.AddObserver(obs);
 //     }
 //
-//     void RemoveObserver(Observer* obs) {
+//     void RemoveObserver(const Observer* obs) {
 //       observer_list_.RemoveObserver(obs);
 //     }
 //
@@ -66,9 +66,6 @@
 
 namespace base {
 
-template <typename ObserverType>
-class ObserverListThreadSafe;
-
 template <class ObserverType>
 class ObserverListBase
     : public SupportsWeakPtr<ObserverListBase<ObserverType>> {
@@ -85,14 +82,30 @@ class ObserverListBase
   };
 
   // An iterator class that can be used to access the list of observers.
-  template <class ContainerType>
   class Iter {
    public:
-    Iter();
-    explicit Iter(ContainerType* list);
-    ~Iter();
+    Iter() : index_(0), max_index_(0) {}
+
+    explicit Iter(const ObserverListBase* list)
+        : list_(const_cast<ObserverListBase*>(list)->AsWeakPtr()),
+          index_(0),
+          max_index_(list->type_ == NOTIFY_ALL
+                         ? std::numeric_limits<size_t>::max()
+                         : list->observers_.size()) {
+      DCHECK(list_);
+      EnsureValidIndex();
+      ++list_->live_iterator_count_;
+    }
+
+    ~Iter() {
+      if (!list_)
+        return;
+
+      DCHECK_GT(list_->live_iterator_count_, 0);
+      if (--list_->live_iterator_count_ == 0)
+        list_->Compact();
+    }
 
-    // Copy constructor.
     Iter(const Iter& other)
         : list_(other.list_),
           index_(other.index_),
@@ -101,7 +114,6 @@ class ObserverListBase
         ++list_->live_iterator_count_;
     }
 
-    // Unified assignment operator.
     Iter& operator=(Iter other) {
       using std::swap;
       swap(list_, other.list_);
@@ -110,23 +122,49 @@ class ObserverListBase
       return *this;
     }
 
-    // A workaround for C2244. MSVC requires fully qualified type name for
-    // return type on a function definition to match a function declaration.
-    using ThisType =
-        typename ObserverListBase<ObserverType>::template Iter<ContainerType>;
+    bool operator==(const Iter& other) const {
+      return (is_end() && other.is_end()) ||
+             (list_.get() == other.list_.get() && index_ == other.index_);
+    }
+
+    bool operator!=(const Iter& other) const { return !(*this == other); }
+
+    Iter& operator++() {
+      if (list_) {
+        ++index_;
+        EnsureValidIndex();
+      }
+      return *this;
+    }
+
+    ObserverType* operator->() const {
+      ObserverType* const current = GetCurrent();
+      DCHECK(current);
+      return current;
+    }
 
-    bool operator==(const Iter& other) const;
-    bool operator!=(const Iter& other) const;
-    ThisType& operator++();
-    ObserverType* operator->() const;
-    ObserverType& operator*() const;
+    ObserverType& operator*() const {
+      ObserverType* const current = GetCurrent();
+      DCHECK(current);
+      return *current;
+    }
 
    private:
     FRIEND_TEST_ALL_PREFIXES(ObserverListTest, BasicStdIterator);
     FRIEND_TEST_ALL_PREFIXES(ObserverListTest, StdIteratorRemoveFront);
 
-    ObserverType* GetCurrent() const;
-    void EnsureValidIndex();
+    ObserverType* GetCurrent() const {
+      DCHECK(list_);
+      DCHECK_LT(index_, clamped_max_index());
+      return list_->observers_[index_];
+    }
+
+    void EnsureValidIndex() {
+      DCHECK(list_);
+      const size_t max_index = clamped_max_index();
+      while (index_ < max_index && !list_->observers_[index_])
+        ++index_;
+    }
 
     size_t clamped_max_index() const {
       return std::min(max_index_, list_->observers_.size());
@@ -134,7 +172,8 @@ class ObserverListBase
 
     bool is_end() const { return !list_ || index_ == clamped_max_index(); }
 
-    WeakPtr<ObserverListBase<ObserverType>> list_;
+    WeakPtr<ObserverListBase> list_;
+
     // When initially constructed and each time the iterator is incremented,
     // |index_| is guaranteed to point to a non-null index if the iterator
     // has not reached the end of the ObserverList.
@@ -142,53 +181,75 @@ class ObserverListBase
     size_t max_index_;
   };
 
-  using Iterator = Iter<ObserverListBase<ObserverType>>;
+  using iterator = Iter;
+  using const_iterator = Iter;
 
-  using iterator = Iter<ObserverListBase<ObserverType>>;
-  iterator begin() {
+  const_iterator begin() const {
     // An optimization: do not involve weak pointers for empty list.
-    // Note: can't use ?: operator here due to some MSVC bug (unit tests fail)
-    if (observers_.empty())
-      return iterator();
-    return iterator(this);
+    return observers_.empty() ? const_iterator() : const_iterator(this);
   }
-  iterator end() { return iterator(); }
 
-  using const_iterator = Iter<const ObserverListBase<ObserverType>>;
-  const_iterator begin() const {
-    if (observers_.empty())
-      return const_iterator();
-    return const_iterator(this);
-  }
   const_iterator end() const { return const_iterator(); }
 
   ObserverListBase() : live_iterator_count_(0), type_(NOTIFY_ALL) {}
   explicit ObserverListBase(NotificationType type)
       : live_iterator_count_(0), type_(type) {}
 
-  // Add an observer to the list.  An observer should not be added to
-  // the same list more than once.
-  void AddObserver(ObserverType* obs);
+  // Add an observer to this list. An observer should not be added to the same
+  // list more than once.
+  //
+  // Precondition: obs != nullptr
+  // Precondition: !HasObserver(obs)
+  void AddObserver(ObserverType* obs) {
+    DCHECK(obs);
+    if (HasObserver(obs)) {
+      NOTREACHED() << "Observers can only be added once!";
+      return;
+    }
+    observers_.push_back(obs);
+  }
+
+  // Removes the given observer from this list. Does nothing if this observer is
+  // not in this list.
+  void RemoveObserver(const ObserverType* obs) {
+    DCHECK(obs);
+    const auto it = std::find(observers_.begin(), observers_.end(), obs);
+    if (it == observers_.end())
+      return;
 
-  // Remove an observer from the list if it is in the list.
-  void RemoveObserver(ObserverType* obs);
+    DCHECK_GE(live_iterator_count_, 0);
+    if (live_iterator_count_) {
+      *it = nullptr;
+    } else {
+      observers_.erase(it);
+    }
+  }
 
   // Determine whether a particular observer is in the list.
-  bool HasObserver(const ObserverType* observer) const;
+  bool HasObserver(const ObserverType* obs) const {
+    return ContainsValue(observers_, obs);
+  }
 
-  void Clear();
+  // Removes all the observers from this list.
+  void Clear() {
+    DCHECK_GE(live_iterator_count_, 0);
+    if (live_iterator_count_) {
+      std::fill(observers_.begin(), observers_.end(), nullptr);
+    } else {
+      observers_.clear();
+    }
+  }
 
  protected:
   size_t size() const { return observers_.size(); }
 
-  void Compact();
+  void Compact() {
+    observers_.erase(std::remove(observers_.begin(), observers_.end(), nullptr),
+                     observers_.end());
+  }
 
  private:
-  friend class ObserverListThreadSafe<ObserverType>;
-
-  typedef std::vector<ObserverType*> ListType;
-
-  ListType observers_;
+  std::vector<ObserverType*> observers_;
 
   // Number of active iterators referencing this ObserverListBase.
   //
@@ -198,168 +259,13 @@ class ObserverListBase
 
   const NotificationType type_;
 
-  template <class ContainerType>
-  friend class Iter;
-
   DISALLOW_COPY_AND_ASSIGN(ObserverListBase);
 };
 
-template <class ObserverType>
-template <class ContainerType>
-ObserverListBase<ObserverType>::Iter<ContainerType>::Iter()
-    : index_(0), max_index_(0) {}
-
-template <class ObserverType>
-template <class ContainerType>
-ObserverListBase<ObserverType>::Iter<ContainerType>::Iter(ContainerType* list)
-    : list_(const_cast<ObserverListBase<ObserverType>*>(list)->AsWeakPtr()),
-      index_(0),
-      max_index_(list->type_ == NOTIFY_ALL ? std::numeric_limits<size_t>::max()
-                                           : list->observers_.size()) {
-  EnsureValidIndex();
-  DCHECK(list_);
-  ++list_->live_iterator_count_;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-ObserverListBase<ObserverType>::Iter<ContainerType>::~Iter() {
-  if (!list_)
-    return;
-
-  DCHECK_GT(list_->live_iterator_count_, 0);
-  if (--list_->live_iterator_count_ == 0)
-    list_->Compact();
-}
-
-template <class ObserverType>
-template <class ContainerType>
-bool ObserverListBase<ObserverType>::Iter<ContainerType>::operator==(
-    const Iter& other) const {
-  if (is_end() && other.is_end())
-    return true;
-  return list_.get() == other.list_.get() && index_ == other.index_;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-bool ObserverListBase<ObserverType>::Iter<ContainerType>::operator!=(
-    const Iter& other) const {
-  return !operator==(other);
-}
-
-template <class ObserverType>
-template <class ContainerType>
-typename ObserverListBase<ObserverType>::template Iter<ContainerType>&
-    ObserverListBase<ObserverType>::Iter<ContainerType>::operator++() {
-  if (list_) {
-    ++index_;
-    EnsureValidIndex();
-  }
-  return *this;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-ObserverType* ObserverListBase<ObserverType>::Iter<ContainerType>::operator->()
-    const {
-  ObserverType* current = GetCurrent();
-  DCHECK(current);
-  return current;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-ObserverType& ObserverListBase<ObserverType>::Iter<ContainerType>::operator*()
-    const {
-  ObserverType* current = GetCurrent();
-  DCHECK(current);
-  return *current;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-ObserverType* ObserverListBase<ObserverType>::Iter<ContainerType>::GetCurrent()
-    const {
-  if (!list_)
-    return nullptr;
-  return index_ < clamped_max_index() ? list_->observers_[index_] : nullptr;
-}
-
-template <class ObserverType>
-template <class ContainerType>
-void ObserverListBase<ObserverType>::Iter<ContainerType>::EnsureValidIndex() {
-  if (!list_)
-    return;
-
-  size_t max_index = clamped_max_index();
-  while (index_ < max_index && !list_->observers_[index_])
-    ++index_;
-}
-
-template <class ObserverType>
-void ObserverListBase<ObserverType>::AddObserver(ObserverType* obs) {
-  DCHECK(obs);
-  if (ContainsValue(observers_, obs)) {
-    NOTREACHED() << "Observers can only be added once!";
-    return;
-  }
-  observers_.push_back(obs);
-}
-
-template <class ObserverType>
-void ObserverListBase<ObserverType>::RemoveObserver(ObserverType* obs) {
-  DCHECK(obs);
-  typename ListType::iterator it =
-    std::find(observers_.begin(), observers_.end(), obs);
-  if (it != observers_.end()) {
-    DCHECK_GE(live_iterator_count_, 0);
-    if (live_iterator_count_) {
-      *it = nullptr;
-    } else {
-      observers_.erase(it);
-    }
-  }
-}
-
-template <class ObserverType>
-bool ObserverListBase<ObserverType>::HasObserver(
-    const ObserverType* observer) const {
-  for (size_t i = 0; i < observers_.size(); ++i) {
-    if (observers_[i] == observer)
-      return true;
-  }
-  return false;
-}
-
-template <class ObserverType>
-void ObserverListBase<ObserverType>::Clear() {
-  DCHECK_GE(live_iterator_count_, 0);
-  if (live_iterator_count_) {
-    for (typename ListType::iterator it = observers_.begin();
-      it != observers_.end(); ++it) {
-      *it = nullptr;
-    }
-  } else {
-    observers_.clear();
-  }
-}
-
-template <class ObserverType>
-void ObserverListBase<ObserverType>::Compact() {
-  observers_.erase(std::remove(observers_.begin(), observers_.end(), nullptr),
-                   observers_.end());
-}
-
 template <class ObserverType, bool check_empty = false>
 class ObserverList : public ObserverListBase<ObserverType> {
  public:
-  typedef typename ObserverListBase<ObserverType>::NotificationType
-      NotificationType;
-
-  ObserverList() {}
-  explicit ObserverList(NotificationType type)
-      : ObserverListBase<ObserverType>(type) {}
+  using ObserverListBase<ObserverType>::ObserverListBase;
 
   ~ObserverList() {
     // When check_empty is true, assert that the list is empty on destruction.