Create ReadingListCoordinator

Create ReadingListCoordinator

This CL creates a ReadingListCoordinator, to manage the ViewController.

BUG=688392, 693591
Review-Url: https://codereview.chromium.org/2693863005
Cr-Commit-Position: refs/heads/master@{#451747}
Committed: https://chromium.googlesource.com/chromium/src/+/6299cc1d8fc24c3fd382b6dbe615a58381b73af0

[gambard, 2017-02-16 10:42:46]

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

[gambard, 2017-02-16 10:42:47]

PTAL.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_commands.h (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_commands.h:12: @protocol
ReadingListCommands
This protocol might be merged with ReadingListCollectionViewControllerDelegate.
Or kept separate if the ReadingListCollectionViewController get a proper
coordinator.

[Olivier, 2017-02-16 15:16:09]

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4234:
_readingListCoordinator.reset([[ReadingListCoordinator alloc]
SHould this be released at some point?

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h
(right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:28:
- (void)readingListHasItem:(BOOL)hasItems;
add s to Item

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_coordinator.h (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_coordinator.h:24:
initWithBaseViewController:(nullable UIViewController*)viewController
What is the point of a coordinator without baseViewController?

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_coordinator.h:25:
browserState:(nonnull ios::ChromeBrowserState*)browserState
I think it is better to pass the service you depend on rather than the
browserState. Is it possible here?

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:100:
[self.commandHandler dismissReadingList];
Should this call [readingListCollectionViewController willBeDismissed]; ?

[gambard, 2017-02-16 15:45:56]

Thanks, PTAL.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4234:
_readingListCoordinator.reset([[ReadingListCoordinator alloc]
On 2017/02/16 15:16:09, Olivier Robin wrote:
> SHould this be released at some point?

What do you mean? It is a scoped_nsobject.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h
(right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:28:
- (void)readingListHasItem:(BOOL)hasItems;
On 2017/02/16 15:16:09, Olivier Robin wrote:
> add s to Item

Done.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_coordinator.h (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_coordinator.h:24:
initWithBaseViewController:(nullable UIViewController*)viewController
On 2017/02/16 15:16:09, Olivier Robin wrote:
> What is the point of a coordinator without baseViewController?

Test for example. And the baseViewController is only used to display the
content. If it is nil it won't display the reading list but it won't crash.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_coordinator.h:25:
browserState:(nonnull ios::ChromeBrowserState*)browserState
On 2017/02/16 15:16:09, Olivier Robin wrote:
> I think it is better to pass the service you depend on rather than the
> browserState. Is it possible here?

It seems that the pattern is to pass the BrowserState to coordinators and the
coordinators distill them to their ViewController.
You can ask mark for more details.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:100:
[self.commandHandler dismissReadingList];
On 2017/02/16 15:16:09, Olivier Robin wrote:
> Should this call [readingListCollectionViewController willBeDismissed]; ?

Well, it might not know about readingListCollectionViewController. And it does
not have a delegate for now.
But I am thinking of merging the two protocols.

[Olivier, 2017-02-17 14:12:34]

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4234:
_readingListCoordinator.reset([[ReadingListCoordinator alloc]
On 2017/02/16 15:45:56, gambard wrote:
> On 2017/02/16 15:16:09, Olivier Robin wrote:
> > SHould this be released at some point?
> 
> What do you mean? It is a scoped_nsobject.

If you show the screen, it will only be dealloced the next time you open it.
Should this scoped pointer be reset on dismiss?

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:100:
[self.commandHandler dismissReadingList];
On 2017/02/16 15:45:56, gambard wrote:
> On 2017/02/16 15:16:09, Olivier Robin wrote:
> > Should this call [readingListCollectionViewController willBeDismissed]; ?
> 
> Well, it might not know about readingListCollectionViewController. And it does
> not have a delegate for now.
> But I am thinking of merging the two protocols.

When you pressed FINISHED button, 
|dismissReadingListCollectionViewController| calls  
[readingListCollectionViewController willBeDismissed];

Shouldn't this gesture (and the escape key) be equivalent?

[gambard, 2017-02-17 15:46:48]

Description was changed from

==========
Create ReadingListCoordinator

This CL creates a ReadingListCoordinator, to manage the ViewController.

BUG=688392
==========

to

==========
Create ReadingListCoordinator

This CL creates a ReadingListCoordinator, to manage the ViewController.

BUG=688392, 693591
==========

[gambard, 2017-02-17 15:47:58]

Thanks, PTAL.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4234:
_readingListCoordinator.reset([[ReadingListCoordinator alloc]
On 2017/02/17 14:12:34, Olivier Robin wrote:
> On 2017/02/16 15:45:56, gambard wrote:
> > On 2017/02/16 15:16:09, Olivier Robin wrote:
> > > SHould this be released at some point?
> > 
> > What do you mean? It is a scoped_nsobject.
> 
> If you show the screen, it will only be dealloced the next time you open it.
> Should this scoped pointer be reset on dismiss?

Yes, but we don't want BVC to handle all coordinator I think.
I will add it to "received memory warning" and clear state on stop.

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right):

https://codereview.chromium.org/2693863005/diff/60001/ios/chrome/browser/ui/r...
ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:100:
[self.commandHandler dismissReadingList];
On 2017/02/17 14:12:34, Olivier Robin wrote:
> On 2017/02/16 15:45:56, gambard wrote:
> > On 2017/02/16 15:16:09, Olivier Robin wrote:
> > > Should this call [readingListCollectionViewController willBeDismissed]; ?
> > 
> > Well, it might not know about readingListCollectionViewController. And it
does
> > not have a delegate for now.
> > But I am thinking of merging the two protocols.
> 
> When you pressed FINISHED button, 
> |dismissReadingListCollectionViewController| calls  
> [readingListCollectionViewController willBeDismissed];
> 
> Shouldn't this gesture (and the escape key) be equivalent?

Yes. This is a bug: crbug.com/693591

[Olivier, 2017-02-20 16:34:02]

LGTM

Please at Todos for the bugs you filed.

[gambard, 2017-02-20 16:37:15]

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

[gambard, 2017-02-20 16:37:15]

+marq@ as BVC owner

[marq (ping after 24h), 2017-02-21 11:29:28]

BVC changes LGTM

[gambard, 2017-02-21 12:07:38]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-02-21 12:07:52]

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-02-21 13:06:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487678858530810,
"parent_rev": "ad615573268058885e7e43a15196e5936dc6237f", "commit_rev":
"6299cc1d8fc24c3fd382b6dbe615a58381b73af0"}

[commit-bot: I haz the power, 2017-02-21 13:06:41]

Description was changed from

==========
Create ReadingListCoordinator

This CL creates a ReadingListCoordinator, to manage the ViewController.

BUG=688392, 693591
==========

to

==========
Create ReadingListCoordinator

This CL creates a ReadingListCoordinator, to manage the ViewController.

BUG=688392, 693591

Review-Url: https://codereview.chromium.org/2693863005
Cr-Commit-Position: refs/heads/master@{#451747}
Committed:
https://chromium.googlesource.com/chromium/src/+/6299cc1d8fc24c3fd382b6dbe615...
==========

[commit-bot: I haz the power, 2017-02-21 13:06:44]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/6299cc1d8fc24c3fd382b6dbe615...