Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609173002:
Reload the ReadingListCollection only if changes occurred (Closed)
DescriptionReload the ReadingListCollection only if changes occurred
The Reading List Collection View does not need to be reloaded for every change.
This CL reloads it only if some changes occurred.
BUG=677924
Review-Url: https://codereview.chromium.org/2609173002
Cr-Commit-Position: refs/heads/master@{#441928}
Committed: https://chromium.googlesource.com/chromium/src/+/c771336bc3e7a8a29e4e0b6268f4b085c5e2da3b
Patch Set 1 #
Total comments: 15
Patch Set 2 : Modify the change detection #
Total comments: 2
Patch Set 3 : Address comment #
Messages
Total messages: 18 (6 generated)
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
PTAL.
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
PTAL.
PTAL.
https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:91: // Whether the model had modifications. has pending modifications. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:350: if (!_shouldMonitorModel) { is this possible? should we DCHECK on this? https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:357: _modelHasBeenModified = YES; Is this true? Can we only keep the flag in readingListModelDidApplyChanges:? https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:549: - (void)reloadDataIfModelChanged { This name would mean more if ([self hasModelChanged]) [self reloadData] and it is not equivalent. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:567: readItems[readIndex]); can readIndex go out of bounds? https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:574: return NO; Do you need to test unread items?
Thanks, PTAL. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:91: // Whether the model had modifications. On 2017/01/03 17:18:28, Olivier Robin wrote: > has pending modifications. Done. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:350: if (!_shouldMonitorModel) { On 2017/01/03 17:18:27, Olivier Robin wrote: > is this possible? > should we DCHECK on this? Yes it is possible, it happens when the batch update token is reset in |updateSelectedItemsWithEntryUpdater| https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:357: _modelHasBeenModified = YES; On 2017/01/03 17:18:28, Olivier Robin wrote: > Is this true? > Can we only keep the flag in readingListModelDidApplyChanges:? Done. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:549: - (void)reloadDataIfModelChanged { On 2017/01/03 17:18:28, Olivier Robin wrote: > This name would mean more > > if ([self hasModelChanged]) > [self reloadData] > > and it is not equivalent. Done. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:567: readItems[readIndex]); On 2017/01/03 17:18:28, Olivier Robin wrote: > can readIndex go out of bounds? Done. https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:574: return NO; On 2017/01/03 17:18:28, Olivier Robin wrote: > Do you need to test unread items? Done.
https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:350: if (!_shouldMonitorModel) { On 2017/01/04 09:58:50, gambard wrote: > On 2017/01/03 17:18:27, Olivier Robin wrote: > > is this possible? > > should we DCHECK on this? > > Yes it is possible, it happens when the batch update token is reset in > |updateSelectedItemsWithEntryUpdater| Proposition: Now that you don't set _modelHasBeenModified, you can stop resetting the token in updateSelectedItemsWithEntryUpdater. So you can DCHEK on this test, and replace it by if(!_modelHasBeenModified) return https://codereview.chromium.org/2609173002/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:581: if (oldItem != newItem) { If URL is the same and title or status changed, , is it possible to just update them inplace? E.g. will distillation done changing title and status on every distillation update the whole list every time?
Thanks, PTAL. https://codereview.chromium.org/2609173002/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:581: if (oldItem != newItem) { On 2017/01/05 07:51:16, Olivier Robin wrote: > If URL is the same and title or status changed, , is it possible to just update > them inplace? > E.g. will distillation done changing title and status on every distillation > update the whole list every time? Done.
https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:350: if (!_shouldMonitorModel) { On 2017/01/05 07:51:15, Olivier Robin wrote: > On 2017/01/04 09:58:50, gambard wrote: > > On 2017/01/03 17:18:27, Olivier Robin wrote: > > > is this possible? > > > should we DCHECK on this? > > > > Yes it is possible, it happens when the batch update token is reset in > > |updateSelectedItemsWithEntryUpdater| > > Proposition: > Now that you don't set _modelHasBeenModified, you can stop resetting the token > in updateSelectedItemsWithEntryUpdater. > So you can DCHEK on this test, and replace it by > if(!_modelHasBeenModified) > return I think you missed this one.
lgtm
The CQ bit was checked by gambard@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2609173002/diff/1/ios/chrome/browser/ui/readi... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:350: if (!_shouldMonitorModel) { On 2017/01/06 10:27:29, Olivier Robin wrote: > On 2017/01/05 07:51:15, Olivier Robin wrote: > > On 2017/01/04 09:58:50, gambard wrote: > > > On 2017/01/03 17:18:27, Olivier Robin wrote: > > > > is this possible? > > > > should we DCHECK on this? > > > > > > Yes it is possible, it happens when the batch update token is reset in > > > |updateSelectedItemsWithEntryUpdater| > > > > Proposition: > > Now that you don't set _modelHasBeenModified, you can stop resetting the token > > in updateSelectedItemsWithEntryUpdater. > > So you can DCHEK on this test, and replace it by > > if(!_modelHasBeenModified) > > return > > I think you missed this one. It would not work because the _modelHasBeenModified is changed only during editing.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483706824236920,
"parent_rev": "e97a0d3d3c1d8f6ed0d5597f9a4f595a9b057b2e", "commit_rev":
"c771336bc3e7a8a29e4e0b6268f4b085c5e2da3b"}
Message was sent while issue was closed.
Description was changed from ========== Reload the ReadingListCollection only if changes occurred The Reading List Collection View does not need to be reloaded for every change. This CL reloads it only if some changes occurred. BUG=677924 ========== to ========== Reload the ReadingListCollection only if changes occurred The Reading List Collection View does not need to be reloaded for every change. This CL reloads it only if some changes occurred. BUG=677924 Review-Url: https://codereview.chromium.org/2609173002 Cr-Commit-Position: refs/heads/master@{#441928} Committed: https://chromium.googlesource.com/chromium/src/+/c771336bc3e7a8a29e4e0b6268f4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c771336bc3e7a8a29e4e0b6268f4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
