[ios] Inserts and deletes history items in the same BatchUpdates block

ios/chrome/browser/ui/history/history_collection_view_controller.mm

Index: ios/chrome/browser/ui/history/history_collection_view_controller.mm
diff --git a/ios/chrome/browser/ui/history/history_collection_view_controller.mm b/ios/chrome/browser/ui/history/history_collection_view_controller.mm
index 4fdf911823afc17d06ed682ae366b2d0b80e929d..10350d13c83e679fc6a6757a4eb5d1897cf83e0d 100644
--- a/ios/chrome/browser/ui/history/history_collection_view_controller.mm
+++ b/ios/chrome/browser/ui/history/history_collection_view_controller.mm
@@ -114,8 +114,11 @@ const CGFloat kSeparatorInset = 10;
 // Removes selected items from the visible collection, but does not delete them
 // from browser history.
 - (void)removeSelectedItemsFromCollection;
-// Removes all items in the collection that are not included in entries.
+// Selects all items in the collection that are not included in entries.
 - (void)filterForHistoryEntries:(NSArray*)entries;
+// Deletes all items in the collection which indexes are included in indexArray,
+// needs to be run inside a performBatchUpdates block.
+- (void)deleteItemsFromCollectionViewModelWithIndex:(NSArray*)indexArray;
 // Adds loading indicator to the top of the history collection, if one is not
 // already present.
 - (void)addLoadingIndicator;
@@ -370,16 +373,25 @@ const CGFloat kSeparatorInset = 10;
       }
     }
     [self.delegate historyCollectionViewControllerDidChangeEntries:self];

[sczs, 2017/04/05 01:57:50]

We can clearly see now that this was being called twice, once here and once in
the completion block. I think we could remove this call, on the other hand,
since this might be Cherry picked, I want to change as less as possible. 

+    if (([self isSearching] && [searchQuery length] > 0 &&
+         [self.currentQuery isEqualToString:searchQuery]) ||
+        self.filterQueryResult) {
+      // If in search mode, filter out entries that are not
+      // part of the search result.
+      [self filterForHistoryEntries:filterResults];
+      NSArray* deletedIndexPaths =
+          self.collectionView.indexPathsForSelectedItems;
+      [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths];

[ramyasharma, 2017/04/06 06:23:27]

Why not call this inline? Especially since this method has a constraint that it
must be called from within PerformBatchUpdates, isn't it better to just call it
inline? Rather than expect the caller in future to ensure its called in
PerformBatchUpdates?
This is assuming that removeSelectedItemsFromCollection can be removed. 

[sczs, 2017/04/07 02:03:25]

Since removeSelectedItemsFromCollection can't be removed I think this is the
best we got for now, but I understand the concern. 

+      self.filterQueryResult = NO;
+    }
   }
       completion:^(BOOL) {

[lpromero, 2017/04/05 09:48:05]

This is the same completion block as in the removeSelectedItems method, please
create a method.

Before we had:

performBatchUpdate
  ... something ...
completion
  filter
    remove
      performBatchUpdates
        deleteItems
      completion
        "updateForNoEntries"

Now we have:

performBatchUpdate
  ... something ...
  filter
  deleteItems
completion
  "updateForNoEntries"

Did I get this right?

[sczs, 2017/04/07 02:03:25]

Exactly right! The exact same code is being executed, only that this time
everything happens inside one performBatchUpdates block.

Will create a method a method for the completion block.

-        if (([self isSearching] && [searchQuery length] > 0 &&
-             [self.currentQuery isEqualToString:searchQuery]) ||
-            self.filterQueryResult) {
-          // If in search mode, filter out entries that are not
-          // part of the search result.
-          [self filterForHistoryEntries:filterResults];
-          self.filterQueryResult = NO;
+        // If only the header section remains, there are no history entries.
+        if ([self.collectionViewModel numberOfSections] == 1) {
+          self.entriesType = NO_ENTRIES;
         }
+        [self updateEntriesStatusMessage];
+        [self.delegate historyCollectionViewControllerDidChangeEntries:self];
       }];
 }
 
@@ -618,17 +630,7 @@ const CGFloat kSeparatorInset = 10;
 - (void)removeSelectedItemsFromCollection {

[ramyasharma, 2017/04/06 06:23:27]

Should this be removed? Is this used elsewhere?

[sczs, 2017/04/07 02:03:25]

This is also used when deleting items from History. In this case we only delete
them from the CollectionView when filtering.

   NSArray* deletedIndexPaths = self.collectionView.indexPathsForSelectedItems;
   [self.collectionView performBatchUpdates:^{
-    [self collectionView:self.collectionView
-        willDeleteItemsAtIndexPaths:deletedIndexPaths];
-    [self.collectionView deleteItemsAtIndexPaths:deletedIndexPaths];
-
-    // Remove any empty sections, except the header section.
-    for (int section = self.collectionView.numberOfSections - 1; section > 0;
-         --section) {
-      if (![self.collectionViewModel numberOfItemsInSection:section]) {
-        [self.entryInserter removeSection:section];
-      }
-    }
+    [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths];
   }
       completion:^(BOOL) {
         // If only the header section remains, there are no history entries.
@@ -640,6 +642,20 @@ const CGFloat kSeparatorInset = 10;
       }];
 }
 
+- (void)deleteItemsFromCollectionViewModelWithIndex:(NSArray*)indexArray {
+  [self collectionView:self.collectionView
+      willDeleteItemsAtIndexPaths:indexArray];
+  [self.collectionView deleteItemsAtIndexPaths:indexArray];
+
+  // Remove any empty sections, except the header section.
+  for (int section = self.collectionView.numberOfSections - 1; section > 0;
+       --section) {
+    if (![self.collectionViewModel numberOfItemsInSection:section]) {
+      [self.entryInserter removeSection:section];
+    }
+  }
+}
+
 - (void)filterForHistoryEntries:(NSArray*)entries {
   self.collectionView.allowsMultipleSelection = YES;
   for (int section = 1; section < [self.collectionViewModel numberOfSections];
@@ -665,7 +681,6 @@ const CGFloat kSeparatorInset = 10;
       }
     }
   }
-  [self removeSelectedItemsFromCollection];
 }
 
 - (void)addLoadingIndicator {