Remove TabModelObserver's method didMoveTab

This CL:
1- Replaces usage of didMoveTab with didMoveWebState in TabStripController
   and in BrowserViewController
2- Create FakeWebStateListObservingDelegate to be used in tests that
   requires the Obj-C version of WebStateListObserver.
3- Update tab_model_unittest to use the FakeWebStateListObservingDelegate
   for testing.

Change-Id: I8868442244ad3e2933823d3b6f5e1c5fe047db9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614030Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660813}

ios/chrome/browser/tabs/BUILD.gn

@@ -190,6 +190,7 @@ source_set("unit_tests") {
     "//ios/chrome/browser/web_state_list:test_support",
     "//ios/chrome/browser/web_state_list/web_usage_enabler",
     "//ios/chrome/test:test_support",
+    "//ios/chrome/test/fakes",
     "//ios/public/provider/chrome/browser",
     "//ios/public/provider/chrome/browser:test_support",
     "//ios/testing:ocmock_support",

ios/chrome/browser/tabs/tab_model_observer.h

@@ -26,16 +26,6 @@
     didRemoveTab:(Tab*)tab
          atIndex:(NSUInteger)index;
 
-// A tab was moved to the given index.  |fromIndex| is guaranteed to be
-// different than |toIndex| (moves to the same index do not trigger
-// notifications).  This method is only called for moves that change the
-// relative order of tabs (for example, closing the first tab decrements all tab
-// indexes by one, but will not call this observer method).
-- (void)tabModel:(TabModel*)model
-      didMoveTab:(Tab*)tab
-       fromIndex:(NSUInteger)fromIndex
-         toIndex:(NSUInteger)toIndex;
-
 // A Tab was replaced in the model, at the given index.
 - (void)tabModel:(TabModel*)model
     didReplaceTab:(Tab*)oldTab

ios/chrome/browser/tabs/tab_model_observers_bridge.mm

@@ -45,18 +45,6 @@
                   inForeground:activating];
 }
 
-- (void)webStateList:(WebStateList*)webStateList
-     didMoveWebState:(web::WebState*)webState
-           fromIndex:(int)fromIndex
-             toIndex:(int)toIndex {
-  DCHECK_GE(fromIndex, 0);
-  DCHECK_GE(toIndex, 0);
-  [_tabModelObservers tabModel:_tabModel
-                    didMoveTab:LegacyTabHelper::GetTabForWebState(webState)
-                     fromIndex:static_cast<NSUInteger>(fromIndex)
-                       toIndex:static_cast<NSUInteger>(toIndex)];
-}
-
 - (void)webStateList:(WebStateList*)webStateList
     didReplaceWebState:(web::WebState*)oldWebState
           withWebState:(web::WebState*)newWebState

ios/chrome/browser/tabs/tab_model_unittest.mm

@@ -23,13 +23,13 @@
 #import "ios/chrome/browser/tabs/tab.h"
 #import "ios/chrome/browser/tabs/tab_helper_util.h"
 #import "ios/chrome/browser/tabs/tab_model.h"
-#import "ios/chrome/browser/tabs/tab_model_observer.h"
 #import "ios/chrome/browser/tabs/tab_private.h"
 #import "ios/chrome/browser/web/chrome_web_client.h"
 #import "ios/chrome/browser/web_state_list/web_state_list.h"
 #import "ios/chrome/browser/web_state_list/web_state_opener.h"
 #import "ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler.h"
 #import "ios/chrome/browser/web_state_list/web_usage_enabler/web_state_list_web_usage_enabler_factory.h"
+#import "ios/chrome/test/fakes/fake_web_state_list_observing_delegate.h"
 #include "ios/chrome/test/ios_chrome_scoped_testing_chrome_browser_state_manager.h"
 #include "ios/web/common/features.h"
 #import "ios/web/navigation/navigation_manager_impl.h"
@@ -51,27 +51,6 @@
 #error "This file requires ARC support."
 #endif
 
