[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

In preparation for back-porting UIViewControllers in the NTP, add support for
loading contentViewControllers inside the clean NTP's scroll view.

This also adds stubs for the NTP mediator and consumer, as well as fix
some navigation errors when loading and unloading the NTP.

BUG=

Review-Url: https://codereview.chromium.org/2785893003
Cr-Commit-Position: refs/heads/master@{#462914}
Committed: https://chromium.googlesource.com/chromium/src/+/6c0f9edbe4a39bdc26e89254a205ac477ca4b902

[justincohen, 2017-03-30 05:26:06]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 05:26:20]

Dry run: 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-03-30 05:38:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 05:38:47]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[justincohen, 2017-04-04 19:45:46]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:46:18]

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

[justincohen, 2017-04-04 20:04:15]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 20:05:39]

Dry run: 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-04-04 20:17:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 20:17:47]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[justincohen, 2017-04-04 22:29:02]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 22:29:47]

Dry run: 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-04-05 01:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 01:47:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[marq (ping after 24h), 2017-04-05 09:46:04]

marq@chromium.org changed reviewers:
+ lpromero@chromium.org, marq@chromium.org, rohitrao@chromium.org

[marq (ping after 24h), 2017-04-05 09:46:05]

Adding the usual suspects as reviewers or on CC.

[marq (ping after 24h), 2017-04-05 12:22:49]

