.. or should it just be defined to be `false`.

Chrome Version       : m70


Prior to r584694, the following was the behavior of ObserverList::HasObserver(nullptr)

 for (auto& o: observer_list_) {
   ASSERT_FALSE(observer_list_.HasObserver(nullptr));
   observer_list_.RemoveObserver(&o);
   ASSERT_TRUE(observer_list.HasObserver(nullptr));
 }
 ASSERT_FALSE(observer_list_.HasObserver(nullptr);

That's "weird", and would confuse the logic used for HasObserver.


We tried

 bool HasObserver(ObserverType* obs) {
   DCHECK(obs);
   ...
 }

However, existing tests _are_ passing nullptr to HasObserver. (and likely Release code as well).

See https://chromium-review.googlesource.com/c/chromium/src/+/1053338/40/base/observer_list_internal.h#71



Instead, we went with

 bool HasObserver(ObserverType* obs) {
   if (obs)
     return false;
   ...
 }

this makes the behavior less weird:

 for (auto& o: observer_list_) {
   ASSERT_FALSE(observer_list_.HasObserver(nullptr));
   observer_list_.RemoveObserver(&o);
   ASSERT_FALSE(observer_list.HasObserver(nullptr));
 }
 ASSERT_FALSE(observer_list_.HasObserver(nullptr);



But, perhaps callers of HasObserver should be responsible for whether or not they pass `nullptr`. For example, it is already prohibited to pass nullptr to Add/RemoveObserver. For now, it is allowed for HasObserver.

Performance-wise it probably doesn't matter. Calls to HasObserver are not common, and often the argument is `this`, which an optimizing compiler should assume to be non-null and elide the `return false` branch.