Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800473004:
[ios] Inserts and deletes history items in the same BatchUpdates block (Closed)
|
Created:
3 years, 8 months ago by sczs Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, ortuno+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Inserts and deletes history items in the same BatchUpdates block
BUG=707241
Review-Url: https://codereview.chromium.org/2800473004
Cr-Commit-Position: refs/heads/master@{#462784}
Committed: https://chromium.googlesource.com/chromium/src/+/3da34e94af62b9ff01e41e5609655608a72d725d
Patch Set 1 #
Total comments: 7
Patch Set 2 : Refactor #Messages
Total messages: 15 (7 generated)
Description was changed from ========== [ios] Inserts and deletes history items in the same BatchUpdates block BUG=707241 ========== to ========== [ios] Inserts and deletes history items in the same BatchUpdates block BUG=707241 ==========
sczs@chromium.org changed reviewers: + lpromero@chromium.org
sczs@chromium.org changed reviewers: + ramyasharma@chromium.org
PTAL There are more details in the Bug page. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:375: [self.delegate historyCollectionViewControllerDidChangeEntries:self]; 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.
Think this didn't get send. Trying again.
lgtm if my comment makes sense :) If I missed something, please provide more explanations in the CL description. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:388: completion:^(BOOL) { 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?
Thanks for fixing this :). https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:384: [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths]; 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. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:630: - (void)removeSelectedItemsFromCollection { Should this be removed? Is this used elsewhere?
Created a Method for the completion blocks. ramyasharma@ please send to CQ if LGTY. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:384: [self deleteItemsFromCollectionViewModelWithIndex:deletedIndexPaths]; On 2017/04/06 06:23:27, ramyasharma wrote: > 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. Since removeSelectedItemsFromCollection can't be removed I think this is the best we got for now, but I understand the concern. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:388: completion:^(BOOL) { On 2017/04/05 09:48:05, lpromero wrote: > 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? 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. https://codereview.chromium.org/2800473004/diff/1/ios/chrome/browser/ui/histo... ios/chrome/browser/ui/history/history_collection_view_controller.mm:630: - (void)removeSelectedItemsFromCollection { On 2017/04/06 06:23:27, ramyasharma wrote: > Should this be removed? Is this used elsewhere? This is also used when deleting items from History. In this case we only delete them from the CollectionView when filtering.
The CQ bit was checked by ramyasharma@chromium.org
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2800473004/#ps20001 (title: "Refactor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491536462593220,
"parent_rev": "9c1cc6a2801bf174202550d981303837277f058d", "commit_rev":
"3da34e94af62b9ff01e41e5609655608a72d725d"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Inserts and deletes history items in the same BatchUpdates block BUG=707241 ========== to ========== [ios] Inserts and deletes history items in the same BatchUpdates block BUG=707241 Review-Url: https://codereview.chromium.org/2800473004 Cr-Commit-Position: refs/heads/master@{#462784} Committed: https://chromium.googlesource.com/chromium/src/+/3da34e94af62b9ff01e41e560965... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3da34e94af62b9ff01e41e560965... |
