Refactor Reading List Model to use URL as key.

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
Committed: https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2
Cr-Commit-Position: refs/heads/master@{#435236}

[Olivier, 2016-11-22 15:30:35]

Description was changed from

==========
Refactor Reading List Model to use URL as key.
==========

to

==========
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).

- 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
==========

[Olivier, 2016-11-22 17:06:45]

olivierrobin@chromium.org changed reviewers:
+ gambard@chromium.org, jif@chromium.org

[Olivier, 2016-11-22 17:06:46]

This is not ready for review.
Please give your comments on the new APIs.

[jif-google, 2016-11-22 18:21:44]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-11-22 18:21:46]

thanks, it's much better :)
Overall I agree with all your choices.
Ping me when you want a full review.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:35:
std::unordered_map<GURL, ReadingListEntry, GURLHasher>;
are you using std::unordered_map instead of std::map to prevent nˆ2 attacks?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:81: virtual const
ReadingListEntry* GetEntryAt(size_t index) const = 0;
I think it would be useful to offer range based for-loops on the reading list
model, like:

for (const ReadingListEntry& entry : reading_list_model) {


}

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:85: virtual const
ReadingListEntry* GetReadEntryAt(size_t index) const = 0;
this will be removed, right?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:89: virtual const
ReadingListEntry* GetUnreadEntryAt(size_t index) const = 0;
this will be removed, right?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:190: if
(!entries_->count(gurl))
instead of calling |->count| and |->at|, just do a single |->find|, and check
that the returned iterator is OK.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:222:
entries_->insert(std::make_pair(entry->URL(), std::move(*entry)));
this is pretty much:
some_func(a, std::move(a));
you like to live dangerously.

How about:

auto& entryInMap = entries_[entry->URL()];
entryInMap = std::move(*entry);

Even better:
auto& entryInMap = entries_[entry->URL()];
entryInMap = std::move(entry.release());

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:236: GURL url =
entry->URL();
const GURL & url = ...

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:300:
RemoveEntryByURL(url);
Wouldn't it make more sense to call "willUpdateEntry" on the observers if a
reading list entry for that URL already existed.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:108: // no entry is
found. If an entry is found, |read| is set to the read status
update comment.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:38: void SaveEntry(const
ReadingListEntry* entry) override;
why a pointer?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:39: void RemoveEntry(const
ReadingListEntry* entry) override;
same

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:126: const
ReadingListEntry* entry);
same

[gambard, 2016-11-23 12:11:07]

