Move the ReadingListModel in a Mediator

Move the ReadingListModel in a Mediator

The ReadingListMediator will now handle the ReadingListModel instead of
passing it directly in the ReadingListCollectionViewController.

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

[gambard, 2017-05-15 09:14:29]

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

[gambard, 2017-05-15 09:14:30]

PTAL.

[stkhapugin, 2017-05-15 15:15:53]

mostly looking good, with some optional nits and a few actually important
questions

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h
(right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:95:
@property(nonatomic, readonly)
While you're at it, can you please add an explicit storage classifier for this
pointer (strong/weak)? This will make every property in this file have a clear
ownership.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:350:
// Ignore model updates when the view controller is doing batch updates.
here and everywhere, I think you want to s/model/dataSource in comments/method
names

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:525:
for (const auto& url : [self.dataSource keys]) {
optional nit: You might be able to use dot syntax: self.dataSource.keys

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_data_sink.h (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_sink.h:13: -
(void)dataSourceReady;
The usual pattern for Cocoa callbacks is to pass the caller as one of the
parameters:

- (void)dataSourceReady:(id)datasource;
- (void)readingListModelDidApplyChanges:(id)model;

Also it's unclear why you use "dataSource" in the first method, but "Model" in
the other two.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_data_source.h (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:22:
@property(nonatomic, readonly, assign) BOOL ready;
(optional), good style for BOOL properties is:

@property (..., getter=isReady) BOOL ready; // Allows both "foo.ready" and "[foo
isReady]"

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:24:
@property(nonatomic, readonly, assign) BOOL hasElements;
if you don't actually plan to have a backing ivar for this and only ever expose
a getter, just expose a method instead: 

- (BOOL) hasElements;

I expect the implementation of this to be "return xxx.count > 0;" or something
similar; if that's the case - just make a method instead.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:28:
@property(nonatomic, weak, nullable) id<ReadingListDataSink> dataSink;
Thanks for nullability!

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_mediator.mm (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:35: 
optional nit: remove newline

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:44:
self.model->MarkAllSeen();
Why removing data sink causes items to be marked as seen? This is bizzare. It
looks as a weird side effect, and you probably want to mark seen explicitly from
where you call -removeDataSink in order to get this side effect for cleaner
code.

[gambard, 2017-05-16 12:46:29]

Thanks, PTAL.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h
(right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:95:
@property(nonatomic, readonly)
On 2017/05/15 15:15:53, stkhapugin wrote:
> While you're at it, can you please add an explicit storage classifier for this
> pointer (strong/weak)? This will make every property in this file have a clear
> ownership.

Done.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm
(right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:350:
// Ignore model updates when the view controller is doing batch updates.
On 2017/05/15 15:15:53, stkhapugin wrote:
> here and everywhere, I think you want to s/model/dataSource in comments/method
> names

Done.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:525:
for (const auto& url : [self.dataSource keys]) {
On 2017/05/15 15:15:53, stkhapugin wrote:
> optional nit: You might be able to use dot syntax: self.dataSource.keys

The idea is to show that it is not a property. Anyway, this will be moved to the
data source in a following CL.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_data_sink.h (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_sink.h:13: -
(void)dataSourceReady;
On 2017/05/15 15:15:53, stkhapugin wrote:
> The usual pattern for Cocoa callbacks is to pass the caller as one of the
> parameters:
> 
> - (void)dataSourceReady:(id)datasource;
> - (void)readingListModelDidApplyChanges:(id)model;
> 
> Also it's unclear why you use "dataSource" in the first method, but "Model" in
> the other two. 

Done.
The other two methods will be merged and renamed. For now I want to make clear
that they are just proxy for the ReadingListObserver methods.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_data_source.h (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:22:
@property(nonatomic, readonly, assign) BOOL ready;
On 2017/05/15 15:15:53, stkhapugin wrote:
> (optional), good style for BOOL properties is:
> 
> @property (..., getter=isReady) BOOL ready; // Allows both "foo.ready" and
"[foo
> isReady]"

Acknowledged.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:24:
@property(nonatomic, readonly, assign) BOOL hasElements;
On 2017/05/15 15:15:53, stkhapugin wrote:
> if you don't actually plan to have a backing ivar for this and only ever
expose
> a getter, just expose a method instead: 
> 
> - (BOOL) hasElements;
> 
> I expect the implementation of this to be "return xxx.count > 0;" or something
> similar; if that's the case - just make a method instead. 

Done.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_data_source.h:28:
@property(nonatomic, weak, nullable) id<ReadingListDataSink> dataSink;
On 2017/05/15 15:15:53, stkhapugin wrote:
> Thanks for nullability!

:)

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/reading_list_mediator.mm (right):

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:35: 
On 2017/05/15 15:15:53, stkhapugin wrote:
> optional nit: remove newline

Done.

https://codereview.chromium.org/2879183002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/reading_list_mediator.mm:44:
self.model->MarkAllSeen();
On 2017/05/15 15:15:53, stkhapugin wrote:
> Why removing data sink causes items to be marked as seen? This is bizzare. It
> looks as a weird side effect, and you probably want to mark seen explicitly
from
> where you call -removeDataSink in order to get this side effect for cleaner
> code.

Done.

[stkhapugin, 2017-05-16 12:57:06]

lgtm

https://codereview.chromium.org/2879183002/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/2879183002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:345:
// Ignore model updates when the view controller is doing batch updates.
comments seems wrong

[gambard, 2017-05-16 15:16:21]

Thanks!

https://codereview.chromium.org/2879183002/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/2879183002/diff/20001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:345:
// Ignore model updates when the view controller is doing batch updates.
On 2017/05/16 12:57:05, stkhapugin wrote:
> comments seems wrong

Done.

[gambard, 2017-05-16 15:16:36]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-05-16 15:16:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org
Link to the patchset: https://codereview.chromium.org/2879183002/#ps40001
(title: "Address comment")

[commit-bot: I haz the power, 2017-05-16 15:17:05]

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-05-16 20:29:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494947796638870,
"parent_rev": "b323ccf05a36077b39dd990685abc2e4473cb6be", "commit_rev":
"f6504f2aa7bf29f529761e859590527c6449c301"}

[commit-bot: I haz the power, 2017-05-16 20:30:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494947796638870,
"parent_rev": "bc0c86d1155575d8a8688aab51517c69e0d3ba8c", "commit_rev":
"0c5ee3cba18800bf7664aaef43afd379e5c9a217"}

[commit-bot: I haz the power, 2017-05-16 20:30:34]

Description was changed from

==========
Move the ReadingListModel in a Mediator

The ReadingListMediator will now handle the ReadingListModel instead of
passing it directly in the ReadingListCollectionViewController.

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

to

==========
Move the ReadingListModel in a Mediator

The ReadingListMediator will now handle the ReadingListModel instead of
passing it directly in the ReadingListCollectionViewController.

BUG=721758, 688392

Review-Url: https://codereview.chromium.org/2879183002
Cr-Commit-Position: refs/heads/master@{#472205}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c5ee3cba18800bf7664aaef43af...
==========

[commit-bot: I haz the power, 2017-05-16 20:30:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0c5ee3cba18800bf7664aaef43af...