CRBProtocolObservers can now be mutated while forwarding methods.

base/ios/crb_protocol_observers_unittest.mm

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "base/ios/crb_protocol_observers.h"
 #include "base/ios/weak_nsobject.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/mac/scoped_nsobject.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gtest_mac.h"
 #include "testing/platform_test.h"
 
 @protocol TestObserver
 
 @required
 - (void)requiredMethod;
 - (void)reset;
 
 @optional
 - (void)optionalMethod;
+- (void)mutateByAddingObserver:(id<TestObserver>)observer;
+- (void)mutateByRemovingObserver:(id<TestObserver>)observer;
 
 @end
 
 // Implements only the required methods in the TestObserver protocol.
 @interface TestPartialObserver : NSObject<TestObserver>
 @property(nonatomic, readonly) BOOL requiredMethodInvoked;
 @end
 
 // Implements all the methods in the TestObserver protocol.
 @interface TestCompleteObserver : TestPartialObserver<TestObserver>
 @property(nonatomic, readonly) BOOL optionalMethodInvoked;
 @end
 
+@interface TestMutateObserver : TestCompleteObserver
+
+- (instancetype)initWithObserver:(CRBProtocolObservers*)observer
+    NS_DESIGNATED_INITIALIZER;
+
+@end
+
 namespace {
 
 class CRBProtocolObserversTest : public PlatformTest {
  public:
   CRBProtocolObserversTest() {}
 
  protected:
   void SetUp() override {
     PlatformTest::SetUp();
 
     observers_.reset([[CRBProtocolObservers observersWithProtocol:
         @protocol(TestObserver)] retain]);
 
     partial_observer_.reset([[TestPartialObserver alloc] init]);
     EXPECT_FALSE([partial_observer_ requiredMethodInvoked]);
 
     complete_observer_.reset([[TestCompleteObserver alloc] init]);
     EXPECT_FALSE([complete_observer_ requiredMethodInvoked]);
     EXPECT_FALSE([complete_observer_ optionalMethodInvoked]);
+
+    mutate_observer_.reset(
+        [[TestMutateObserver alloc] initWithObserver:observers_.get()]);
+    EXPECT_FALSE([mutate_observer_ requiredMethodInvoked]);
   }
 
   base::scoped_nsobject<id> observers_;
   base::scoped_nsobject<TestPartialObserver> partial_observer_;
   base::scoped_nsobject<TestCompleteObserver> complete_observer_;
+  base::scoped_nsobject<TestMutateObserver> mutate_observer_;
 };
 
 // Verifies basic functionality of -[CRBProtocolObservers addObserver:] and
 // -[CRBProtocolObservers removeObserver:].
 TEST_F(CRBProtocolObserversTest, AddRemoveObserver) {
   // Add an observer and verify that the CRBProtocolObservers instance forwards
   // an invocation to it.
   [observers_ addObserver:partial_observer_];
   [observers_ requiredMethod];
   EXPECT_TRUE([partial_observer_ requiredMethodInvoked]);
@@ skipping 44 matching lines @@
     // autoreleased array that holds all the observers.
     base::mac::ScopedNSAutoreleasePool pool;
     [observers_ requiredMethod];
     EXPECT_TRUE([partial_observer_ requiredMethodInvoked]);
   }
 
   partial_observer_.reset();
   EXPECT_FALSE(weak_observer.get());
 }
 
+// Verifies that - [CRBProtocolObservers addObserver:] and
+// - [CRBProtocolObservers removeObserver:] can be called while methods are
+// being forwarded.
+TEST_F(CRBProtocolObserversTest, MutateObservers) {
+  // Indirectly add an observer while forwarding an observer method.
+  [observers_ addObserver:mutate_observer_];
+
+  [observers_ mutateByAddingObserver:partial_observer_];
+  EXPECT_FALSE([partial_observer_ requiredMethodInvoked]);
+
+  // Check that methods are correctly forwared to the indirectly added observer.
+  [mutate_observer_ reset];
+  [observers_ requiredMethod];
+  EXPECT_TRUE([mutate_observer_ requiredMethodInvoked]);
+  EXPECT_TRUE([partial_observer_ requiredMethodInvoked]);
+
+  [mutate_observer_ reset];
+  [partial_observer_ reset];
+
+  // Indirectly remove an observer while forwarding an observer method.
+  [observers_ mutateByRemovingObserver:partial_observer_];
+
+  // Check that method is not forwared to the indirectly removed observer.
+  [observers_ requiredMethod];
+  EXPECT_TRUE([mutate_observer_ requiredMethodInvoked]);
+  EXPECT_FALSE([partial_observer_ requiredMethodInvoked]);
+}
+

[droger, 2015/06/04 10:04:23]

We should also probably test the case where _invocation_depth is > 1.

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

I added a test for the nested case.
I also added another test for the case of an observer removing itself while
being notified.

 }  // namespace
 
 @implementation TestPartialObserver {
   BOOL _requiredMethodInvoked;
 }
 
 - (BOOL)requiredMethodInvoked {
   return _requiredMethodInvoked;
 }
 
@@ skipping 18 matching lines @@
 - (void)optionalMethod {
   _optionalMethodInvoked = YES;
 }
 
 - (void)reset {
   [super reset];
   _optionalMethodInvoked = NO;
 }
 
 @end
+
+@implementation TestMutateObserver {
+  __weak CRBProtocolObservers* _observers;
+}
+
+- (instancetype)initWithObserver:(CRBProtocolObservers*)observers {
+  self = [super init];
+  if (self) {
+    _observers = observers;
+  }
+  return self;
+}
+
+- (void)mutateByAddingObserver:(id<TestObserver>)observer {
+  [_observers addObserver:observer];
+}
+
+- (void)mutateByRemovingObserver:(id<TestObserver>)observer {
+  [_observers removeObserver:observer];
+}
+
+@end