New issue
Advanced search Search tips

Issue 664924 link

Starred by 1 user
Status: Fixed
Owner:
olivierrobin@chromium.org
Closed: Dec 2016
Cc:
ios-bugs@chromium.org
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Simplify read_/unread_ logic in ReadingListModel

Project Member Reported by olivierrobin@chromium.org, Nov 14 2016 
The current implementation force multiple scans on each action with impact in both readibility and performances.

Possibilities to improve:
- merge the two lists and only filter them on display
- Add a read parameter on the notifications that would allow to factorize the code.

Comment 1 by olivierrobin@chromium.org, Nov 22 2016

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/448b3411da25817b84202bff7c37bbc816c4b2b2

commit 448b3411da25817b84202bff7c37bbc816c4b2b2
Author: olivierrobin <olivierrobin@chromium.org>
Date: Wed Nov 30 13:03:53 2016

Refactor Reading List Model to use URL as key.

Here are the new principles for ReadingList model.
- Entries are uniquely defined by their URL. All methods to access or alter entries in the model are therefore using URL as key.

- All observers callbacks are therefore give the URL of the entry being modified, with the exception of WillAddEntry that takes the whole entry (as it is not yet in the model). A DidAddEntry observer is added if an observer want to do operation on this newly added entry.

- Model is now backed up by an unordered_map<GURL, ReadingListEntry>. This will make all operations accessing or updating entries O(1) instead of O(n).

- The read/unread status is now in the ReadingListEntry because
  - most operation on entries are read status independant, so have the read status in the model requires to have two paths for each operations. Having the read status in the entry makes all operations easier
  - ReadingListEntries in Chrome have always a defined read/unread status
  - ReadingListEntry now reflect the sync counterpart ReadingListSpecifics.

- To display the entries, they must be ordered by UpdateTime. This CL adds a cache that allows to access the entries with GetReadEntryAt(size_t index) and GetUnreadEntryAt(size_t index). This cache will be moved in the ReadingListViewController later

- This CL adds dummy methods for downstream compatibility. These will be removed later.

BUG= 664924 

Review-Url: https://codereview.chromium.org/2525663002
Cr-Commit-Position: refs/heads/master@{#435236}

[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_entry.cc
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_entry.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_entry_unittest.cc
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_bridge_observer.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_bridge_observer.mm
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_impl.cc
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_impl.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_observer.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_storage.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_model_unittest.mm
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_store.cc
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_store.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_store_delegate.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/components/reading_list/ios/reading_list_store_unittest.mm
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/ios/chrome/browser/reading_list/reading_list_download_service.cc
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/ios/chrome/browser/reading_list/reading_list_download_service.h
[modify] https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2/ios/chrome/browser/reading_list/reading_list_web_state_observer.mm

Comment 3 by olivierrobin@chromium.org, Nov 30 2016

Labels: Hotlist-ReadingList
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 1 2016 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/f674a9ac1dae2bcb039acd533a6a4b68f2c3357b

commit f674a9ac1dae2bcb039acd533a6a4b68f2c3357b
Author: olivierrobin <olivierrobin@google.com>
Date: Thu Dec 01 14:54:43 2016

Comment 6 by olivierrobin@chromium.org, Dec 2 2016

Status: Fixed (was: Started)
►

Sign in to add a comment