I think it would be better to not include the incognito stuff in this CL. The
general thinking around Incognito is that we won't mix incognito and
non-incognito code like this.

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
File ios/chrome/browser/ui/ntp/new_tab_page_view.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:83: if
(self.translatesAutoresizingMaskIntoConstraints) {
It's not clear to my why this is conditional here. Can you update the comment?

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:110: [self setFrame:self.frame];
This feels deeply wrong. Why is it needed?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:20:
@property(nonatomic, strong) UIViewController* viewController;
The base class already has this as a property. Normally coordinators redeclare
it to be of a more specific UIVC subclass.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:22:
BookmarkHomeTabletNTPController* wrapperViewController;
Comment for this property.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:30: //
PLACEHOLDER: Re-using old view controllers for now.
More of a HACK:

(placeholder is a throw-away implementation; a hack is ugly to-be-fixed code
that gets an old-architecture feature working in the new architecture).

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:36:
[self.context.baseViewController presentViewController:self.viewController
I think our preferred technique is for the parent coordinator to take care of
presenting/embedding in -childCoordinatorDidStart.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:46:
self.viewController = [[UIViewController alloc] init];
Does this ever get presented or added anywhere?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/ntp_commands.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // ======                
       New Architecture                         =====
Remove the banner.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:15: // Start a Chrome Home
panel.
s/start/show everywhere?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:10: // Tell NTP what tab
bar items to display.
"Tab bar" is kind of confusing because these are tabs in the UITabBar sense, not
tabs in the Chrome sense.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:11: -
(void)setTabBarItems:(NSMutableArray*)item;
Generic on the array?

Name the parameter 'items', since it's a collection.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h:20: // Calling code
should assign this before starting this coordinator.
We've moved to most classes which display webState stuff just looking at the
WebStateList in Browser and showing the active WebState, instead of explicitly
pushing a specific WebState down to everything.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:37:
self.viewController.ntpCommandHandler = self;
Any reason for these commands not to go through the dispatcher?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:43: for
(BrowserCoordinator* child in self.children) {
Does this belong in the superclass?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:48: -
(void)startNTPHomePanel {
#pragma mark - NTPCommands

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:72: if
(IsIPadIdiom()) {
The coordinator shouldn't care about what device size this is.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:46: 
#pragma mark - 

for each adopted protocol.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:47: -
(void)loadURL:(const GURL&)url
Move this to a mediator?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:30: self.consumer =
consumer;
_consumer = consumer;

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:15:
@property(nonatomic, weak) id<NTPCommands> ntpCommandHandler;
just 'commandHandler' is fine, or (more likely) 'dispatcher'.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:23: @private
no need for @private here.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:29:
@property(nonatomic, strong) NewTabPageView* ntpView;
NTPView, or NewTabView, or something.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:30:
@property(nonatomic, retain) NSArray* tabBarItems;
s/retain/strong/ in ARC-land.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:63: [self.view
addSubview:_ntpView];
s/_ntpView/self.ntpView everywhere, for consistency and easy of future
refactoring.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:72:
self.ntpView.tabBar.items = _tabBarItems;
self.tabBarItems?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:74: 
#pragma mark - NTPConsumer

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:76: _tabBarItems
= items;
Should this just directly set them on self.ntpView.tabBar?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:85: if
(IsIPadIdiom()) {
Please use size classes instead of IsIPadIdiom().

[justincohen, 2017-04-05 19:28:25]

Removed incognito.  Not sure what the best approach is if I need to remove
device checks in lieu of size classes, although certainly for the hack approach
I can swap it out no problem.  Most of the changes are done, some of them
responded with questions.   Thanks for the review!

PTAL!

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
File ios/chrome/browser/ui/ntp/new_tab_page_view.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:83: if
(self.translatesAutoresizingMaskIntoConstraints) {
On 2017/04/05 12:22:47, marq wrote:
> It's not clear to my why this is conditional here. Can you update the comment?

Added a comment explaining why.

https://codereview.chromium.org/2785893003/diff/80001/ios/chrome/browser/ui/n...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:110: [self setFrame:self.frame];
On 2017/04/05 12:22:47, marq wrote:
> This feels deeply wrong. Why is it needed?

Added a comment explaining why.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:20:
@property(nonatomic, strong) UIViewController* viewController;
On 2017/04/05 12:22:48, marq wrote:
> The base class already has this as a property. Normally coordinators redeclare
> it to be of a more specific UIVC subclass.

The base class is read only?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:22:
BookmarkHomeTabletNTPController* wrapperViewController;
On 2017/04/05 12:22:48, marq wrote:
> Comment for this property.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:30: //
PLACEHOLDER: Re-using old view controllers for now.
On 2017/04/05 12:22:48, marq wrote:
> More of a HACK:
> 
> (placeholder is a throw-away implementation; a hack is ugly to-be-fixed code
> that gets an old-architecture feature working in the new architecture).

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:36:
[self.context.baseViewController presentViewController:self.viewController
On 2017/04/05 12:22:48, marq wrote:
> I think our preferred technique is for the parent coordinator to take care of
> presenting/embedding in -childCoordinatorDidStart.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:46:
self.viewController = [[UIViewController alloc] init];
On 2017/04/05 12:22:47, marq wrote:
> Does this ever get presented or added anywhere?

Yes, it gets added in the NTP iPad scroll view.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/ntp_commands.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // ======                
       New Architecture                         =====
On 2017/04/05 12:22:48, marq wrote:
> Remove the banner.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:15: // Start a Chrome Home
panel.
On 2017/04/05 12:22:48, marq wrote:
> s/start/show everywhere?

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:10: // Tell NTP what tab
bar items to display.
On 2017/04/05 12:22:48, marq wrote:
> "Tab bar" is kind of confusing because these are tabs in the UITabBar sense,
not
> tabs in the Chrome sense. 

removed the word tab.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:11: -
(void)setTabBarItems:(NSMutableArray*)item;
On 2017/04/05 12:22:48, marq wrote:
> Generic on the array?
> 
> Name the parameter 'items', since it's a collection.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h:20: // Calling code
should assign this before starting this coordinator.
On 2017/04/05 12:22:48, marq wrote:
> We've moved to most classes which display webState stuff just looking at the
> WebStateList in Browser and showing the active WebState, instead of explicitly
> pushing a specific WebState down to everything.

What if this NTP does not represent the active web state?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:37:
self.viewController.ntpCommandHandler = self;
On 2017/04/05 12:22:48, marq wrote:
> Any reason for these commands not to go through the dispatcher? 

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:43: for
(BrowserCoordinator* child in self.children) {
On 2017/04/05 12:22:48, marq wrote:
> Does this belong in the superclass?

tab coordinator currently does the same thing.  Is there any reason this
shouldn't go in BrowserCoordinator?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:48: -
(void)startNTPHomePanel {
On 2017/04/05 12:22:48, marq wrote:
> #pragma mark - NTPCommands

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:72: if
(IsIPadIdiom()) {
On 2017/04/05 12:22:48, marq wrote:
> The coordinator shouldn't care about what device size this is. 

Who should handle the differences in presentation style for the NTP.  In iPad we
embed a view controller inside a scroll view.  On iPhone we use
-presentViewController?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:46: 
On 2017/04/05 12:22:48, marq wrote:
> #pragma mark - 
> 
> for each adopted protocol.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:47: -
(void)loadURL:(const GURL&)url
On 2017/04/05 12:22:48, marq wrote:
> Move this to a mediator?

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:30: self.consumer =
consumer;
On 2017/04/05 12:22:48, marq wrote:
> _consumer = consumer;

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:15:
@property(nonatomic, weak) id<NTPCommands> ntpCommandHandler;
On 2017/04/05 12:22:48, marq wrote:
> just 'commandHandler' is fine, or (more likely) 'dispatcher'.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:23: @private
On 2017/04/05 12:22:49, marq wrote:
> no need for @private here.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:29:
@property(nonatomic, strong) NewTabPageView* ntpView;
On 2017/04/05 12:22:48, marq wrote:
> NTPView, or NewTabView, or something.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:30:
@property(nonatomic, retain) NSArray* tabBarItems;
On 2017/04/05 12:22:48, marq wrote:
> s/retain/strong/ in ARC-land.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:63: [self.view
addSubview:_ntpView];
On 2017/04/05 12:22:48, marq wrote:
> s/_ntpView/self.ntpView everywhere, for consistency and easy of future
> refactoring.

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:72:
self.ntpView.tabBar.items = _tabBarItems;
On 2017/04/05 12:22:48, marq wrote:
> self.tabBarItems? 

I could create a property and override the setter to call
self.NTPView.tabBar.items... but it only gets set here, once.  Why bother?

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:74: 
On 2017/04/05 12:22:49, marq wrote:
> #pragma mark - NTPConsumer

Done.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:76: _tabBarItems
= items;
On 2017/04/05 12:22:49, marq wrote:
> Should this just directly set them on self.ntpView.tabBar?

No, that doesn't exist yet.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:85: if
(IsIPadIdiom()) {
On 2017/04/05 12:22:49, marq wrote:
> Please use size classes instead of IsIPadIdiom().

I could, but that would require a number of UI changes that I think are outside
the scope of this CL.

Currently on iPhone 7+ we still show 'phone' UI.

Secondly, since our phone UI and tablet UI are so different, if were to view the
NTP in landscape on a 7+, swipe to bookmarks, and then rotate to a phone, we
would have to rebuild the NTP and pop a view controller for phone.  In reverse,
if you were to view the NTP on a phone in portrait, tap bookmarks, and then
rotate to landscape, you would have to dismiss the popup, rebuild the NTP, and
scroll to bookmarks.

What do you suggest?

[justincohen, 2017-04-05 19:36:28]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 19:36:53]

Dry run: 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-04-05 20:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 20:42:57]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-04-06 07:58:04]

Description was changed from

==========
[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

In preparation for back-porting UIViewControllers in the NTP, add support for
loading contentViewControllers inside the clean NTP's scroll view.

This also adds stubs for the NTP mediator and consumer, as well as fix some
some navigation errors when loading and unloading the NTP.

BUG=
==========

to

==========
[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

In preparation for back-porting UIViewControllers in the NTP, add support for
loading contentViewControllers inside the clean NTP's scroll view.

This also adds stubs for the NTP mediator and consumer, as well as fix
some navigation errors when loading and unloading the NTP.

BUG=
==========

[lpromero, 2017-04-06 13:01:06]

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:43: for
(BrowserCoordinator* child in self.children) {
On 2017/04/05 19:28:24, justincohen wrote:
> On 2017/04/05 12:22:48, marq wrote:
> > Does this belong in the superclass?
> 
> tab coordinator currently does the same thing.  Is there any reason this
> shouldn't go in BrowserCoordinator?

I am thinking about moving it there.

https://codereview.chromium.org/2785893003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/ntp/new_tab_page_view.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:87: // |-layoutSubviews| is not
successfully triggered when |-setNeedsLayout| is
Reflow the comment.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/BUILD.gn:14: "//ios/chrome/browser/ui:ui",
No need to repeat the target name when it's the folder name. Here and below
twice.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:8: #import
<UIKit/UIKit.h>
Add new line.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:11: @interface
BookmarksCoordinator : BrowserCoordinator
Comment.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:12: @end
Please add a unit tests file and a simple test that the view controller is set
up correctly when started.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:22:
BookmarkHomeTabletNTPController* wrapperViewController;
This class is not a view controller. Consider renaming the property.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/ntp_commands.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // Command protocol for
commands relating to the Settings UI.
s/Settings/NTP

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:11: // Start a Chrome Home
panel.
s/Start/Starts here and below.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:13: // Start a bookmarks
panel on iPad or present a bookmarks panel on iPhone.
s/present/presents

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:13: // Start a bookmarks
panel on iPad or present a bookmarks panel on iPhone.
What does Start mean in this context? Start from coordinators? Presentation is
also a kind of coordinator start.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/BUILD.gn:22: "//ios/chrome/browser/ui:ui",
Idem for not repeating the target when it can be implicit. I wish gn gen
produced a better error message when missing a deps.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/BUILD.gn:27:
"//ios/clean/chrome/browser/ui/open_tabs/",
Remove the trailing slash.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:7: 
#import Foundation

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:8: @class
NewTabPageBarItem;
Add a new line.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:11: // Tell NTP what bar
items to display.
*Tells

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:12: -
(void)setBarItems:(NSMutableArray<NewTabPageBarItem*>*)items;
Does it need to be mutable?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:106: if
(IsIPadIdiom()) {
Remove this section as it is done in childCoordinatorDidStart.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_HOME_COORDINATOR_H_
Please add a unittests file.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:15: @interface
NTPHomeCoordinator : BrowserCoordinator
Comment.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:20:
@property(nonatomic, strong) GoogleLandingController* wrapperViewController;
This is not a view controller. Should the property be renamed?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_HOME_MEDIATOR_H_
Please add a unittests file.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:13: @interface
NTPHomeMediator : NSObject<UrlLoader, OmniboxFocuser>
Comment.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm:1: 
Remove empty line.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_MEDIATOR_H_
Please add a unittests file.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h:12: @interface
NTPMediator : NSObject
Comments.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:1: 
Remove empty line.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:33: 
Remove empty line.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:64: [self.consumer
setBarItems:tabBarItems];
[tabBarItems copy]

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:16: // The
dispatcher for this view controller
Add more new lines to this interface.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:19: // Add a
Chrome Home panel.
*Adds, here and below.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:24: -
(void)addOpenTabsViewController:(UIViewController*)controller;
Please add unittests for these methods. Or a Showcase EarlGrey test.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:73:
self.NTPView.tabBar.items = _tabBarItems;
Should you DCHECK that _tabBarItems is set? Because setBarItems doesn't set the
NTPView.tabBar.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:130:
_tabBarItems = items;
This supposes it is called before the view is loaded.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:135: -
(void)scrollViewDidScroll:(UIScrollView*)scrollView {
Could this logic be in a separate object whose single responsibility would be
driving handling this call? Seems the only knowledge it needs is the tabBar.
This object would then be directly the scrollview delegate.
NewTabPageScrollViewDelegate class?
This file will become exceedingly long if we don't split the logic.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:172:
changePanel:(BOOL)changePanel {
This is unused. Will it stay unused?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/open_tabs/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/open_tabs/BUILD.gn:5: source_set("open_tabs") {
Should it be "opened_tabs"?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/open_tabs/open_tabs_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/open_tabs/open_tabs_coordinator.h:11: @interface
OpenTabsCoordinator : BrowserCoordinator
Please add comments, as it is unclear what this coordinates: does it open tabs,
is it a list of opened tabs?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:84: // contentViewController
when it starts afterwards.
Should we then delay starting the webCoordinator then, instead of resetting the
NTP view controller?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:105: [self.viewController
setContentViewController:nil];
Use self.viewController.contentViewController = nil to keep the style
consistent.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:105: [self.viewController
setContentViewController:nil];
Why this call? Seems like you expect a side effect? It should be documented.
I think you should rather implement childCoordinatorWillStop and based on the
type of the coordinator, reset the content view controller. (Or maybe not even
care about the stopped child type and reset the contentViewController.)

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:17:
"//ios/chrome/browser:browser",
No need to repeat browser.

[rohitrao (ping after 24h), 2017-04-06 13:08:13]

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:44:
self.viewController.view = [self.wrapperViewController view];
Is it ok to assign to |view| like this, or should we subclass and implement
loadView?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/ntp_commands.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // Command protocol for
commands relating to the Settings UI.
Update comment to not reference Settings.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:53: for
(BrowserCoordinator* child in self.children) {
Copy the comment from ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:

  // PLACEHOLDER: Stop child coordinators here for now. We might deal with this
  // differently later on.

Might even be worth filing a bug to come to a decision here and convert these to
TODOs.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:59: -
(void)childCoordinatorDidStart:(BrowserCoordinator*)coordinator {
Can you briefly sketch out the lifecycles of the various child coordinators? 
Does the BookmarksCoordinator always exist, or is it created only when the user
taps on the button or tries to swipe over to that panel?

When you swipe away from the bookmarks panel on tablet, do we stop and destroy
the BookmarksCoordinator, or does it stay alive forever?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:15: @interface
NTPHomeCoordinator : BrowserCoordinator
Filenames should match the class names, either use NewTabPage or NTP everywhere.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:105:
NewTabPageBarItem* item = self.NTPView.tabBar.items[1];
This is weird -- we get a set of items from the mediator, but then we make
assumptions about exactly what that set contains, in what order.  We should try
to refactor this so that the mediator hands the VC an opaque blob of data, and
the VC displays it accordingly but doesn't otherwise need to know what's in it.

[marq (ping after 24h), 2017-04-06 14:30:14]

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:72:
self.ntpView.tabBar.items = _tabBarItems;
On 2017/04/05 19:28:24, justincohen wrote:
> On 2017/04/05 12:22:48, marq wrote:
> > self.tabBarItems? 
> 
> I could create a property and override the setter to call
> self.NTPView.tabBar.items... but it only gets set here, once.  Why bother?

I meant that you have a tabBarItems property, so it's cleaner (IMO) to use it
here instead of _tabBarItems.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:85: if
(IsIPadIdiom()) {
On 2017/04/05 19:28:24, justincohen wrote:
> On 2017/04/05 12:22:49, marq wrote:
> > Please use size classes instead of IsIPadIdiom().
> 
> I could, but that would require a number of UI changes that I think are
outside
> the scope of this CL.
> 
> Currently on iPhone 7+ we still show 'phone' UI.
> 
> Secondly, since our phone UI and tablet UI are so different, if were to view
the
> NTP in landscape on a 7+, swipe to bookmarks, and then rotate to a phone, we
> would have to rebuild the NTP and pop a view controller for phone.  In
reverse,
> if you were to view the NTP on a phone in portrait, tap bookmarks, and then
> rotate to landscape, you would have to dismiss the popup, rebuild the NTP, and
> scroll to bookmarks.
> 
> What do you suggest?

We can hold off on any change for now; I think it's more important to get all of
the size/format logic into the UI layer first, and then we can see about
cleaning up idiom vs. size classes.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:44:
self.viewController.view = [self.wrapperViewController view];
On 2017/04/06 13:08:13, rohitrao wrote:
> Is it ok to assign to |view| like this, or should we subclass and implement
> loadView?

|view| is a public writable property, so implicitly this is OK. Note that
setting |view| will not trigger |viewDidLoad| as far as I know.

However, it is worth noting that the UIViewController docs say "Each view
controller object is the sole owner of its view. You must not associate the same
view object with multiple view controller objects. The only exception to this
rule is that a container view controller implementation may add this view as a
subview in its own view hierarchy."

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h:21:
@property(nonatomic, assign) web::WebState* webState;
Is this only used to pass on to the home coordinator?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:16: // Calling code
should assign this before starting this coordinator.
This isn't a coordinator.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm:31:
self.webState->GetNavigationManager()->LoadURLWithParams(params);
I don't think we want to plumb the webstate down to this mediator just so it can
open URLs. Maybe opening URLs (in the same tab) should be a command that the tab
coordinator handles?

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:135: -
(void)scrollViewDidScroll:(UIScrollView*)scrollView {
On 2017/04/06 13:01:06, lpromero wrote:
> Could this logic be in a separate object whose single responsibility would be
> driving handling this call? Seems the only knowledge it needs is the tabBar.
> This object would then be directly the scrollview delegate.
> NewTabPageScrollViewDelegate class?
> This file will become exceedingly long if we don't split the logic.

+1

[justincohen, 2017-04-06 18:25:11]

ptal

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:43: for
(BrowserCoordinator* child in self.children) {
On 2017/04/06 13:01:04, lpromero wrote:
> On 2017/04/05 19:28:24, justincohen wrote:
> > On 2017/04/05 12:22:48, marq wrote:
> > > Does this belong in the superclass?
> > 
> > tab coordinator currently does the same thing.  Is there any reason this
> > shouldn't go in BrowserCoordinator?
> 
> I am thinking about moving it there.

Acknowledged.

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:72:
self.ntpView.tabBar.items = _tabBarItems;
On 2017/04/06 14:30:13, marq wrote:
> On 2017/04/05 19:28:24, justincohen wrote:
> > On 2017/04/05 12:22:48, marq wrote:
> > > self.tabBarItems? 
> > 
> > I could create a property and override the setter to call
> > self.NTPView.tabBar.items... but it only gets set here, once.  Why bother?
> 
> I meant that you have a tabBarItems property, so it's cleaner (IMO) to use it
> here instead of _tabBarItems.

Acknowledged.

https://codereview.chromium.org/2785893003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/ntp/new_tab_page_view.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/ntp/new_tab_page_view.mm:87: // |-layoutSubviews| is not
successfully triggered when |-setNeedsLayout| is
On 2017/04/06 13:01:04, lpromero wrote:
> Reflow the comment.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/BUILD.gn:14: "//ios/chrome/browser/ui:ui",
On 2017/04/06 13:01:04, lpromero wrote:
> No need to repeat the target name when it's the folder name. Here and below
> twice.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:8: #import
<UIKit/UIKit.h>
On 2017/04/06 13:01:04, lpromero wrote:
> Add new line.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:11: @interface
BookmarksCoordinator : BrowserCoordinator
On 2017/04/06 13:01:04, lpromero wrote:
> Comment.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.h:12: @end
On 2017/04/06 13:01:04, lpromero wrote:
> Please add a unit tests file and a simple test that the view controller is set
> up correctly when started.

Added unittest but it's just a stub for now.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:22:
BookmarkHomeTabletNTPController* wrapperViewController;
On 2017/04/06 13:01:04, lpromero wrote:
> This class is not a view controller. Consider renaming the property.

Done, here and everywhere.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:44:
self.viewController.view = [self.wrapperViewController view];
On 2017/04/06 13:08:13, rohitrao wrote:
> Is it ok to assign to |view| like this, or should we subclass and implement
> loadView?

I think this is OK for now.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/bookmarks/bookmarks_coordinator.mm:44:
self.viewController.view = [self.wrapperViewController view];
On 2017/04/06 14:30:13, marq wrote:
> On 2017/04/06 13:08:13, rohitrao wrote:
> > Is it ok to assign to |view| like this, or should we subclass and implement
> > loadView?
> 
> |view| is a public writable property, so implicitly this is OK. Note that
> setting |view| will not trigger |viewDidLoad| as far as I know.
> 
> However, it is worth noting that the UIViewController docs say "Each view
> controller object is the sole owner of its view. You must not associate the
same
> view object with multiple view controller objects. The only exception to this
> rule is that a container view controller implementation may add this view as a
> subview in its own view hierarchy."

This is a temporary solution that will go away..

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/ntp_commands.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // Command protocol for
commands relating to the Settings UI.
On 2017/04/06 13:01:04, lpromero wrote:
> s/Settings/NTP

oops, thanks.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:8: // Command protocol for
commands relating to the Settings UI.
On 2017/04/06 13:08:13, rohitrao wrote:
> Update comment to not reference Settings.

copy pasta is delicious.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:11: // Start a Chrome Home
panel.
On 2017/04/06 13:01:04, lpromero wrote:
> s/Start/Starts here and below.

Changed to display.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:13: // Start a bookmarks
panel on iPad or present a bookmarks panel on iPhone.
On 2017/04/06 13:01:04, lpromero wrote:
> What does Start mean in this context? Start from coordinators? Presentation is
> also a kind of coordinator start.

Changed wording to display with better explanation

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/ntp_commands.h:13: // Start a bookmarks
panel on iPad or present a bookmarks panel on iPhone.
On 2017/04/06 13:01:04, lpromero wrote:
> s/present/presents

Acknowledged.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/BUILD.gn:22: "//ios/chrome/browser/ui:ui",
On 2017/04/06 13:01:04, lpromero wrote:
> Idem for not repeating the target when it can be implicit. I wish gn gen
> produced a better error message when missing a deps.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/BUILD.gn:27:
"//ios/clean/chrome/browser/ui/open_tabs/",
On 2017/04/06 13:01:04, lpromero wrote:
> Remove the trailing slash.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:7: 
On 2017/04/06 13:01:04, lpromero wrote:
> #import Foundation

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:8: @class
NewTabPageBarItem;
On 2017/04/06 13:01:04, lpromero wrote:
> Add a new line.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:11: // Tell NTP what bar
items to display.
On 2017/04/06 13:01:05, lpromero wrote:
> *Tells

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_consumer.h:12: -
(void)setBarItems:(NSMutableArray<NewTabPageBarItem*>*)items;
On 2017/04/06 13:01:05, lpromero wrote:
> Does it need to be mutable?

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.h:21:
@property(nonatomic, assign) web::WebState* webState;
On 2017/04/06 14:30:14, marq wrote:
> Is this only used to pass on to the home coordinator?

Yep.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:53: for
(BrowserCoordinator* child in self.children) {
On 2017/04/06 13:08:13, rohitrao wrote:
> Copy the comment from ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:
> 
>   // PLACEHOLDER: Stop child coordinators here for now. We might deal with
this
>   // differently later on.
> 
> Might even be worth filing a bug to come to a decision here and convert these
to
> TODOs.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:59: -
(void)childCoordinatorDidStart:(BrowserCoordinator*)coordinator {
On 2017/04/06 13:08:13, rohitrao wrote:
> Can you briefly sketch out the lifecycles of the various child coordinators? 
> Does the BookmarksCoordinator always exist, or is it created only when the
user
> taps on the button or tries to swipe over to that panel?
> 
> When you swipe away from the bookmarks panel on tablet, do we stop and destroy
> the BookmarksCoordinator, or does it stay alive forever?

We don't start a child coordinator until it is visible on screen.  We do not
destroy childCoordinators when they move off screen.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_coordinator.mm:106: if
(IsIPadIdiom()) {
On 2017/04/06 13:01:05, lpromero wrote:
> Remove this section as it is done in childCoordinatorDidStart.

crap, bad patch here.  thanks for catching this.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_HOME_COORDINATOR_H_
On 2017/04/06 13:01:05, lpromero wrote:
> Please add a unittests file.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:15: @interface
NTPHomeCoordinator : BrowserCoordinator
On 2017/04/06 13:01:05, lpromero wrote:
> Comment.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.h:15: @interface
NTPHomeCoordinator : BrowserCoordinator
On 2017/04/06 13:08:13, rohitrao wrote:
> Filenames should match the class names, either use NewTabPage or NTP
everywhere.

Per offline conversation, renamed to NTP everywhere, since that's what the rest
of chrome does.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_coordinator.mm:20:
@property(nonatomic, strong) GoogleLandingController* wrapperViewController;
On 2017/04/06 13:01:05, lpromero wrote:
> This is not a view controller. Should the property be renamed?

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_HOME_MEDIATOR_H_
On 2017/04/06 13:01:05, lpromero wrote:
> Please add a unittests file.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:13: @interface
NTPHomeMediator : NSObject<UrlLoader, OmniboxFocuser>
On 2017/04/06 13:01:05, lpromero wrote:
> Comment.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.h:16: // Calling code
should assign this before starting this coordinator.
On 2017/04/06 14:30:14, marq wrote:
> This isn't a coordinator.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm:1: 
On 2017/04/06 13:01:05, lpromero wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_home_mediator.mm:31:
self.webState->GetNavigationManager()->LoadURLWithParams(params);
On 2017/04/06 14:30:14, marq wrote:
> I don't think we want to plumb the webstate down to this mediator just so it
can
> open URLs. Maybe opening URLs (in the same tab) should be a command that the
tab
> coordinator handles? 

Moved to a dispatcher and a TabCommand.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h:6: #define
IOS_CLEAN_CHROME_BROWSER_UI_NTP_NEW_TAB_PAGE_MEDIATOR_H_
On 2017/04/06 13:01:05, lpromero wrote:
> Please add a unittests file.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.h:12: @interface
NTPMediator : NSObject
On 2017/04/06 13:01:05, lpromero wrote:
> Comments.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:1: 
On 2017/04/06 13:01:05, lpromero wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:33: 
On 2017/04/06 13:01:05, lpromero wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_mediator.mm:64: [self.consumer
setBarItems:tabBarItems];
On 2017/04/06 13:01:05, lpromero wrote:
> [tabBarItems copy]

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:16: // The
dispatcher for this view controller
On 2017/04/06 13:01:06, lpromero wrote:
> Add more new lines to this interface.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:19: // Add a
Chrome Home panel.
On 2017/04/06 13:01:06, lpromero wrote:
> *Adds, here and below.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.h:24: -
(void)addOpenTabsViewController:(UIViewController*)controller;
On 2017/04/06 13:01:06, lpromero wrote:
> Please add unittests for these methods. Or a Showcase EarlGrey test.

Added stub unittests for now.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:73:
self.NTPView.tabBar.items = _tabBarItems;
On 2017/04/06 13:01:06, lpromero wrote:
> Should you DCHECK that _tabBarItems is set? Because setBarItems doesn't set
the
> NTPView.tabBar.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:105:
NewTabPageBarItem* item = self.NTPView.tabBar.items[1];
On 2017/04/06 13:08:13, rohitrao wrote:
> This is weird -- we get a set of items from the mediator, but then we make
> assumptions about exactly what that set contains, in what order.  We should
try
> to refactor this so that the mediator hands the VC an opaque blob of data, and
> the VC displays it accordingly but doesn't otherwise need to know what's in
it.

More of this logic will move into the mediator in a followup CL.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:130:
_tabBarItems = items;
On 2017/04/06 13:01:06, lpromero wrote:
> This supposes it is called before the view is loaded.

Correct.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:135: -
(void)scrollViewDidScroll:(UIScrollView*)scrollView {
On 2017/04/06 14:30:14, marq wrote:
> On 2017/04/06 13:01:06, lpromero wrote:
> > Could this logic be in a separate object whose single responsibility would
be
> > driving handling this call? Seems the only knowledge it needs is the tabBar.
> > This object would then be directly the scrollview delegate.
> > NewTabPageScrollViewDelegate class?
> > This file will become exceedingly long if we don't split the logic.
> 
> +1

Acknowledged.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:135: -
(void)scrollViewDidScroll:(UIScrollView*)scrollView {
On 2017/04/06 13:01:06, lpromero wrote:
> Could this logic be in a separate object whose single responsibility would be
> driving handling this call? Seems the only knowledge it needs is the tabBar.
> This object would then be directly the scrollview delegate.
> NewTabPageScrollViewDelegate class?
> This file will become exceedingly long if we don't split the logic.

There's not that much more logic necessary in this view controller, and the NTP
isn't going to exist in bijou.  However, created a tracking bug regardless.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/new_tab_page_view_controller.mm:172:
changePanel:(BOOL)changePanel {
On 2017/04/06 13:01:06, lpromero wrote:
> This is unused. Will it stay unused?

This is not unused.  It's set in |tabBar.delegate = self;| above.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/open_tabs/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/open_tabs/BUILD.gn:5: source_set("open_tabs") {
On 2017/04/06 13:01:06, lpromero wrote:
> Should it be "opened_tabs"?

renamed everything to recent_tabs

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/open_tabs/open_tabs_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/open_tabs/open_tabs_coordinator.h:11: @interface
OpenTabsCoordinator : BrowserCoordinator
On 2017/04/06 13:01:06, lpromero wrote:
> Please add comments, as it is unclear what this coordinates: does it open
tabs,
> is it a list of opened tabs?

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:84: // contentViewController
when it starts afterwards.
On 2017/04/06 13:01:06, lpromero wrote:
> Should we then delay starting the webCoordinator then, instead of resetting
the
> NTP view controller?

Perhaps, saving for a followup.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:105: [self.viewController
setContentViewController:nil];
On 2017/04/06 13:01:06, lpromero wrote:
> Use self.viewController.contentViewController = nil to keep the style
> consistent.

Done.

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:105: [self.viewController
setContentViewController:nil];
On 2017/04/06 13:01:06, lpromero wrote:
> Why this call? Seems like you expect a side effect? It should be documented.
> I think you should rather implement childCoordinatorWillStop and based on the
> type of the coordinator, reset the content view controller. (Or maybe not even
> care about the stopped child type and reset the contentViewController.)

Moved to childCoordinatorWillStop

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2785893003/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:17:
"//ios/chrome/browser:browser",
On 2017/04/06 13:01:06, lpromero wrote:
> No need to repeat browser.

Done.

[justincohen, 2017-04-06 18:25:17]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 18:26:37]

Dry run: 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-04-06 18:31:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 18:31:43]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[justincohen, 2017-04-06 19:46:29]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 19:47:24]

Dry run: 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-04-06 20:55:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 20:55:45]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-07 12:25:01]

LGTM with mostly nits and one major fix.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/ntp_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/ntp_view_controller.h:21: -
(void)addHomePanelViewController:(UIViewController*)controller;
No need to change for this CL, but maybe these should be properties for
consistency with other container VCs?

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/ntp_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/ntp_view_controller.mm:94: -
(void)addControllerToScrollView:(UIViewController*)controller {
Comment for this method (or add a forward declaration with a comment), and move
under a #pragma mark - private.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.h:11: // A
coordinator for the recent tabs UI, which can be presented modally on it's
nit: its

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.mm:35:
[self.context.baseViewController presentViewController:self.viewController
We've gotten rid of context; move this to the parent coordinator's
-childDidStart.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:93: if
(self.webState->GetLastCommittedURL() == GURL("chrome://newtab/")) {
Use kChromeUINewTabURL from ios/chrome/browser/chrome_url_constants.h.

[justincohen, 2017-04-07 15:27:30]

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/ntp_view_controller.h (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/ntp_view_controller.h:21: -
(void)addHomePanelViewController:(UIViewController*)controller;
On 2017/04/07 12:25:01, marq wrote:
> No need to change for this CL, but maybe these should be properties for
> consistency with other container VCs?

This is a great idea and I made the change to this CL!

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/ntp/ntp_view_controller.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/ntp/ntp_view_controller.mm:94: -
(void)addControllerToScrollView:(UIViewController*)controller {
On 2017/04/07 12:25:01, marq wrote:
> Comment for this method (or add a forward declaration with a comment), and
move
> under a #pragma mark - private.

Done.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.h (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.h:11: // A
coordinator for the recent tabs UI, which can be presented modally on it's
On 2017/04/07 12:25:01, marq wrote:
> nit: its

Done.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.mm:35:
[self.context.baseViewController presentViewController:self.viewController
On 2017/04/07 12:25:01, marq wrote:
> We've gotten rid of context; move this to the parent coordinator's
> -childDidStart.

Done.

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2785893003/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:93: if
(self.webState->GetLastCommittedURL() == GURL("chrome://newtab/")) {
On 2017/04/07 12:25:01, marq wrote:
> Use kChromeUINewTabURL from ios/chrome/browser/chrome_url_constants.h.

Done.

[justincohen, 2017-04-07 15:27:42]

The CQ bit was checked by justincohen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 15:27:55]

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

[lpromero, 2017-04-07 15:38:03]

lgtm

[justincohen, 2017-04-07 16:01:11]

The CQ bit was unchecked by justincohen@chromium.org

[justincohen, 2017-04-07 16:01:19]

The CQ bit was checked by justincohen@chromium.org

[justincohen, 2017-04-07 16:01:19]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2785893003/#ps180001
(title: "Respond to comments, rebase")

[commit-bot: I haz the power, 2017-04-07 16:01:52]

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

[justincohen, 2017-04-07 16:02:44]

The CQ bit was unchecked by justincohen@chromium.org

[justincohen, 2017-04-07 16:03:49]

The CQ bit was checked by justincohen@chromium.org

[justincohen, 2017-04-07 16:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2785893003/#ps200001
(title: "Comment nit")

[commit-bot: I haz the power, 2017-04-07 16:04:28]

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-04-07 17:42:08]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491581029262650,
"parent_rev": "c3257e6eb48627fde7ad129012ee9904110e4969", "commit_rev":
"6c0f9edbe4a39bdc26e89254a205ac477ca4b902"}

[commit-bot: I haz the power, 2017-04-07 17:43:00]

Description was changed from

==========
[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

In preparation for back-porting UIViewControllers in the NTP, add support for
loading contentViewControllers inside the clean NTP's scroll view.

This also adds stubs for the NTP mediator and consumer, as well as fix
some navigation errors when loading and unloading the NTP.

BUG=
==========

to

==========
[ios clean] Add placeholder for NTP bookmarks, chrome home and open tabs.

In preparation for back-porting UIViewControllers in the NTP, add support for
loading contentViewControllers inside the clean NTP's scroll view.

This also adds stubs for the NTP mediator and consumer, as well as fix
some navigation errors when loading and unloading the NTP.

BUG=

Review-Url: https://codereview.chromium.org/2785893003
Cr-Commit-Position: refs/heads/master@{#462914}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c0f9edbe4a39bdc26e89254a205...
==========

[commit-bot: I haz the power, 2017-04-07 17:43:02]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/6c0f9edbe4a39bdc26e89254a205...