[iOS] make GridCommands idempotent.

As a follow-up to crrev/999712, this CL updates the GridCommands protocol to,
where possible, express actions on tabs in the tab grid in terms of item IDs instead
of indexes.

Bug: 986379
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ib92ba83fe37ac789b693d006e0ca60cb665b22b5
Reviewed-on: https://chromium-review.googlesource.com/1000860
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549494}

ios/chrome/browser/ui/tab_grid/grid/grid_commands.h

@@ -12,12 +12,12 @@
 // Tells the receiver to insert a new item at |index|. It is an error to call
 // this method with an |index| greater than the number of items in the model.
 - (void)insertNewItemAtIndex:(NSUInteger)index;
-// Tells the receiver to select the item at |index|. It is an error to call this
-// method with an |index| greater than the largest index in the model.
-- (void)selectItemAtIndex:(NSUInteger)index;
-// Tells the receiver to close the item at |index|. It is an error to call this
-// method with an |index| greater than the largest index in the model.
-- (void)closeItemAtIndex:(NSUInteger)index;
+// Tells the receiver to select the item with identifier |itemID|. If there is
+// no item with that identifier, no change in selection should be made.
+- (void)selectItemWithID:(NSString*)itemID;
+// Tells the receiver to close the item with identifier |itemID|. If there is
+// no item with that identifier, no item is closed.
+- (void)closeItemWithID:(NSString*)itemID;
 // Tells the receiver to close all items.
 - (void)closeAllItems;
 @end

ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.h

@@ -16,14 +16,14 @@
 
 // Protocol used to relay relevant user interactions from a grid UI.
 @protocol GridViewControllerDelegate
-// Tells the delegate that the item at |index| was selected in
+// Tells the delegate that the item with |itemID| was selected in
 // |gridViewController|.
 - (void)gridViewController:(GridViewController*)gridViewController
-      didSelectItemAtIndex:(NSUInteger)index;
-// Tells the delegate that the item at |index| was closed in
+       didSelectItemWithID:(NSString*)itemID;
+// Tells the delegate that the item with |itemID| was closed in
 // |gridViewController|.
 - (void)gridViewController:(GridViewController*)gridViewController
-       didCloseItemAtIndex:(NSUInteger)index;
+        didCloseItemWithID:(NSString*)itemID;
 // Tells the delegate that the the number of items in |gridViewController|
 // changed to |count|.
 - (void)gridViewController:(GridViewController*)gridViewController

ios/chrome/browser/ui/tab_grid/grid/grid_view_controller.mm

@@ -184,15 +184,20 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 
 - (void)collectionView:(UICollectionView*)collectionView
     didSelectItemAtIndexPath:(NSIndexPath*)indexPath {
-  [self.delegate gridViewController:self didSelectItemAtIndex:indexPath.item];
+  NSUInteger index = base::checked_cast<NSUInteger>(indexPath.item);
+  DCHECK_LT(index, self.items.count);
+  NSString* itemID = self.items[index].identifier;
+  [self.delegate gridViewController:self didSelectItemWithID:itemID];
 }
 
 #pragma mark - GridCellDelegate
 
 - (void)closeButtonTappedForCell:(GridCell*)cell {
-  NSInteger index = [self.collectionView indexPathForCell:cell].item;
-  [self.delegate gridViewController:self
-                didCloseItemAtIndex:base::checked_cast<NSUInteger>(index)];
+  NSUInteger index = base::checked_cast<NSUInteger>(
+      [self.collectionView indexPathForCell:cell].item);
+  DCHECK_LT(index, self.items.count);
+  NSString* itemID = self.items[index].identifier;
+  [self.delegate gridViewController:self didCloseItemWithID:itemID];
 }
 
 #pragma mark - GridConsumer

ios/chrome/browser/ui/tab_grid/grid/grid_view_controller_unittest.mm

@@ -38,12 +38,12 @@
   self.itemCount = count;
 }
 - (void)gridViewController:(GridViewController*)gridViewController