-// Defines a TabModelObserver for use in unittests.  This class can be used to
-// test if an observer method was called or not.
-@interface TabModelObserverPong : NSObject<TabModelObserver> {
-  // TODO(crbug.com/661989): Add tests for the other observer methods.
-  BOOL tabMovedWasCalled_;
-}
-@property(nonatomic, assign) BOOL tabMovedWasCalled;
-@end
-
-@implementation TabModelObserverPong
-@synthesize tabMovedWasCalled = tabMovedWasCalled_;
-
-- (void)tabModel:(TabModel*)model
-      didMoveTab:(Tab*)tab
-       fromIndex:(NSUInteger)fromIndex
-         toIndex:(NSUInteger)toIndex {
-  tabMovedWasCalled_ = YES;
-}
-
-@end
-
 namespace {
 
 const char kURL1[] = "https://www.some.url.com";
@@ -631,58 +610,58 @@ TEST_P(TabModelTest, MoveTabs) {
   ASSERT_NSEQ(tab2, [tab_model_ tabAtIndex:2]);
 
   // Check that observer methods are called.
-  TabModelObserverPong* tab_model_observer;
-  tab_model_observer = [[TabModelObserverPong alloc] init];
-  [tab_model_ addObserver:tab_model_observer];
+  FakeWebStateListObservingDelegate* web_state_list_observer =
+      [[FakeWebStateListObservingDelegate alloc] init];
+  [web_state_list_observer observeWebStateList:tab_model_.webStateList];
 
   // Move a tab from index 1 to index 0 (move tab left by one).
-  [tab_model_observer setTabMovedWasCalled:NO];
+  [web_state_list_observer setDidMoveWebStateWasCalled:NO];
   [tab_model_ moveTab:[tab_model_ tabAtIndex:1] toIndex:0];
   ASSERT_EQ(3U, [tab_model_ count]);
   EXPECT_NSEQ(tab1, [tab_model_ tabAtIndex:0]);
   EXPECT_NSEQ(tab0, [tab_model_ tabAtIndex:1]);
   EXPECT_NSEQ(tab2, [tab_model_ tabAtIndex:2]);
-  EXPECT_TRUE([tab_model_observer tabMovedWasCalled]);
+  EXPECT_TRUE([web_state_list_observer didMoveWebStateWasCalled]);
 
   // Move a tab from index 1 to index 2 (move tab right by one).
-  [tab_model_observer setTabMovedWasCalled:NO];
+  [web_state_list_observer setDidMoveWebStateWasCalled:NO];
   [tab_model_ moveTab:[tab_model_ tabAtIndex:1] toIndex:2];
   ASSERT_EQ(3U, [tab_model_ count]);
   EXPECT_NSEQ(tab1, [tab_model_ tabAtIndex:0]);
   EXPECT_NSEQ(tab2, [tab_model_ tabAtIndex:1]);
   EXPECT_NSEQ(tab0, [tab_model_ tabAtIndex:2]);
-  EXPECT_TRUE([tab_model_observer tabMovedWasCalled]);
+  EXPECT_TRUE([web_state_list_observer didMoveWebStateWasCalled]);
 
   // Move a tab from index 0 to index 2 (move tab right by more than one).
-  [tab_model_observer setTabMovedWasCalled:NO];
+  [web_state_list_observer setDidMoveWebStateWasCalled:NO];
   [tab_model_ moveTab:[tab_model_ tabAtIndex:0] toIndex:2];
   ASSERT_EQ(3U, [tab_model_ count]);
   EXPECT_NSEQ(tab2, [tab_model_ tabAtIndex:0]);
   EXPECT_NSEQ(tab0, [tab_model_ tabAtIndex:1]);
   EXPECT_NSEQ(tab1, [tab_model_ tabAtIndex:2]);
-  EXPECT_TRUE([tab_model_observer tabMovedWasCalled]);
+  EXPECT_TRUE([web_state_list_observer didMoveWebStateWasCalled]);
 
   // Move a tab from index 2 to index 0 (move tab left by more than one).
-  [tab_model_observer setTabMovedWasCalled:NO];
+  [web_state_list_observer setDidMoveWebStateWasCalled:NO];
   [tab_model_ moveTab:[tab_model_ tabAtIndex:2] toIndex:0];
   ASSERT_EQ(3U, [tab_model_ count]);
   EXPECT_NSEQ(tab1, [tab_model_ tabAtIndex:0]);
   EXPECT_NSEQ(tab2, [tab_model_ tabAtIndex:1]);
   EXPECT_NSEQ(tab0, [tab_model_ tabAtIndex:2]);
-  EXPECT_TRUE([tab_model_observer tabMovedWasCalled]);
+  EXPECT_TRUE([web_state_list_observer didMoveWebStateWasCalled]);
 
   // Move a tab from index 2 to index 2 (move tab to the same index).
-  [tab_model_observer setTabMovedWasCalled:NO];
+  [web_state_list_observer setDidMoveWebStateWasCalled:NO];
   [tab_model_ moveTab:[tab_model_ tabAtIndex:2] toIndex:2];
   ASSERT_EQ(3U, [tab_model_ count]);
   EXPECT_NSEQ(tab1, [tab_model_ tabAtIndex:0]);
   EXPECT_NSEQ(tab2, [tab_model_ tabAtIndex:1]);
   EXPECT_NSEQ(tab0, [tab_model_ tabAtIndex:2]);
-  EXPECT_FALSE([tab_model_observer tabMovedWasCalled]);
+  EXPECT_FALSE([web_state_list_observer didMoveWebStateWasCalled]);
 
   // TabModel asserts that there are no observer when it is deallocated,
   // so remove the observer before the end of the method.
-  [tab_model_ removeObserver:tab_model_observer];
+  [web_state_list_observer stopObservingWebStateList:tab_model_.webStateList];
 }
 
 TEST_P(TabModelTest, TabCreatedOnInsertion) {

ios/chrome/browser/ui/tabs/tab_strip_controller.mm

@@ -972,6 +972,21 @@ UIColor* BackgroundColor() {
   [_tabStripView setNeedsLayout];
 }
 
+// Observer method. |webState| moved in |webStateList|.
+- (void)webStateList:(WebStateList*)webStateList
+     didMoveWebState:(web::WebState*)webState
+           fromIndex:(int)fromIndex
+             toIndex:(int)toIndex {
+  DCHECK(!_isReordering);
+
+  // Reorder the objects in _tabArray to keep in sync with the model ordering.
+  NSUInteger arrayIndex = [self indexForModelIndex:fromIndex];
+  TabView* view = [_tabArray objectAtIndex:arrayIndex];
+  [_tabArray removeObject:view];
+  [_tabArray insertObject:view atIndex:toIndex];
+  [self setNeedsLayoutWithAnimation];
+}
+
 // Observer method, |webState| removed from |webStateList|.
 - (void)webStateList:(WebStateList*)webStateList
     didDetachWebState:(web::WebState*)webState
@@ -1030,21 +1045,6 @@ UIColor* BackgroundColor() {
   [self updateTabCount];
 }
 
-// Observer method.
-- (void)tabModel:(TabModel*)model
-      didMoveTab:(Tab*)tab
-       fromIndex:(NSUInteger)fromIndex
-         toIndex:(NSUInteger)toIndex {
-  DCHECK(!_isReordering);
-
-  // Reorder the objects in _tabArray to keep in sync with the model ordering.
-  NSUInteger arrayIndex = [self indexForModelIndex:fromIndex];
-  TabView* view = [_tabArray objectAtIndex:arrayIndex];
-  [_tabArray removeObject:view];
-  [_tabArray insertObject:view atIndex:toIndex];
-  [self setNeedsLayoutWithAnimation];
-}
-
 // Observer method.
 - (void)tabModel:(TabModel*)model didChangeTab:(Tab*)tab {
   NSUInteger modelIndex = [_tabModel indexOfTab:tab];

ios/chrome/test/fakes/BUILD.gn

@@ -29,6 +29,8 @@ source_set("fakes") {
     "fake_store_kit_launcher.mm",
     "fake_ui_view_controller.h",
     "fake_ui_view_controller.mm",
+    "fake_web_state_list_observing_delegate.h",
+    "fake_web_state_list_observing_delegate.mm",
   ]
 
   deps = [
@@ -42,6 +44,7 @@ source_set("fakes") {
     "//ios/chrome/browser/ui/overscroll_actions",
     "//ios/chrome/browser/ui/presenters",
     "//ios/chrome/browser/web:web_internal",
+    "//ios/chrome/browser/web_state_list",
     "//ios/web/public",
     "//ios/web/public/download",
   ]

ios/chrome/test/fakes/fake_web_state_list_observing_delegate.h

+// Copyright 2019 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.
+
+#ifndef IOS_CHROME_TEST_FAKES_FAKE_WEB_STATE_LIST_OBSERVING_DELEGATE_H_
+#define IOS_CHROME_TEST_FAKES_FAKE_WEB_STATE_LIST_OBSERVING_DELEGATE_H_
+
+#import <Foundation/Foundation.h>
+
+#import "ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h"
+
+class WebStateList;
+
+// Defines a WebStateListObserving for use in unittests. Used to test if an
+// observer method was called or not.
+@interface FakeWebStateListObservingDelegate : NSObject <WebStateListObserving>
+@property(nonatomic, assign) BOOL didMoveWebStateWasCalled;
+- (void)observeWebStateList:(WebStateList*)webStateList;
+- (void)stopObservingWebStateList:(WebStateList*)webStateList;
+
+@end
+
+#endif  // IOS_CHROME_TEST_FAKES_FAKE_WEB_STATE_LIST_OBSERVING_DELEGATE_H_

ios/chrome/test/fakes/fake_web_state_list_observing_delegate.mm

+// Copyright 2019 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 "ios/chrome/test/fakes/fake_web_state_list_observing_delegate.h"
+
+#import "ios/chrome/browser/web_state_list/web_state_list.h"
+#import "ios/web/public/web_state/web_state.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+@implementation FakeWebStateListObservingDelegate {
+  // Bridges C++ WebStateListObserver methods to this
+  // WebStateListObservingDelegate.
+  std::unique_ptr<WebStateListObserver> _webStateListObserver;
+}
+
+@synthesize didMoveWebStateWasCalled = _didMoveWebStateWasCalled;
+
+- (instancetype)init {
+  if ((self = [super init])) {
+    _webStateListObserver = std::make_unique<WebStateListObserverBridge>(self);
+  }
+  return self;
+}
+
+- (void)observeWebStateList:(WebStateList*)webStateList {
+  webStateList->AddObserver(_webStateListObserver.get());
+}
+
+- (void)stopObservingWebStateList:(WebStateList*)webStateList {
+  webStateList->RemoveObserver(_webStateListObserver.get());
+}
+
+#pragma mark - WebStateListObserving protocol
+- (void)webStateList:(WebStateList*)webStateList
+     didMoveWebState:(web::WebState*)webState
+           fromIndex:(int)fromIndex
+             toIndex:(int)toIndex {
+  _didMoveWebStateWasCalled = YES;
+}
+@end