Remove ObserverList::might_have_observers()? |
||
Issue description
There's a lot of subtle stuff that uses this, and it feels rather fragile:
Example #1:
// If we are removing the final observer for the source, then stop
// observing routes for it.
// might_have_observers() is reliable here on the assumption that this call
// is not inside the ObserverList iteration.
routes_query->observers.RemoveObserver(observer);
if (!routes_query->observers.might_have_observers())
which assumes that this method is never called while the ObserverList is being iterated.
Example #2:
for (auto& observer : animation_observer_list_)
observer.OnAnimationStep(args.frame_time);
if (animation_observer_list_.might_have_observers())
host_->SetNeedsAnimate();
which might be able to just use a local bool to track this.
Example #3:
ListenerKey key(listener->GetName(), listener->GetType());
ListenerMap::iterator observer_list_iterator = listeners_.find(key);
DCHECK(observer_list_iterator != listeners_.end());
DCHECK(observer_list_iterator->second->HasObserver(listener));
observer_list_iterator->second->RemoveObserver(listener);
// Remove the observer list from the map if it is empty
if (!observer_list_iterator->second->might_have_observers()) {
// Schedule the actual removal for later in case the listener removal
// happens while iterating over the observer list.
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::Bind(&MDnsClientImpl::Core::CleanupObserverList,
AsWeakPtr(), key));
}
which has two conflicting aspects:
- might_have_observers() will never return false in the middle of observer iteration
- yet it uses a PostTask() to try to handle this case safely?
Example #4:
void NetLog::UpdateIsCapturing() {
lock_.AssertAcquired();
base::subtle::NoBarrier_Store(&is_capturing_,
observers_.might_have_observers() ? 1 : 0);
}
Which combines far too many fun things into one: locks, atomics, and a potentially inaccurate answer from might_have_observers().
Strawman #1
Just replace all these with observers.begin() != observers.end(). Disadvantage is that this is a bit more expensive, but it's also guaranteed to be accurate. I think the fact that it's slightly more expensive is unlikely to matter in practice?
Strawman #2
Add a callback to ObserverList that is invoked when the ObserverList transitions to/from 0 observers.
Personally, I prefer option #1 here but I'm curious if other people have thoughts.
,
Oct 20 2016
begin == end sgtm
,
Jan 10
Archiving P3s older than 1 year with no owner or component. |
||
►
Sign in to add a comment |
||
Comment 1 by loyso@chromium.org
, Oct 17 2016I vote for #1. bool ObserverList::empty() const { return begin() == end(); }