Way better!

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:107: void
ReadingListDownloadService::ScheduleDownloadEntryFromURL(const GURL& url) {
Exist only because the entry could not been requested as a pointer. It could be
merged with ScheduleDownloadEntry.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:114: void
ReadingListDownloadService::DownloadEntryFromURL(const GURL& url) {
Same as above.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:161: const
ReadingListEntry& entry) {
This method only uses the URL. Pass the URL as argument instead of the entry?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model.h (left):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:74: virtual const
ReadingListEntry& GetReadEntryAtIndex(size_t index) const = 0;
You are breaking downstream by renaming them no?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:68: virtual size_t
read_size() const = 0;
This can probably be removed.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:94: virtual bool
CallbackEntryURL(
This can probably be removed, it was a workaround because we only returned
reference not pointers.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_bridge_observer.h
(right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_bridge_observer.h:30:
willAddEntry:(const ReadingListEntry&)url;
From your CL description, WillAddEntry should pass the entry not the URL.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:30:
read_entry_count_(0),
I would have only kept the unread entry count. As you have both we can probably
keep it for tests and things like that, but it is not used.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:85: return
unread_entry_count_ + read_entry_count_;
entries_->size() is probably shorter and you are using it just above.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:119: const
ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
I am not sure this method makes sense.
The order "unread by date then read by date" seems weird to me. I would only
keep the GetUnread/ReadEntryAt(), then remove them completely once it is moved
to ReadingListViewController.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:126: const
ReadingListEntry* ReadingListModelImpl::GetEntryAt(size_t index) const {
Remove this method. You only need the old method for downstream compatibility.
When changing the downstream code you can move those methods there with the
cache.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:142: const
ReadingListEntry* ReadingListModelImpl::GetReadEntryAt(
Same as above.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:46: size_t
unread_size() const override;
Will this be deleted?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:117: void
MoveEntryFrom(ReadingListEntries* entries,
Is this still used?

[Olivier, 2016-11-28 14:54:15]

Thanks.

PTAL.
This version is downstream compatible.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:107: void
ReadingListDownloadService::ScheduleDownloadEntryFromURL(const GURL& url) {
On 2016/11/23 12:11:06, gambard wrote:
> Exist only because the entry could not been requested as a pointer. It could
be
> merged with ScheduleDownloadEntry.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:114: void
ReadingListDownloadService::DownloadEntryFromURL(const GURL& url) {
On 2016/11/23 12:11:06, gambard wrote:
> Same as above.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:161: const
ReadingListEntry& entry) {
On 2016/11/23 12:11:06, gambard wrote:
> This method only uses the URL. Pass the URL as argument instead of the entry?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.h:35:
std::unordered_map<GURL, ReadingListEntry, GURLHasher>;
On 2016/11/22 18:21:45, jif-google wrote:
> are you using std::unordered_map instead of std::map to prevent nˆ2 attacks?

I asked the C++ expert what to use here. He said unordered_map.
Who am I to question him?

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model.h (left):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:74: virtual const
ReadingListEntry& GetReadEntryAtIndex(size_t index) const = 0;
On 2016/11/23 12:11:06, gambard wrote:
> You are breaking downstream by renaming them no?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:68: virtual size_t
read_size() const = 0;
On 2016/11/23 12:11:06, gambard wrote:
> This can probably be removed.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:81: virtual const
ReadingListEntry* GetEntryAt(size_t index) const = 0;
On 2016/11/22 18:21:45, jif-google wrote:
> I think it would be useful to offer range based for-loops on the reading list
> model, like:
> 
> for (const ReadingListEntry& entry : reading_list_model) {
> 
> 
> }

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:85: virtual const
ReadingListEntry* GetReadEntryAt(size_t index) const = 0;
On 2016/11/22 18:21:45, jif-google wrote:
> this will be removed, right?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:89: virtual const
ReadingListEntry* GetUnreadEntryAt(size_t index) const = 0;
On 2016/11/22 18:21:45, jif-google wrote:
> this will be removed, right?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model.h:94: virtual bool
CallbackEntryURL(
On 2016/11/23 12:11:06, gambard wrote:
> This can probably be removed, it was a workaround because we only returned
> reference not pointers.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_bridge_observer.h
(right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_bridge_observer.h:30:
willAddEntry:(const ReadingListEntry&)url;
On 2016/11/23 12:11:06, gambard wrote:
> From your CL description, WillAddEntry should pass the entry not the URL.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:30:
read_entry_count_(0),
On 2016/11/23 12:11:07, gambard wrote:
> I would have only kept the unread entry count. As you have both we can
probably
> keep it for tests and things like that, but it is not used.

I will keep them at the moment, but I may remove one of them in the cleaning CL.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:85: return
unread_entry_count_ + read_entry_count_;
On 2016/11/23 12:11:07, gambard wrote:
> entries_->size() is probably shorter and you are using it just above.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:119: const
ReadingListEntry* ReadingListModelImpl::GetEntryFromURL(
On 2016/11/23 12:11:06, gambard wrote:
> I am not sure this method makes sense.
> The order "unread by date then read by date" seems weird to me. I would only
> keep the GetUnread/ReadEntryAt(), then remove them completely once it is moved
> to ReadingListViewController.

I guess this applies to GetEntryAt. Removed.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:126: const
ReadingListEntry* ReadingListModelImpl::GetEntryAt(size_t index) const {
On 2016/11/23 12:11:07, gambard wrote:
> Remove this method. You only need the old method for downstream compatibility.
> When changing the downstream code you can move those methods there with the
> cache.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:142: const
ReadingListEntry* ReadingListModelImpl::GetReadEntryAt(
On 2016/11/23 12:11:06, gambard wrote:
> Same as above.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:190: if
(!entries_->count(gurl))
On 2016/11/22 18:21:45, jif-google wrote:
> instead of calling |->count| and |->at|, just do a single |->find|, and check
> that the returned iterator is OK.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:222:
entries_->insert(std::make_pair(entry->URL(), std::move(*entry)));
On 2016/11/22 18:21:45, jif-google wrote:
> this is pretty much:
> some_func(a, std::move(a));
> you like to live dangerously.
> 
> How about:
> 
> auto& entryInMap = entries_[entry->URL()];
> entryInMap = std::move(*entry);
> 
> Even better:
> auto& entryInMap = entries_[entry->URL()];
> entryInMap = std::move(entry.release());

Done.
You cannot use operator[] as there is no default constructor for
ReadingListEntry.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:236: GURL url =
entry->URL();
On 2016/11/22 18:21:45, jif-google wrote:
> const GURL & url = ...

I need the url adter the std::move, so I make a copy.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.cc:300:
RemoveEntryByURL(url);
On 2016/11/22 18:21:45, jif-google wrote:
> Wouldn't it make more sense to call "willUpdateEntry" on the observers if a
> reading list entry for that URL already existed.

The current workflow is to remove the entry, then readd it (meaning redownload
the offline version).

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_model_impl.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:46: size_t
unread_size() const override;
On 2016/11/23 12:11:07, gambard wrote:
> Will this be deleted?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:108: // no entry is
found. If an entry is found, |read| is set to the read status
On 2016/11/22 18:21:45, jif-google wrote:
> update comment.

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_impl.h:117: void
MoveEntryFrom(ReadingListEntries* entries,
On 2016/11/23 12:11:07, gambard wrote:
> Is this still used?

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_store.h (right):

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:38: void SaveEntry(const
ReadingListEntry* entry) override;
On 2016/11/22 18:21:45, jif-google wrote:
> why a pointer?

done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:39: void RemoveEntry(const
ReadingListEntry* entry) override;
On 2016/11/22 18:21:45, jif-google wrote:
> same

Done.

https://codereview.chromium.org/2525663002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_store.h:126: const
ReadingListEntry* entry);
On 2016/11/22 18:21:45, jif-google wrote:
> same

Done.

[Olivier, 2016-11-28 14:55:11]

Description was changed from

==========
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).

- 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
==========

to

==========
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
==========

[Olivier, 2016-11-28 14:55:49]

olivierrobin@chromium.org changed reviewers:
+ noyau@chromium.org

[noyau (Ping after 24h), 2016-11-28 15:29:32]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:35: std::unordered_map<GURL,
ReadingListEntry, GURLHasher>;
Why is this here actually?  It is not used in this file, should it be moved
(along with the GURLHasher) to ReadingListModel?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:57: virtual const
ReadingListEntries::iterator end() const = 0;
Can this just iterate URLs instead of exposing the map<>?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:60: virtual size_t size() const
= 0;
No need for the unread size? Isn't it what is used in the menu?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:86: virtual void
SetReadStatus(const GURL& url, bool read) = 0;
You removed all the comments about what's happening when something changed from
read to unread and vice versa.

I personally prefer the old two methods API rather than a method with a bool. It
is more readable when used.

[noyau (Ping after 24h), 2016-11-28 15:30:15]

https://codereview.chromium.org/2525663002/diff/80001/components/reading_list...
File components/reading_list/ios/reading_list_model_unittest.mm (right):

https://codereview.chromium.org/2525663002/diff/80001/components/reading_list...
components/reading_list/ios/reading_list_model_unittest.mm:211: DCHECK_EQ(size,
morel_->unread_size());
morel?

[Olivier, 2016-11-28 15:38:10]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:35: std::unordered_map<GURL,
ReadingListEntry, GURLHasher>;
On 2016/11/28 15:29:31, noyau wrote:
> Why is this here actually?  It is not used in this file, should it be moved
> (along with the GURLHasher) to ReadingListModel?

If I move it, I must move it in ReadingListModel, ReadingListStore,
ReadingListUnittests...

This is not impossible, but we will need to keep all of them in sync (specially
the hash function).

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:57: virtual const
ReadingListEntries::iterator end() const = 0;
On 2016/11/28 15:29:32, noyau wrote:
> Can this just iterate URLs instead of exposing the map<>?

We can, I will check how to do that.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:60: virtual size_t size() const
= 0;
On 2016/11/28 15:29:32, noyau wrote:
> No need for the unread size? Isn't it what is used in the menu?

Yes, we need it.
I cleaned it too fast
Added back in next PS.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:86: virtual void
SetReadStatus(const GURL& url, bool read) = 0;
On 2016/11/28 15:29:31, noyau wrote:
> You removed all the comments about what's happening when something changed
from
> read to unread and vice versa.
> 
> I personally prefer the old two methods API rather than a method with a bool.
It
> is more readable when used.

I changed because read/unread is now a field in the entry, and the model is just
forwarding this call to the entry (like the other SetTitle,
SetDistillationState...). The SetRead method of ReadingListEntry documents that
it will set the UpdateTime.
Also the documentation about "moving" to the top is not true anymore as the map
is unordered.

[Olivier, 2016-11-28 16:16:56]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:57: virtual const
ReadingListEntries::iterator end() const = 0;
On 2016/11/28 15:38:10, Olivier Robin wrote:
> On 2016/11/28 15:29:32, noyau wrote:
> > Can this just iterate URLs instead of exposing the map<>?
> 
> We can, I will check how to do that.

It is actually not easy as ReadingListModel is an abstract class.
We would have to create a nested ReadingListModel::iterator that is another
abstract class, then a ReadingListModelImpl::iterator that inherits it.

Another option is to keep a vector of URLs, but IMO it is worse than the current
solution.

P.S. I am not an iterator expert, so a simpler solution may exist.

[jif-google, 2016-11-28 16:28:22]

My wrist hurt from scrolling.
Overall LG, just nits.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:83: // If the |url| is in the
reading list entry(|url|).read != |read|, sets the
missing "and"?
...reading list and entry(|url).read !=...

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:118: // Allows to iterate
through read entries in the model. Must be called on a
s/Allows to iterate/Allows iterating/

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:176: return
this->entries_->at(left_url).UpdateTime() >
Note:
if this were not code that was going to be deleted, I'd prefer:
(*this->entries_)[left_url].UpdateTime
because there's no need to check the bounds (which at() does).

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:211: GURL url =
entry->URL();
const GURL& url = entry->URL();

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:212:
entries_->insert(std::make_pair(url, std::move(*entry)));
(*entries_)[url] = std::move(*entry); ?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:228: GURL url =
entry->URL();
const GURL& ?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:241:
entries_->find(url)->second = std::move(*entry);
(*entries_)[url] = std::move(*entry); ?

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:303:
entries_->insert(std::make_pair(url, std::move(entry)));
(*entries)[url] = std::move(entry);

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:315: return
entries_->at(url);
return (*entries)[url];

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:332: ReadingListEntry&
entry = entries_->at(url);
(*entries)[url]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:361: if
(!entries_->count(url)) {
How about we search the entry only once, as opposed to twice (with count() and
at())?

auto it = entries_->find(url);
if (it == entries_.end())
 return;

ReadingListEntry& entry = truchmuche(it);

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:386: if
(!entries_->count(url)) {
same trick: only search for the entry once by using find().

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:412: if
(!entries_->count(url)) {
search for the entry once by using find().

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_impl.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.h:9: #include
<unordered_map>
This is not needed.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_unittest.mm (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_unittest.mm:34: entries_->insert(
If possible, try writing
(*entries_)[GURL("http://unread_a.com")] = std::move(unread_a);
instead.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:145:
loaded_entries->insert(std::make_pair(url, std::move(*entry)));
use operator[] if possible

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:275: // for (int index = 0;
index < count; index++) {
remove

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:384: for (std::string
url_string : storage_keys) {
const std::string& url_string

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:265:
old_entry.AsReadingListSpecifics();
no need for old_entry.SetRead(true); ?

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:89: GURL
local_url(url);
I don't understand why we need to use a local GURL.
|local_url| won't outlive |url|.

[Olivier, 2016-11-28 17:58:25]

Thanks
PTAL

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:83: // If the |url| is in the
reading list entry(|url|).read != |read|, sets the
On 2016/11/28 16:28:21, jif-google wrote:
> missing "and"?
> ...reading list and entry(|url).read !=...

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:118: // Allows to iterate
through read entries in the model. Must be called on a
On 2016/11/28 16:28:21, jif-google wrote:
> s/Allows to iterate/Allows iterating/

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:176: return
this->entries_->at(left_url).UpdateTime() >
On 2016/11/28 16:28:21, jif-google wrote:
> Note:
> if this were not code that was going to be deleted, I'd prefer:
> (*this->entries_)[left_url].UpdateTime
> because there's no need to check the bounds (which at() does).

You cannot use operator[] on ReadingListEntries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:211: GURL url =
entry->URL();
On 2016/11/28 16:28:21, jif-google wrote:
> const GURL& url = entry->URL();

I need it for after the move, so I think I need a copy..

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:212:
entries_->insert(std::make_pair(url, std::move(*entry)));
On 2016/11/28 16:28:21, jif-google wrote:
> (*entries_)[url] = std::move(*entry); ?

You cannot use operator[] on entries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:228: GURL url =
entry->URL();
On 2016/11/28 16:28:21, jif-google wrote:
> const GURL& ?

I need it for after the move, so I think I need a copy..

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:241:
entries_->find(url)->second = std::move(*entry);
On 2016/11/28 16:28:21, jif-google wrote:
> (*entries_)[url] = std::move(*entry); ?

You cannot use operator[] on entries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:303:
entries_->insert(std::make_pair(url, std::move(entry)));
On 2016/11/28 16:28:21, jif-google wrote:
> (*entries)[url] = std::move(entry);

You cannot use operator[] on entries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:315: return
entries_->at(url);
On 2016/11/28 16:28:21, jif-google wrote:
> return (*entries)[url];

You cannot use operator[] on entries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:332: ReadingListEntry&
entry = entries_->at(url);
On 2016/11/28 16:28:21, jif-google wrote:
> (*entries)[url]

You cannot use operator[] on entries.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:361: if
(!entries_->count(url)) {
On 2016/11/28 16:28:21, jif-google wrote:
> How about we search the entry only once, as opposed to twice (with count() and
> at())?
> 
> auto it = entries_->find(url);
> if (it == entries_.end())
>  return;
> 
> ReadingListEntry& entry = truchmuche(it);

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:386: if
(!entries_->count(url)) {
On 2016/11/28 16:28:21, jif-google wrote:
> same trick: only search for the entry once by using find().

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:412: if
(!entries_->count(url)) {
On 2016/11/28 16:28:21, jif-google wrote:
> search for the entry once by using find().

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:145:
loaded_entries->insert(std::make_pair(url, std::move(*entry)));
On 2016/11/28 16:28:21, jif-google wrote:
> use operator[] if possible

Not possible

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:275: // for (int index = 0;
index < count; index++) {
On 2016/11/28 16:28:21, jif-google wrote:
> remove

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store.cc:384: for (std::string
url_string : storage_keys) {
On 2016/11/28 16:28:21, jif-google wrote:
> const std::string& url_string

Done.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:265:
old_entry.AsReadingListSpecifics();
On 2016/11/28 16:28:21, jif-google wrote:
> no need for old_entry.SetRead(true); ?

Done.

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:89: GURL
local_url(url);
On 2016/11/28 16:28:21, jif-google wrote:
> I don't understand why we need to use a local GURL.
> |local_url| won't outlive |url|.

Because it will be posted on another thread.
I will ask sdefresne tomorrow.

[jif, 2016-11-28 18:15:32]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_impl.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:211: GURL url =
entry->URL();
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > const GURL& url = entry->URL();
> 
> I need it for after the move, so I think I need a copy..

oh wow, did not see that.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:212:
entries_->insert(std::make_pair(url, std::move(*entry)));
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > (*entries_)[url] = std::move(*entry); ?
> 
> You cannot use operator[] on entries.

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:228: GURL url =
entry->URL();
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > const GURL& ?
> 
> I need it for after the move, so I think I need a copy..

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:241:
entries_->find(url)->second = std::move(*entry);
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > (*entries_)[url] = std::move(*entry); ?
> 
> You cannot use operator[] on entries.

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:303:
entries_->insert(std::make_pair(url, std::move(entry)));
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > (*entries)[url] = std::move(entry);
> 
> You cannot use operator[] on entries.

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:315: return
entries_->at(url);
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > return (*entries)[url];
> 
> You cannot use operator[] on entries.

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_impl.cc:332: ReadingListEntry&
entry = entries_->at(url);
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > (*entries)[url]
> 
> You cannot use operator[] on entries.

Acknowledged.

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model_unittest.mm (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model_unittest.mm:34: entries_->insert(
On 2016/11/28 16:28:21, jif-google wrote:
> If possible, try writing
> (*entries_)[GURL("http://unread_a.com")] = std::move(unread_a);
> instead.

I get it, it's not possible.

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:89: GURL
local_url(url);
On 2016/11/28 17:58:25, Olivier Robin wrote:
> On 2016/11/28 16:28:21, jif-google wrote:
> > I don't understand why we need to use a local GURL.
> > |local_url| won't outlive |url|.
> 
> Because it will be posted on another thread.
> I will ask sdefresne tomorrow.

Spoiler:
https://cs.chromium.org/chromium/src/docs/callback.md?q=callback.md&sq=packag...

So it seems you can use |url| directly.

[gambard, 2016-11-29 10:09:23]

lgtm with comments

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
File components/reading_list/ios/reading_list_model_observer.h (right):

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:43: // |index| is the
original position  and |read| the origin status. The element
|index| does not exist anymore. Update comment.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:48: // Invoked when
elements are added to the read or the unread list.
Update comments, "read or the unread list" is not relevant. When -> before
(ambiguous).
// Invoked before elements are added to the reading list.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:52: // Invoked when
elements have been added to the read or the unread list. This
Same as above.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:53: // method is
called the
sentence cut in the middle.

[Olivier, 2016-11-30 09:15:23]

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_entry.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_entry.h:35: std::unordered_map<GURL,
ReadingListEntry, GURLHasher>;
On 2016/11/28 15:38:10, Olivier Robin wrote:
> On 2016/11/28 15:29:31, noyau wrote:
> > Why is this here actually?  It is not used in this file, should it be moved
> > (along with the GURLHasher) to ReadingListModel?
> 
> If I move it, I must move it in ReadingListModel, ReadingListStore,
> ReadingListUnittests...
> 
> This is not impossible, but we will need to keep all of them in sync
(specially
> the hash function).

Moved it (replaced by std::map as performances should be similar for the small
number of entries and it avoid having GURL hasher).

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_model.h (right):

https://codereview.chromium.org/2525663002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_model.h:57: virtual const
ReadingListEntries::iterator end() const = 0;
On 2016/11/28 16:16:56, Olivier Robin wrote:
> On 2016/11/28 15:38:10, Olivier Robin wrote:
> > On 2016/11/28 15:29:32, noyau wrote:
> > > Can this just iterate URLs instead of exposing the map<>?
> > 
> > We can, I will check how to do that.
> 
> It is actually not easy as ReadingListModel is an abstract class.
> We would have to create a nested ReadingListModel::iterator that is another
> abstract class, then a ReadingListModelImpl::iterator that inherits it.
> 
> Another option is to keep a vector of URLs, but IMO it is worse than the
current
> solution.
> 
> P.S. I am not an iterator expert, so a simpler solution may exist.

After a discussion with sdefresne, it seems the standard way to do this is to
return a vector of GURL in a Keys method. I did that here.

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2525663002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:89: GURL
local_url(url);
On 2016/11/28 18:15:31, jif wrote:
> On 2016/11/28 17:58:25, Olivier Robin wrote:
> > On 2016/11/28 16:28:21, jif-google wrote:
> > > I don't understand why we need to use a local GURL.
> > > |local_url| won't outlive |url|.
> > 
> > Because it will be posted on another thread.
> > I will ask sdefresne tomorrow.
> 
> Spoiler:
>
https://cs.chromium.org/chromium/src/docs/callback.md?q=callback.md&sq=packag...
> 
> So it seems you can use |url| directly.

Done.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
File components/reading_list/ios/reading_list_model_observer.h (right):

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:43: // |index| is the
original position  and |read| the origin status. The element
On 2016/11/29 10:09:23, gambard wrote:
> |index| does not exist anymore. Update comment.

Done.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:48: // Invoked when
elements are added to the read or the unread list.
On 2016/11/29 10:09:23, gambard wrote:
> Update comments, "read or the unread list" is not relevant. When -> before
> (ambiguous).
> // Invoked before elements are added to the reading list.

Done.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:52: // Invoked when
elements have been added to the read or the unread list. This
On 2016/11/29 10:09:23, gambard wrote:
> Same as above.

Done.

https://codereview.chromium.org/2525663002/diff/120001/components/reading_lis...
components/reading_list/ios/reading_list_model_observer.h:53: // method is
called the
On 2016/11/29 10:09:23, gambard wrote:
> sentence cut in the middle.

Done.

[jif, 2016-11-30 10:25:25]

jif@chromium.org changed reviewers:
- jif@google.com

[jif, 2016-11-30 10:25:26]

lgtm

[Olivier, 2016-11-30 12:54:09]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-30 12:54:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
gambard@chromium.org, jif@chromium.org
Link to the patchset: https://codereview.chromium.org/2525663002/#ps160001
(title: "fix")

[commit-bot: I haz the power, 2016-11-30 12:54:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 13:03:57]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480510449077700,
"parent_rev": "e156c6ee0cd0186246a3ecfb956939a167d5c7b1", "commit_rev":
"41f1294715d1be94e642da6473692249bab9f0b3"}

[commit-bot: I haz the power, 2016-11-30 13:04:18]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-11-30 13:04:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-30 13:05:53]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2
Cr-Commit-Position: refs/heads/master@{#435236}
==========

[commit-bot: I haz the power, 2016-11-30 13:05:54]

Patchset 9 (id:??) landed as
https://crrev.com/448b3411da25817b84202bff7c37bbc816c4b2b2
Cr-Commit-Position: refs/heads/master@{#435236}