Remove updatesCollectionView from grid view controller.

In order to allow the tab thumbnails to be up to date when the thumb
strip is visible, the grid's collection view needs to be updated even
when other view controllers are obscuring it. When selecting a new item
from the collection view (blue highlight around the thumbnail), the item
that is currently selected needs to be deselected to prevent multiple
simultaneous selection.

Bug: 1127604
Change-Id: Ice919b9b7c068d381d667b0ac0ad82c41abfed35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426590
Commit-Queue: Roberto Moura <mouraroberto@google.com>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Auto-Submit: Roberto Moura <mouraroberto@google.com>
Cr-Commit-Position: refs/heads/master@{#811968}

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

@@ -51,11 +51,6 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
                                   UICollectionViewDelegate,
                                   UICollectionViewDragDelegate,
                                   UICollectionViewDropDelegate>
-// There is no need to update the collection view when other view controllers
-// are obscuring the collection view. Bookkeeping is based on |-viewWillAppear:|
-// and |-viewWillDisappear methods. Note that the |Did| methods are not reliably
-// called (e.g., edge case in multitasking).
-@property(nonatomic, assign) BOOL updatesCollectionView;
 // A collection view of items in a grid format.
 @property(nonatomic, weak) UICollectionView* collectionView;
 // The local model backing the collection view.
@@ -287,7 +282,6 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 }
 
 - (void)contentWillAppearAnimated:(BOOL)animated {
-  self.updatesCollectionView = YES;
   self.defaultLayout.animatesItemUpdates = YES;
   [self.collectionView reloadData];
   // Selection is invalid if there are no items.
@@ -305,7 +299,6 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 }
 
 - (void)contentWillDisappear {
-  self.updatesCollectionView = NO;
 }
 
 #pragma mark - UICollectionViewDataSource
@@ -558,17 +551,14 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 
   self.items = [items mutableCopy];
   self.selectedItemID = selectedItemID;
