Commit 38ef961b authored by Leonard Grey's avatar Leonard Grey Committed by Commit Bot

Revert "Remove updatesCollectionView from grid view controller."

This reverts commit 28310fb5.

Reason for revert: [Sheriff] Failing deterministically on ios-simulator-noncq
Example: https://ci.chromium.org/p/chromium/builders/ci/ios-simulator-noncq/12130

Original change's description:
> 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: Ic1e4a2dd4d0f2c269031b9d27b47b38c56b07a40
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410494
> Reviewed-by: Gauthier Ambard <gambard@chromium.org>
> Commit-Queue: Roberto Moura <mouraroberto@google.com>
> Auto-Submit: Roberto Moura <mouraroberto@google.com>
> Cr-Commit-Position: refs/heads/master@{#809263}

TBR=gambard@chromium.org,mouraroberto@google.com

Change-Id: Ia7c8b2a2946424754838f5d2fdcd76d203745e59
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1127604
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423170Reviewed-by: default avatarLeonard Grey <lgrey@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809337}
parent cfa45d07
...@@ -51,6 +51,11 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -51,6 +51,11 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
UICollectionViewDelegate, UICollectionViewDelegate,
UICollectionViewDragDelegate, UICollectionViewDragDelegate,
UICollectionViewDropDelegate> 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. // A collection view of items in a grid format.
@property(nonatomic, weak) UICollectionView* collectionView; @property(nonatomic, weak) UICollectionView* collectionView;
// The local model backing the collection view. // The local model backing the collection view.
...@@ -282,6 +287,7 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -282,6 +287,7 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
} }
- (void)contentWillAppearAnimated:(BOOL)animated { - (void)contentWillAppearAnimated:(BOOL)animated {
self.updatesCollectionView = YES;
self.defaultLayout.animatesItemUpdates = YES; self.defaultLayout.animatesItemUpdates = YES;
[self.collectionView reloadData]; [self.collectionView reloadData];
// Selection is invalid if there are no items. // Selection is invalid if there are no items.
...@@ -299,6 +305,7 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -299,6 +305,7 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
} }
- (void)contentWillDisappear { - (void)contentWillDisappear {
self.updatesCollectionView = NO;
} }
#pragma mark - UICollectionViewDataSource #pragma mark - UICollectionViewDataSource
...@@ -522,14 +529,17 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -522,14 +529,17 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
self.items = [items mutableCopy]; self.items = [items mutableCopy];
self.selectedItemID = selectedItemID; self.selectedItemID = selectedItemID;
[self.collectionView reloadData]; if ([self updatesCollectionView]) {
[self.collectionView selectItemAtIndexPath:CreateIndexPath(self.selectedIndex) [self.collectionView reloadData];
animated:YES [self.collectionView
scrollPosition:UICollectionViewScrollPositionTop]; selectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
if (self.items.count > 0) { animated:YES
[self removeEmptyStateAnimated:YES]; scrollPosition:UICollectionViewScrollPositionTop];
} else { if (self.items.count > 0) {
[self animateEmptyStateIn]; [self removeEmptyStateAnimated:YES];
} else {
[self animateEmptyStateIn];
}
} }
// Whether the view is visible or not, the delegate must be updated. // Whether the view is visible or not, the delegate must be updated.
[self.delegate gridViewController:self didChangeItemCount:self.items.count]; [self.delegate gridViewController:self didChangeItemCount:self.items.count];
...@@ -608,13 +618,9 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -608,13 +618,9 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
} }
- (void)selectItemWithID:(NSString*)selectedItemID { - (void)selectItemWithID:(NSString*)selectedItemID {
if (self.selectedItemID == selectedItemID)
return;
[self.collectionView
deselectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
animated:YES];
self.selectedItemID = selectedItemID; self.selectedItemID = selectedItemID;
if (!([self updatesCollectionView] && self.showsSelectionUpdates))
return;
[self.collectionView [self.collectionView
selectItemAtIndexPath:CreateIndexPath(self.selectedIndex) selectItemAtIndexPath:CreateIndexPath(self.selectedIndex)
animated:YES animated:YES
...@@ -629,6 +635,8 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -629,6 +635,8 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
[self indexOfItemWithID:item.identifier] == NSNotFound); [self indexOfItemWithID:item.identifier] == NSNotFound);
NSUInteger index = [self indexOfItemWithID:itemID]; NSUInteger index = [self indexOfItemWithID:itemID];
self.items[index] = item; self.items[index] = item;
if (![self updatesCollectionView])
return;
GridCell* cell = base::mac::ObjCCastStrict<GridCell>( GridCell* cell = base::mac::ObjCCastStrict<GridCell>(
[self.collectionView cellForItemAtIndexPath:CreateIndexPath(index)]); [self.collectionView cellForItemAtIndexPath:CreateIndexPath(index)]);
// |cell| may be nil if it is scrolled offscreen. // |cell| may be nil if it is scrolled offscreen.
...@@ -705,13 +713,22 @@ NSIndexPath* CreateIndexPath(NSInteger index) { ...@@ -705,13 +713,22 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
#pragma mark - Private #pragma mark - Private
// Performs model updates and view updates. // 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.
- (void)performModelUpdates:(ProceduralBlock)modelUpdates - (void)performModelUpdates:(ProceduralBlock)modelUpdates
collectionViewUpdates:(ProceduralBlock)collectionViewUpdates collectionViewUpdates:(ProceduralBlock)collectionViewUpdates
collectionViewUpdatesCompletion: collectionViewUpdatesCompletion:
(ProceduralBlockWithBool)collectionViewUpdatesCompletion { (ProceduralBlockWithBool)collectionViewUpdatesCompletion {
modelUpdates(); // If the view isn't visible, there's no need for the collection view to
// update.
if (![self updatesCollectionView]) {
modelUpdates();
return;
}
[self.collectionView performBatchUpdates:^{ [self.collectionView performBatchUpdates:^{
// Synchronize model and view updates.
modelUpdates();
collectionViewUpdates(); collectionViewUpdates();
} }
completion:collectionViewUpdatesCompletion]; completion:collectionViewUpdatesCompletion];
......
...@@ -77,13 +77,12 @@ class ReloadResponseProvider : public web::DataResponseProvider { ...@@ -77,13 +77,12 @@ class ReloadResponseProvider : public web::DataResponseProvider {
@implementation BrowsingTestCase @implementation BrowsingTestCase
// Matcher for the title of the current tab (on tablet only), which is // Matcher for the title of the current tab (on tablet only).
// sufficiently visible.
id<GREYMatcher> TabWithTitle(const std::string& tab_title) { id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
id<GREYMatcher> notPartOfOmnibox = id<GREYMatcher> notPartOfOmnibox =
grey_not(grey_ancestor(chrome_test_util::Omnibox())); grey_not(grey_ancestor(chrome_test_util::Omnibox()));
return grey_allOf(grey_accessibilityLabel(base::SysUTF8ToNSString(tab_title)), return grey_allOf(grey_accessibilityLabel(base::SysUTF8ToNSString(tab_title)),
notPartOfOmnibox, grey_sufficientlyVisible(), nil); notPartOfOmnibox, nil);
} }
// Tests that page successfully reloads. // Tests that page successfully reloads.
...@@ -121,7 +120,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) { ...@@ -121,7 +120,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
destinationURL.spec().substr(destinationURL.scheme().length() + 3); destinationURL.spec().substr(destinationURL.scheme().length() + 3);
[[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)] [[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)]
assertWithMatcher:grey_notNil()]; assertWithMatcher:grey_sufficientlyVisible()];
} }
// Tests that after a PDF is loaded, the title appears in the tab bar on iPad. // Tests that after a PDF is loaded, the title appears in the tab bar on iPad.
...@@ -139,7 +138,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) { ...@@ -139,7 +138,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
destinationURL.spec().substr(destinationURL.scheme().length() + 3); destinationURL.spec().substr(destinationURL.scheme().length() + 3);
[[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)] [[EarlGrey selectElementWithMatcher:TabWithTitle(URLWithoutScheme)]
assertWithMatcher:grey_notNil()]; assertWithMatcher:grey_sufficientlyVisible()];
} }
// Tests that tab title is set to the specified title from a JavaScript. // Tests that tab title is set to the specified title from a JavaScript.
...@@ -156,7 +155,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) { ...@@ -156,7 +155,7 @@ id<GREYMatcher> TabWithTitle(const std::string& tab_title) {
[ChromeEarlGrey loadURL:URL]; [ChromeEarlGrey loadURL:URL];
[[EarlGrey selectElementWithMatcher:TabWithTitle(kPageTitle)] [[EarlGrey selectElementWithMatcher:TabWithTitle(kPageTitle)]
assertWithMatcher:grey_notNil()]; assertWithMatcher:grey_sufficientlyVisible()];
} }
// Tests clicking a link with target="_blank" and "event.stopPropagation()" // Tests clicking a link with target="_blank" and "event.stopPropagation()"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment