Fixing an edge case in CRWBrowsingDataStore

ios/web/crw_browsing_data_store_unittest.mm

Index: ios/web/crw_browsing_data_store_unittest.mm
diff --git a/ios/web/crw_browsing_data_store_unittest.mm b/ios/web/crw_browsing_data_store_unittest.mm
index 647399753da15e7166b0cdbc98ab2d9904702c5d..0472c2f650d01f1ef5b234f4a8509c15f9280d0f 100644
--- a/ios/web/crw_browsing_data_store_unittest.mm
+++ b/ios/web/crw_browsing_data_store_unittest.mm
@@ -4,6 +4,7 @@
 
 #import "ios/web/public/crw_browsing_data_store.h"
 
+#include "base/logging.h"
 #import "base/mac/scoped_nsobject.h"
 #include "base/memory/scoped_ptr.h"
 #import "base/test/ios/wait_util.h"
@@ -14,6 +15,58 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
+// An observer to observe the |mode| key changes to a CRWBrowsingDataStore.
+// Used for testing purposes.
+@interface CRWTestBrowsingDataStoreObserver : NSObject
+// Designated init. |browsingDataStore| cannot be null.
+- (instancetype)initWithBrowsingDataStore:
+        (CRWBrowsingDataStore*)browsingDataStore NS_DESIGNATED_INITIALIZER;
+// The number of times that the mode of the underlying CRWBrowsingDataStore
+// changed.
+@property(nonatomic, assign) NSUInteger numberOfTimesModeValueChanged;
+@end
+
+@implementation CRWTestBrowsingDataStoreObserver {
+  // The underlying CRWBrowsingDataStore.
+  __weak CRWBrowsingDataStore* _browsingDataStore;
+  // The last observer mode of the |_browsingDataStore;|
+  CRWBrowsingDataStoreMode _lastObservedMode;
+}
+
+@synthesize numberOfTimesModeValueChanged = _numberOfTimesModeValueChanged;

[Eugene But (OOO till 7-30), 2015/05/29 21:14:26]

s/numberOfTimesModeValueChanged/modeChangeCount

+
+- (instancetype)initWithBrowsingDataStore:
+        (CRWBrowsingDataStore*)browsingDataStore {
+  self = [super init];
+  if (self) {
+    DCHECK(browsingDataStore);
+    [browsingDataStore addObserver:self
+                        forKeyPath:@"mode"
+                           options:0
+                           context:nil];
+    _browsingDataStore = browsingDataStore;
+    _lastObservedMode = [_browsingDataStore mode];

[Eugene But (OOO till 7-30), 2015/05/29 21:14:26]

Is this used anywhere else?

[shreyasv1, 2015/05/29 22:22:57]

Good catch. Removed.

+  }
+  return self;
+}
+
+- (void)observeValueForKeyPath:(NSString*)keyPath
+                      ofObject:(id)object
+                        change:(NSDictionary*)change
+                       context:(void*)context {
+  DCHECK([keyPath isEqual:@"mode"]);
+  DCHECK_EQ(_browsingDataStore, object);
+
+  ++self.numberOfTimesModeValueChanged;
+}
+
+- (void)dealloc {
+  [_browsingDataStore removeObserver:self forKeyPath:@"mode"];
+  [super dealloc];
+}
+
+@end
+
 namespace web {
 namespace {
 
@@ -56,28 +109,33 @@ TEST_F(BrowsingDataStoreTest, InitialModeAndNoPendingOperations) {
 // Tests that CRWBrowsingDataStore handles several consecutive calls to
 // |makeActive| and |makeInactive| correctly.
 TEST_F(BrowsingDataStoreTest, MakeActiveAndInactiveOperations) {
-  ProceduralBlock makeActiveCallback = ^{
-    ASSERT_TRUE([NSThread isMainThread]);
-    CRWBrowsingDataStoreMode mode = [browsing_data_store_ mode];
-    EXPECT_TRUE((mode == ACTIVE) || (mode == SYNCHRONIZING));
-  };
-  ProceduralBlock makeInactiveCallback = ^{
+  base::scoped_nsobject<CRWTestBrowsingDataStoreObserver> observer(
+      [[CRWTestBrowsingDataStoreObserver alloc]
+          initWithBrowsingDataStore:browsing_data_store_]);
+  EXPECT_EQ(0U, [observer numberOfTimesModeValueChanged]);
+
+  id unsucessfullCallback = ^(BOOL success) {
     ASSERT_TRUE([NSThread isMainThread]);
     CRWBrowsingDataStoreMode mode = [browsing_data_store_ mode];
-    EXPECT_TRUE((mode == INACTIVE) || (mode == SYNCHRONIZING));
+    EXPECT_FALSE(success);
+    EXPECT_EQ(SYNCHRONIZING, mode);
   };
-  [browsing_data_store_ makeActiveWithCompletionHandler:makeActiveCallback];
+  [browsing_data_store_ makeActiveWithCompletionHandler:unsucessfullCallback];
   EXPECT_EQ(SYNCHRONIZING, [browsing_data_store_ mode]);
+  EXPECT_EQ(1U, [observer numberOfTimesModeValueChanged]);
 
-  [browsing_data_store_ makeInactiveWithCompletionHandler:makeInactiveCallback];
+  [browsing_data_store_ makeInactiveWithCompletionHandler:unsucessfullCallback];
   EXPECT_EQ(SYNCHRONIZING, [browsing_data_store_ mode]);
 
-  [browsing_data_store_ makeActiveWithCompletionHandler:makeActiveCallback];
+  [browsing_data_store_ makeActiveWithCompletionHandler:unsucessfullCallback];
   EXPECT_EQ(SYNCHRONIZING, [browsing_data_store_ mode]);
 
   __block BOOL block_was_called = NO;
-  [browsing_data_store_ makeInactiveWithCompletionHandler:^{
-    makeInactiveCallback();
+  [browsing_data_store_ makeInactiveWithCompletionHandler:^(BOOL success) {
+    ASSERT_TRUE([NSThread isMainThread]);
+    CRWBrowsingDataStoreMode mode = [browsing_data_store_ mode];
+    EXPECT_TRUE(success);
+    EXPECT_EQ(INACTIVE, mode);
     block_was_called = YES;
   }];
   EXPECT_EQ(SYNCHRONIZING, [browsing_data_store_ mode]);
@@ -85,6 +143,9 @@ TEST_F(BrowsingDataStoreTest, MakeActiveAndInactiveOperations) {
   base::test::ios::WaitUntilCondition(^bool{
     return block_was_called;
   });
+
+  EXPECT_EQ(INACTIVE, [browsing_data_store_ mode]);
+  EXPECT_EQ(2U, [observer numberOfTimesModeValueChanged]);
 }
 
 // Tests that CRWBrowsingDataStore correctly handles |removeDataOfTypes:| call.