-  if ([self updatesCollectionView]) {
-    [self.collectionView reloadData];
-    [self.collectionView
-        selectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
-                     animated:YES
-               scrollPosition:UICollectionViewScrollPositionTop];
-    if (self.items.count > 0) {
-      [self removeEmptyStateAnimated:YES];
-    } else {
-      [self animateEmptyStateIn];
-    }
+  [self.collectionView reloadData];
+  [self.collectionView selectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
+                                    animated:YES
+                              scrollPosition:UICollectionViewScrollPositionTop];
+  if (self.items.count > 0) {
+    [self removeEmptyStateAnimated:YES];
+  } else {
+    [self animateEmptyStateIn];
   }
   // Whether the view is visible or not, the delegate must be updated.
   [self.delegate gridViewController:self didChangeItemCount:self.items.count];
@@ -647,9 +637,13 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 }
 
 - (void)selectItemWithID:(NSString*)selectedItemID {
-  self.selectedItemID = selectedItemID;
-  if (!([self updatesCollectionView] && self.showsSelectionUpdates))
+  if (self.selectedItemID == selectedItemID)
     return;
+
+  [self.collectionView
+      deselectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
+                     animated:YES];
+  self.selectedItemID = selectedItemID;
   [self.collectionView
       selectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
                    animated:YES
@@ -664,8 +658,6 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
          [self indexOfItemWithID:item.identifier] == NSNotFound);
   NSUInteger index = [self indexOfItemWithID:itemID];
   self.items[index] = item;
-  if (![self updatesCollectionView])
-    return;
   GridCell* cell = base::mac::ObjCCastStrict<GridCell>(
       [self.collectionView cellForItemAtIndexPath:CreateIndexPath(index)]);
   // |cell| may be nil if it is scrolled offscreen.
@@ -742,19 +734,11 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 
 #pragma mark - Private
 
-// Performs model updates and view updates together if the view is appeared, or
-// only the model updates if the view is not appeared. |completion| is only run
-// if view is appeared.
+// Performs model updates and view updates together.
 - (void)performModelUpdates:(ProceduralBlock)modelUpdates
               collectionViewUpdates:(ProceduralBlock)collectionViewUpdates
     collectionViewUpdatesCompletion:
         (ProceduralBlockWithBool)collectionViewUpdatesCompletion {
-  // If the view isn't visible, there's no need for the collection view to
-  // update.
-  if (![self updatesCollectionView]) {
-    modelUpdates();
-    return;
-  }
   [self.collectionView performBatchUpdates:^{
     // Synchronize model and view updates.
     modelUpdates();

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

@@ -17,18 +17,12 @@
 #endif
 
 // Test object that exposes the inner state for test verification.
-@interface TestGridViewController : GridViewController
+@interface GridViewController (Testing)
 @property(nonatomic, readonly) NSMutableArray<GridItem*>* items;
 @property(nonatomic, readonly) NSUInteger selectedIndex;
 @property(nonatomic, readonly) UICollectionView* collectionView;
 @property(nonatomic, assign, getter=isViewAppeared) BOOL viewAppeared;
 @end
-@implementation TestGridViewController
-@dynamic items;
-@dynamic selectedIndex;
-@dynamic collectionView;
-@dynamic viewAppeared;
-@end
 
 // Fake object that conforms to GridViewControllerDelegate.
 @interface FakeGridViewControllerDelegate
@@ -62,7 +56,8 @@
 class GridViewControllerTest : public RootViewControllerTest {
  public:
   GridViewControllerTest() {
-    view_controller_ = [[TestGridViewController alloc] init];
+    view_controller_ = [[GridViewController alloc] init];
+    [view_controller_ loadView];
     NSArray* items = @[
       [[GridItem alloc] initWithIdentifier:@"A"],
       [[GridItem alloc] initWithIdentifier:@"B"]
@@ -74,7 +69,7 @@ class GridViewControllerTest : public RootViewControllerTest {
   }
 
  protected:
-  TestGridViewController* view_controller_;
+  GridViewController* view_controller_;
   FakeGridViewControllerDelegate* delegate_;
 };
 

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

@@ -234,7 +234,9 @@ char kResponse3[] = "Test Page 3 content";
 
 - (void)longPressRecentTabWithTitle:(NSString*)title {
   // The test page may be there multiple times.
-  [[[EarlGrey selectElementWithMatcher:grey_accessibilityLabel(title)]
+  [[[EarlGrey
+      selectElementWithMatcher:grey_allOf(grey_accessibilityLabel(title),
+                                          grey_sufficientlyVisible(), nil)]
       atIndex:0] performAction:grey_longPress()];
 }
 

ios/chrome/browser/web/browsing_egtest.mm

@@ -77,12 +77,13 @@ class ReloadResponseProvider : public web::DataResponseProvider {
 
 @implementation BrowsingTestCase
 
-// Matcher for the title of the current tab (on tablet only).
+// Matcher for the title of the current tab (on tablet only), which is
+// sufficiently visible.
 id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
   id<GREYMatcher> notPartOfOmnibox =
       grey_not(grey_ancestor(chrome_test_util::Omnibox()));
   return grey_allOf(grey_accessibilityLabel(base::SysUTF8ToNSString(tab_title)),
-                    notPartOfOmnibox, nil);
+                    notPartOfOmnibox, grey_sufficientlyVisible(), nil);
 }
 
 // Tests that page successfully reloads.
@@ -120,7 +121,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
       destinationURL.spec().substr(destinationURL.scheme().length() + 3);
 
   [[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)]
-      assertWithMatcher:grey_sufficientlyVisible()];
+      assertWithMatcher:grey_notNil()];
 }
 
 // Tests that after a PDF is loaded, the title appears in the tab bar on iPad.
@@ -138,7 +139,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
       destinationURL.spec().substr(destinationURL.scheme().length() + 3);
 
   [[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)]
-      assertWithMatcher:grey_sufficientlyVisible()];
+      assertWithMatcher:grey_notNil()];
 }
 
 // Tests that tab title is set to the specified title from a JavaScript.
@@ -155,7 +156,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
   [ChromeEarlGrey loadURL:URL];
 
   [[EarlGrey selectElementWithMatcher:TabWithTitle(kPageTitle)]
-      assertWithMatcher:grey_sufficientlyVisible()];
+      assertWithMatcher:grey_notNil()];
 }
 
 // Tests clicking a link with target="_blank" and "event.stopPropagation()"