CRBProtocolObservers can now be mutated while forwarding methods.

base/ios/crb_protocol_observers.mm

Index: base/ios/crb_protocol_observers.mm
diff --git a/base/ios/crb_protocol_observers.mm b/base/ios/crb_protocol_observers.mm
index ee9e23fcb4e255c29ff0a33590027a25360d9d0e..60fe5f9f48951eab147026caf6d5e9f2b79a7b8c 100644
--- a/base/ios/crb_protocol_observers.mm
+++ b/base/ios/crb_protocol_observers.mm
@@ -5,10 +5,60 @@
 #import "base/ios/crb_protocol_observers.h"
 
 #include <objc/runtime.h>
+#include <vector>
 
 #include "base/logging.h"
 #include "base/mac/scoped_nsobject.h"
 
+@interface CRBProtocolObservers () {
+  base::scoped_nsobject<Protocol> _protocol;
+ @public
+  std::vector<__weak id> _observers;

[sdefresne, 2015/06/04 10:45:34]

According to
https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transition...,
I think it should be __unsafe_unretained:

- __strong is the default. An object remains “alive” as long as there is a
strong pointer to it.
- __weak specifies a reference that does not keep the referenced object alive. A
weak reference is set to nil when there are no strong references to the object.
- __unsafe_unretained specifies a reference that does not keep the referenced
object alive and is not set to nil when there are no strong references to the
object. If the object it references is deallocated, the pointer is left
dangling.

[jbbegue, 2015/06/04 12:13:03]

Done.

+  int _invocation_depth;

[sdefresne, 2015/06/04 10:18:34]

_invocationDepth

[jbbegue, 2015/06/04 12:13:02]

Done.

+}
+
+- (void)compact;
+
+@end
+
+namespace {
+
+class Iterator {
+ public:
+  explicit Iterator(CRBProtocolObservers* protocolObservers);
+  ~Iterator();
+  id GetNext();
+
+ private:
+  CRBProtocolObservers* protocolObservers_ = nullptr;

[sdefresne, 2015/06/04 10:18:34]

protocol_observers_

[jbbegue, 2015/06/04 12:13:03]

Done.

+  size_t index_ = 0;
+  size_t max_index_;
+};
+
+Iterator::Iterator(CRBProtocolObservers* protocolObservers)
+    : protocolObservers_(protocolObservers),
+      index_(0),
+      max_index_(protocolObservers->_observers.size()) {
+  ++protocolObservers->_invocation_depth;
+}
+
+Iterator::~Iterator() {
+  if (protocolObservers_ && --protocolObservers_->_invocation_depth == 0)
+    [protocolObservers_ compact];
+}
+
+id Iterator::GetNext() {
+  if (!protocolObservers_)
+    return nil;
+  auto& observers = protocolObservers_->_observers;
+  // Skip null elements.
+  size_t max_index = std::min(max_index_, observers.size());
+  while (index_ < max_index && !observers[index_])
+    ++index_;
+  return index_ < max_index ? observers[index_++] : nullptr;

[sdefresne, 2015/06/04 10:18:34]

s/nullptr/nil/

[jbbegue, 2015/06/04 12:13:03]

Done.

+}
+}
+
 @interface CRBProtocolObservers ()
 
 // Designated initializer.
@@ -16,12 +66,9 @@
 
 @end
 
-@implementation CRBProtocolObservers {
-  base::scoped_nsobject<Protocol> _protocol;
-  base::scoped_nsobject<NSHashTable> _observers;
-}
+@implementation CRBProtocolObservers
 
-+ (CRBProtocolObservers*)observersWithProtocol:(Protocol*)protocol {
++ (instancetype)observersWithProtocol:(Protocol*)protocol {
   return [[[self alloc] initWithProtocol:protocol] autorelease];
 }
 
@@ -34,7 +81,6 @@
   self = [super init];
   if (self) {
     _protocol.reset([protocol retain]);
-    _observers.reset([[NSHashTable weakObjectsHashTable] retain]);
   }
   return self;
 }
@@ -44,12 +90,29 @@
 }
 
 - (void)addObserver:(id)observer {
+  DCHECK(observer);
   DCHECK([observer conformsToProtocol:self.protocol]);
-  [_observers addObject:observer];
+
+  if (std::find(_observers.begin(), _observers.end(), observer) !=
+      _observers.end())
+    return;
+
+  _observers.push_back(observer);
 }
 
 - (void)removeObserver:(id)observer {
-  [_observers removeObject:observer];
+  DCHECK(observer);
+  auto it = std::find(_observers.begin(), _observers.end(), observer);
+  if (it != _observers.end()) {
+    if (_invocation_depth)
+      *it = nullptr;

[sdefresne, 2015/06/04 10:18:34]

s/nullptr/nil/

[jbbegue, 2015/06/04 12:13:03]

Done.

+    else
+      _observers.erase(it);
+  }
+}
+
+- (BOOL)empty {
+  return _observers.empty();
 }
 
 #pragma mark - NSObject
@@ -80,9 +143,13 @@
 }
 
 - (void)forwardInvocation:(NSInvocation*)invocation {
+  DCHECK(invocation);
+  if (_observers.empty())
+    return;
   SEL selector = [invocation selector];
-  base::scoped_nsobject<NSArray> observers([[_observers allObjects] retain]);
-  for (id observer in observers.get()) {
+  Iterator it(self);
+  id observer;
+  while ((observer = it.GetNext()) != nullptr) {

[sdefresne, 2015/06/04 10:18:34]

s/nullptr/ or even remove the test

[jbbegue, 2015/06/04 12:13:03]

Done.

     if ([observer respondsToSelector:selector])
       [invocation invokeWithTarget:observer];
   }
@@ -90,10 +157,19 @@
 
 - (void)executeOnObservers:(ExecutionWithObserverBlock)callback {
   DCHECK(callback);
-  base::scoped_nsobject<NSArray> observers([[_observers allObjects] retain]);
-  for (id observer in observers.get()) {
+  if (_observers.empty())
+    return;
+  Iterator it(self);
+  id observer;
+  while ((observer = it.GetNext()) != nullptr)

[sdefresne, 2015/06/04 10:18:34]

s/nullptr/ or even remove the test

[jbbegue, 2015/06/04 12:13:02]

Done.

     callback(observer);
-  }
+}
+
+#pragma mark - Private
+
+- (void)compact {
+  _observers.erase(std::remove(_observers.begin(), _observers.end(), nullptr),

[sdefresne, 2015/06/04 10:18:34]

s/nullptr/nil

[jbbegue, 2015/06/04 12:13:03]

Done.

+                   _observers.end());
 }
 
 @end