[ios] Active web state observer in tab collection.

ios/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.mm

 // Copyright 2017 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/clean/chrome/browser/ui/tab_collection/tab_collection_view_controller.h"
 
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
 #import "ios/clean/chrome/browser/ui/tab_collection/tab_collection_item.h"
 #import "ios/clean/chrome/browser/ui/tab_collection/tab_collection_tab_cell.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
+namespace {
+const int kInvalidIndex = -1;
+}
+
 @interface TabCollectionViewController ()<UICollectionViewDelegate,
                                           SessionCellDelegate>
 @property(nonatomic, readwrite) UICollectionView* tabs;
 @property(nonatomic, readwrite) NSMutableArray<TabCollectionItem*>* items;
 @property(nonatomic, assign) int selectedIndex;
 @end
 
 @implementation TabCollectionViewController
 @synthesize tabs = _tabs;
 @synthesize items = _items;
@@ skipping 15 matching lines @@
   self.tabs.delegate = self;
   [self.tabs registerClass:[TabCollectionTabCell class]
       forCellWithReuseIdentifier:[TabCollectionTabCell identifier]];
 
   [NSLayoutConstraint activateConstraints:@[
     [self.tabs.topAnchor constraintEqualToAnchor:self.view.topAnchor],
     [self.tabs.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor],
     [self.tabs.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
     [self.tabs.trailingAnchor constraintEqualToAnchor:self.view.trailingAnchor],
   ]];
-
-  [self selectItemAtIndex:self.selectedIndex];
 }
 
 - (UIStatusBarStyle)preferredStatusBarStyle {
   return UIStatusBarStyleLightContent;
 }
 
 #pragma mark - Required subclass override
 
 - (UICollectionViewLayout*)collectionViewLayout {
   NOTREACHED() << "You must override "
@@ skipping 33 matching lines @@
           dequeueReusableCellWithReuseIdentifier:[TabCollectionTabCell
                                                      identifier]
                                     forIndexPath:indexPath]);
   cell.delegate = self;
   [cell setSessionType:TabSwitcherSessionType::REGULAR_SESSION];
   DCHECK_LE(indexPath.item, INT_MAX);
   int index = static_cast<int>(indexPath.item);
   [cell setAppearanceForTabTitle:self.items[index].title
                          favicon:nil
                         cellSize:CGSizeZero];
+  [cell setSelected:(index == self.selectedIndex)];
   return cell;
 }
 
 #pragma mark - UICollectionViewDelegate methods
 
 - (BOOL)collectionView:(UICollectionView*)collectionView
     shouldSelectItemAtIndexPath:(NSIndexPath*)indexPath {
   // Prevent user selection of items.
   return NO;
 }
@@ skipping 15 matching lines @@
 - (void)deleteButtonPressedForCell:(UICollectionViewCell*)cell {
   NSInteger item = [[self.tabs indexPathForCell:cell] item];
   DCHECK_LE(item, INT_MAX);
   int index = static_cast<int>(item);
   [self closeTabAtIndex:index];
 }
 
 #pragma mark - TabCollectionConsumer methods
 
 - (void)insertItem:(TabCollectionItem*)item atIndex:(int)index {
+  DCHECK_GE(index, 0);
   DCHECK_LE(static_cast<NSUInteger>(index), self.items.count);
+  if (self.selectedIndex >= index) {
+    self.selectedIndex++;

[rohitrao (ping after 24h), 2017/05/26 12:44:01]

Is this VC independently trying to keep track of how the active tab index
changes when tabs are added and removed?  I don't think we should have this kind
of logic here -- it'll be too easy to get out of sync if we change the
corresponding logic in WebStateList.

Can we push the active tab index into the consumer instead?

[edchin, 2017/05/26 18:09:00]

The VC will get into an internally inconsistent state without this code. 
For example, a new cell is inserted by the end of this method. 
The highlighted cell was previously cell 5, but now it is cell 6. 
But the selected index was not updated, so this VC thinks it should
be highlighting cell 5. You have inconsistent state at the end of this
method.

We could pass the active index along with this method. (perhaps
that's what you were suggesting all along)

+  }
   [self.items insertObject:item atIndex:index];
   [self.tabs insertItemsAtIndexPaths:@[ [self indexPathForIndex:index] ]];
 }
 
 - (void)deleteItemAtIndex:(int)index {
+  DCHECK_GE(index, 0);
   DCHECK_LT(static_cast<NSUInteger>(index), self.items.count);
+  if (self.selectedIndex == index) {

[sczs, 2017/05/26 16:12:52]

I also agree with Rohit, is it possible to push the selectedIndex from the
mediator? 

[edchin, 2017/05/26 18:09:00]

See comment above.

+    self.selectedIndex = kInvalidIndex;
+  } else if (self.selectedIndex > index) {
+    self.selectedIndex--;
+  }
   [self.items removeObjectAtIndex:index];
   [self.tabs deleteItemsAtIndexPaths:@[ [self indexPathForIndex:index] ]];
 }
 
 - (void)moveItemFromIndex:(int)fromIndex toIndex:(int)toIndex {
   TabCollectionItem* item = self.items[fromIndex];
   [self.items removeObjectAtIndex:fromIndex];
   [self.items insertObject:item atIndex:toIndex];
   [self.tabs moveItemAtIndexPath:[self indexPathForIndex:fromIndex]
                      toIndexPath:[self indexPathForIndex:toIndex]];
 }
 
 - (void)replaceItemAtIndex:(int)index withItem:(TabCollectionItem*)item {
-  [self.items removeObjectAtIndex:index];
-  [self.items insertObject:item atIndex:index];
+  DCHECK_GE(index, 0);
+  DCHECK_LT(static_cast<NSUInteger>(index), self.items.count);
+  self.items[index] = item;
+  [self.tabs reloadItemsAtIndexPaths:@[ [self indexPathForIndex:index] ]];
 }
 
 - (void)selectItemAtIndex:(int)index {
+  NSMutableArray* indexPaths = [[NSMutableArray alloc] init];
+  if (self.selectedIndex >= 0) {
+    [indexPaths addObject:[self indexPathForIndex:self.selectedIndex]];
+  }
+  if (index >= 0) {
+    DCHECK_LT(static_cast<NSUInteger>(index), self.items.count);
+    [indexPaths addObject:[self indexPathForIndex:index]];
+  }
   self.selectedIndex = index;
-  [self.tabs selectItemAtIndexPath:[self indexPathForIndex:index]
-                          animated:YES
-                    scrollPosition:UITableViewScrollPositionNone];
+  [self.tabs reloadItemsAtIndexPaths:indexPaths];
 }
 
 - (void)populateItems:(NSArray<TabCollectionItem*>*)items {
   self.items = [items mutableCopy];
   [self.tabs reloadData];
 }
 
 #pragma mark - Private
 
 - (NSIndexPath*)indexPathForIndex:(int)index {
   return [NSIndexPath indexPathForItem:index inSection:0];
 }
 
 @end