-      didSelectItemAtIndex:(NSUInteger)index {
+       didSelectItemWithID:(NSString*)itemID {
   // No-op for unittests. This is only called when a user taps on a cell, not
   // generically when selectedIndex is updated.
 }
 - (void)gridViewController:(GridViewController*)gridViewController
-       didCloseItemAtIndex:(NSUInteger)index {
+        didCloseItemWithID:(NSString*)itemID {
   // No-op for unittests. This is only called when a user taps to close a cell,
   // not generically when items are removed from the data source.
 }

ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm

@@ -54,6 +54,16 @@ NSString* GetActiveTabId(WebStateList* webStateList) {
   return tabHelper->tab_id();
 }
 
+int GetIndexOfTabWithId(WebStateList* webStateList, NSString* identifier) {
+  for (int i = 0; i < webStateList->count(); i++) {
+    web::WebState* webState = webStateList->GetWebStateAt(i);
+    TabIdTabHelper* tabHelper = TabIdTabHelper::FromWebState(webState);
+    if ([tabHelper->tab_id() isEqualToString:identifier])
+      return i;
+  }
+  return -1;
+}
+
 }  // namespace
 
 @interface TabGridMediator ()<CRWWebStateObserver, WebStateListObserving>
@@ -199,12 +209,16 @@ NSString* GetActiveTabId(WebStateList* webStateList) {
   self.webStateList->GetWebStateAt(index)->OpenURL(openParams);
 }
 
-- (void)selectItemAtIndex:(NSUInteger)index {
-  self.webStateList->ActivateWebStateAt(index);
+- (void)selectItemWithID:(NSString*)itemID {
+  int index = GetIndexOfTabWithId(self.webStateList, itemID);
+  if (index >= 0)
+    self.webStateList->ActivateWebStateAt(index);
 }
 
-- (void)closeItemAtIndex:(NSUInteger)index {
-  self.webStateList->CloseWebStateAt(index, WebStateList::CLOSE_USER_ACTION);
+- (void)closeItemWithID:(NSString*)itemID {
+  int index = GetIndexOfTabWithId(self.webStateList, itemID);
+  if (index >= 0)
+    self.webStateList->CloseWebStateAt(index, WebStateList::CLOSE_USER_ACTION);
 }
 
 - (void)closeAllItems {

ios/chrome/browser/ui/tab_grid/tab_grid_mediator_unittest.mm

@@ -174,6 +174,8 @@ TEST_F(TabGridMediatorTest, ConsumerInsertItem) {
 }
 
 // Tests that the consumer is notified when a web state is removed.
+// The selected web state at index 1 is removed. The web state originally
+// at index 2 should be the new selected item.
 TEST_F(TabGridMediatorTest, ConsumerRemoveItem) {
   web_state_list_->CloseWebStateAt(1, WebStateList::CLOSE_NO_FLAGS);
   EXPECT_EQ(2UL, consumer_.items.count);
@@ -191,6 +193,8 @@ TEST_F(TabGridMediatorTest, ConsumerUpdateSelectedItem) {
 }
 
 // Tests that the consumer is notified when a web state is replaced.
+// The selected item is replaced, so the new selected item id should be the
+// id of the new item.
 TEST_F(TabGridMediatorTest, ConsumerReplaceItem) {
   auto new_web_state = std::make_unique<web::TestWebState>();
   TabIdTabHelper::CreateForWebState(new_web_state.get());
@@ -212,18 +216,22 @@ TEST_F(TabGridMediatorTest, ConsumerMoveItem) {
   EXPECT_NSEQ(item2, consumer_.items[1]);
 }
 
-// Tests that the active index is updated when |-selectItemAtIndex:| is called.
+// Tests that the active index is updated when |-selectItemWithID:| is called.
 TEST_F(TabGridMediatorTest, SelectItemCommand) {
   // Previous selected index is 1.
-  [mediator_ selectItemAtIndex:2];
+  NSString* identifier =
+      TabIdTabHelper::FromWebState(web_state_list_->GetWebStateAt(2))->tab_id();
+  [mediator_ selectItemWithID:identifier];
   EXPECT_EQ(2, web_state_list_->active_index());
 }
 
 // Tests that the |web_state_list_| count is decremented when
-// |-closeItemAtIndex:| is called.
+// |-closeItemWithID:| is called.
 TEST_F(TabGridMediatorTest, CloseItemCommand) {
   // Previously there were 3 items.
-  [mediator_ closeItemAtIndex:1];
+  NSString* identifier =
+      TabIdTabHelper::FromWebState(web_state_list_->GetWebStateAt(0))->tab_id();
+  [mediator_ closeItemWithID:identifier];
   EXPECT_EQ(2, web_state_list_->count());
 }
 

ios/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm

@@ -715,21 +715,21 @@ UIAlertController* NotImplementedAlert() {
 #pragma mark - GridViewControllerDelegate
 
 - (void)gridViewController:(GridViewController*)gridViewController
-      didSelectItemAtIndex:(NSUInteger)index {
+       didSelectItemWithID:(NSString*)itemID {
   if (gridViewController == self.regularTabsViewController) {
-    [self.regularTabsDelegate selectItemAtIndex:index];
+    [self.regularTabsDelegate selectItemWithID:itemID];
   } else if (gridViewController == self.incognitoTabsViewController) {
-    [self.incognitoTabsDelegate selectItemAtIndex:index];
+    [self.incognitoTabsDelegate selectItemWithID:itemID];
   }
   [self.tabPresentationDelegate showActiveTabInPage:self.currentPage];
 }
 
 - (void)gridViewController:(GridViewController*)gridViewController
-       didCloseItemAtIndex:(NSUInteger)index {
+        didCloseItemWithID:(NSString*)itemID {
   if (gridViewController == self.regularTabsViewController) {
-    [self.regularTabsDelegate closeItemAtIndex:index];
+    [self.regularTabsDelegate closeItemWithID:itemID];
   } else if (gridViewController == self.incognitoTabsViewController) {
-    [self.incognitoTabsDelegate closeItemAtIndex:index];
+    [self.incognitoTabsDelegate closeItemWithID:itemID];
   }
 }