ReadingListDataSource returns items instead of Entries

ReadingListDataSource returns items instead of Entries

The ReadingListEntry are used in the model. The UI layer should not depend on
the ReadingListEntry. This CL move the creation of the ReadingListItems from
the view controller to the mediator.

BUG=721758, 688392
Review-Url: https://codereview.chromium.org/2885383002
Cr-Commit-Position: refs/heads/master@{#477281}
Committed: https://chromium.googlesource.com/chromium/src/+/b9e02c078fa310d0169787bb7a2187384ee4fc18

[gambard, 2017-05-17 12:09:00]

gambard@chromium.org changed reviewers:
+ stkhapugin@chromium.org

[gambard, 2017-05-17 12:09:00]

PTAL.

[gambard, 2017-06-02 14:59:05]

ping :)

[gambard, 2017-06-06 12:13:29]

gambard@chromium.org changed reviewers:
+ jif@chromium.org
- stkhapugin@chromium.org

[gambard, 2017-06-06 12:13:29]

jif@: PTAL.

[jif, 2017-06-06 13:12:27]

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:87:
// Fills section |sectionIdentifier| with the items from |array| in order.
remove "in order", it's confusing.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:98:
// Fetches the |faviconURL| of this |item|, then reconfigures it.
s/it/the item/

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_mediator.mm (right):

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:75: const
ReadingListEntry* entry = self.model->GetEntryByURL(url);
DCHECK(entry);

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:79:
readDictionary[@(entry->UpdateTime())] = item;
If two entries have the same UpdateTime(), then the first entry gets overridden.

If I were you, I'd do this:

std::vector< const ReadingListEntry*> readEntries;
std::vector< const ReadingListEntry*> unreadEntries;

for (const auto& url : self.model->Keys()) {
  // fill readEntries and unreadEntries
}

std::sort(readEntries, lambda that looks at UpdateTime());
std::sort(unreadEntries, lambda that looks at UpdateTime());

for (const auto& readEntry : readEntries) {
  [readArray addObject: ...];
}
for (const auto& unreadEntry : unreadEntries) {
  [unreadArray addObject: ...];
}

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:95: }
How about adding:
DCHECK(self.model->Keys().size() == [readArray count] + [unreadArray count]);

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:150: GURL url =
entry.URL();
can url be a "const GURL&" ?

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:169: item.title =
[title length] ? title : fullUrlString;
Slightly shorter/faster:

if ([title length]) {
  item.title = base::SysUTF8ToNSString(entry.Title());
} else {
  item.title = base::SysUTF16ToNSString(url_formatter::FormatUrl(realURL));
}

[gambard, 2017-06-06 14:12:00]

Thanks, PTAL.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:87:
// Fills section |sectionIdentifier| with the items from |array| in order.
On 2017/06/06 13:12:23, jif wrote:
> remove "in order", it's confusing.

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:98:
// Fetches the |faviconURL| of this |item|, then reconfigures it.
On 2017/06/06 13:12:23, jif wrote:
> s/it/the item/

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_mediator.mm (right):

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:75: const
ReadingListEntry* entry = self.model->GetEntryByURL(url);
On 2017/06/06 13:12:23, jif wrote:
> DCHECK(entry);

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:79:
readDictionary[@(entry->UpdateTime())] = item;
On 2017/06/06 13:12:23, jif wrote:
> If two entries have the same UpdateTime(), then the first entry gets
overridden.
> 
> If I were you, I'd do this:
> 
> std::vector< const ReadingListEntry*> readEntries;
> std::vector< const ReadingListEntry*> unreadEntries;
> 
> for (const auto& url : self.model->Keys()) {
>   // fill readEntries and unreadEntries
> }
> 
> std::sort(readEntries, lambda that looks at UpdateTime());
> std::sort(unreadEntries, lambda that looks at UpdateTime());
> 
> for (const auto& readEntry : readEntries) {
>   [readArray addObject: ...];
> }
> for (const auto& unreadEntry : unreadEntries) {
>   [unreadArray addObject: ...];
> }

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:95: }
On 2017/06/06 13:12:25, jif wrote:
> How about adding:
> DCHECK(self.model->Keys().size() == [readArray count] + [unreadArray count]);

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:150: GURL url =
entry.URL();
On 2017/06/06 13:12:23, jif wrote:
> can url be a "const GURL&" ?

Done.

https://codereview.chromium.org/2885383002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:169: item.title =
[title length] ? title : fullUrlString;
On 2017/06/06 13:12:24, jif wrote:
> Slightly shorter/faster:
> 
> if ([title length]) {
>   item.title = base::SysUTF8ToNSString(entry.Title());
> } else {
>   item.title = base::SysUTF16ToNSString(url_formatter::FormatUrl(realURL));
> }

Done.

[jif, 2017-06-06 14:17:16]

lgtm

[gambard, 2017-06-06 14:19:44]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-06-06 14:19:54]

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, 2017-06-06 14:29:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496758784534720,
"parent_rev": "1c3ab685dd76d567e213ac9c59c17596e780e962", "commit_rev":
"b9e02c078fa310d0169787bb7a2187384ee4fc18"}

[commit-bot: I haz the power, 2017-06-06 14:29:30]

Description was changed from

==========
ReadingListDataSource returns items instead of Entries

The ReadingListEntry are used in the model. The UI layer should not depend on
the ReadingListEntry. This CL move the creation of the ReadingListItems from
the view controller to the mediator.

BUG=721758, 688392
==========

to

==========
ReadingListDataSource returns items instead of Entries

The ReadingListEntry are used in the model. The UI layer should not depend on
the ReadingListEntry. This CL move the creation of the ReadingListItems from
the view controller to the mediator.

BUG=721758, 688392

Review-Url: https://codereview.chromium.org/2885383002
Cr-Commit-Position: refs/heads/master@{#477281}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9e02c078fa310d0169787bb7a21...
==========

[commit-bot: I haz the power, 2017-06-06 14:29:30]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b9e02c078fa310d0169787bb7a21...