[ios] Adds LocationBarCoordinator.

[ios] Adds LocationBarCoordinator.

This CL adds the basics of an omnibox to the clean skeleton app.  The toolbar
now contains a real OmniboxTextFieldIOS with its associated machinery
(OmniboxViewIOS and LocationBarController).  The URL in the omnibox is kept up
to date during navigation and across tab switches.  Typing in the omnibox and
pressing Go will also navigate to the default match.

The omnibox popup is not yet implemented, and neither are the animations when
the omnibox gains or loses focus.

BUG=708341
Review-Url: https://codereview.chromium.org/2761343002
Cr-Commit-Position: refs/heads/master@{#462119}
Committed: https://chromium.googlesource.com/chromium/src/+/74849bc2fdb3ebde47584620b9a1c8da0c72bcd0

[rohitrao (ping after 24h), 2017-04-05 00:29:05]

Description was changed from

==========
[ios] Adds LocationBarCoordinator.

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

to

==========
[ios] Adds LocationBarCoordinator.

This CL adds the basics of an omnibox to the clean skeleton app.  The toolbar
now contains a real OmniboxTextFieldIOS with its associated machinery
(OmniboxViewIOS and LocationBarController).  The URL in the omnibox is kept up
to date during navigation and across tab switches.  Typing in the omnibox and
pressing Go will also navigate to the default match.

The omnibox popup is not yet implemented, and neither are the animations when
the omnibox gains or loses focus.

BUG=708341
==========

[rohitrao (ping after 24h), 2017-04-05 00:29:24]

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

[commit-bot: I haz the power, 2017-04-05 00:30:26]

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 00:33:56]

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

[commit-bot: I haz the power, 2017-04-05 00:33:57]

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

[rohitrao (ping after 24h), 2017-04-05 00:33:57]

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

[rohitrao (ping after 24h), 2017-04-05 00:33:58]

First attempt at adding the omnibox, ready for a real review.

I don't yet have unittests, although I added empty files for them.  I also
haven't yet figured out how to get the popup working, and I've seriously punted
on moving files to ios/shared.

This gets the basics of the omnibox working, and it lets you type in the text
field and navigate to new sites.  I'd like to land this and keep iterating --
I'm happy with the basic structure, but I won't be surprised if lots of little
things change.

I've tried to keep to the spirit of mediators and consumers, even if I couldn't
actually use them in practice.  I've also added header comments to document why
the omnibox looks different from everything else.

Let me know what you think!

[rohitrao (ping after 24h), 2017-04-05 01:32:18]

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

[commit-bot: I haz the power, 2017-04-05 01:33:52]

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 03:04:43]

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

[commit-bot: I haz the power, 2017-04-05 03:04:44]

Dry run: This issue passed the CQ dry run.

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

I think there's plenty to discuss here. 

If this CL constitutes "hacking in" the location bar, and future CLs will
untangle things, then I'm generally in favor (and I'd suggest that a lot of the
comments should be marked HACK accordingly).

On the other hand, if the intent is that (for example) LoactionBarControllerImpl
will continue to function this way long-term, then I'm generally not in favor.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm:21:
@property(nonatomic, readwrite, strong) LocationBarMediator* mediator;
No need for readwrite, since this isn't a redeclaration.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm:43:
browser->browser_state(), nil,
Add comments to nil params so it's obvious what they are without looking at the
LocationBarControllerImpl header -- "nil /* preloaded */", etc.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_mediator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_mediator.mm:110: }
Are these to-be-implemented?

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_mediator_unittest.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_mediator_unittest.mm:9: #endif
Nice tests ;-)

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:17:
@implementation LocationBarViewController {
Spurious {}

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:26:
self.omnibox =
Don't assign to properties in the init; use _omnibox instead.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:37:
self.omnibox.translatesAutoresizingMaskIntoConstraints = YES;
YES is the default, so no need for this (unless it's not the default for the
superclass, in which case a comment would be good).

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:41:
[self.view addSubview:self.omnibox];
Why do this in -loadView and not -viewDidLoad ?

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:69:
locationBarCoordinator.viewController;
Should this part be in -childCoordinatorDidStart?

https://codereview.chromium.org/2761343002/diff/180001/ios/shared/chrome/brow...
File
ios/shared/chrome/browser/ui/browser_list/browser_web_state_list_delegate.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/shared/chrome/brow...
ios/shared/chrome/browser/ui/browser_list/browser_web_state_list_delegate.mm:23:
IOSChromeSessionTabHelper::CreateForWebState(web_state);
Is this needed for this CL?

Is your thinking that this is the right place to add tab helpers?

[rohitrao (ping after 24h), 2017-04-05 11:40:22]

This is not intended to be a hack.

I can potentially break LocationBarControllerImpl up into two classes -- one
that computes the new values for various pieces of UI, and a second that does
the work of actually updating the UI.  This would split a little more cleanly
around the mediator/consumer boundary, and the latter code could be pushed into
the VC.

Overall, though, LocationBarController doesn't do all that much, and there's not
much room to make larger changes.  The VC vends a text field that gets wrapped
in an OmniboxViewIOS.  That view (via its cross-platform base class) creates and
owns an OmniboxEditModel.  90% of omnibox updates are done by the model talking
directly to the view, with no iOS-specific code involved. 
(LocationBarController is iOS-specific, but it's largely concerned with
decorations, not the omnibox itself.)

[rohitrao (ping after 24h), 2017-04-05 13:36:14]

https://codereview.chromium.org/2761343002/diff/180001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:169:
OmniboxViewIOS::OmniboxViewIOS(OmniboxTextFieldIOS* field,
Mark and I chatted in person about this design.  I'm going to try and wrap the
OmniboxTextFieldIOS in two protocols: one that's a write-only consumer
interface, and a second that's a read-only datasource interface.

The LocationBarController and OmniboxViewIOS will get pointers to these two
protocols instead of getting a pointer to the text field directly.  That should
let us keep the text field encapsulated within the VC (instead the VC will
implement these protocols and forward calls to the field).

The LocationBarController and OmniboxViewIOS will continue to live in the
mediator for now.

[rohitrao (ping after 24h), 2017-04-05 14:36:07]

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

[commit-bot: I haz the power, 2017-04-05 14:36:32]

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

[rohitrao (ping after 24h), 2017-04-05 14:37:05]

Addressed review comments.  Larger changes (including the consumer/datasource
protocols) will come in a future CL.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm:21:
@property(nonatomic, readwrite, strong) LocationBarMediator* mediator;
On 2017/04/05 09:07:18, marq wrote:
> No need for readwrite, since this isn't a redeclaration.

Removed, but I've always preferred to leave it in, because that way I never have
to think about what the default is.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_coordinator.mm:43:
browser->browser_state(), nil,
On 2017/04/05 09:07:18, marq wrote:
> Add comments to nil params so it's obvious what they are without looking at
the
> LocationBarControllerImpl header -- "nil /* preloaded */", etc.

Done.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_mediator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_mediator.mm:110: }
On 2017/04/05 09:07:18, marq wrote:
> Are these to-be-implemented?

These are all TODOs.  I don't know what needs to go in them yet.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_mediator_unittest.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_mediator_unittest.mm:9: #endif
On 2017/04/05 09:07:19, marq wrote:
> Nice tests ;-)

Only the best will pass muster.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:17:
@implementation LocationBarViewController {
On 2017/04/05 09:07:19, marq wrote:
> Spurious {}

I was expecting to need them for something =)

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:26:
self.omnibox =
On 2017/04/05 09:07:19, marq wrote:
> Don't assign to properties in the init; use _omnibox instead.

Done.  We are not consistent about this in ios/clean, but I agree that we
shouldn't use properties in init.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:37:
self.omnibox.translatesAutoresizingMaskIntoConstraints = YES;
On 2017/04/05 09:07:19, marq wrote:
> YES is the default, so no need for this (unless it's not the default for the
> superclass, in which case a comment would be good).

Done.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:41:
[self.view addSubview:self.omnibox];
On 2017/04/05 09:07:19, marq wrote:
> Why do this in -loadView and not -viewDidLoad ?

Moved.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:69:
locationBarCoordinator.viewController;
On 2017/04/05 09:07:19, marq wrote:
> Should this part be in -childCoordinatorDidStart?

Moved.  I don't think we ever decided on the pattern to use for children that
are always "started".  I'd prefer to always hook up VCs in
childCoordinatorDidStart (as opposed to what I originally wrote here).

https://codereview.chromium.org/2761343002/diff/180001/ios/shared/chrome/brow...
File
ios/shared/chrome/browser/ui/browser_list/browser_web_state_list_delegate.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/shared/chrome/brow...
ios/shared/chrome/browser/ui/browser_list/browser_web_state_list_delegate.mm:23:
IOSChromeSessionTabHelper::CreateForWebState(web_state);
On 2017/04/05 09:07:19, marq wrote:
> Is this needed for this CL?
> 
> Is your thinking that this is the right place to add tab helpers?

The omnibox requires these two helpers to exist.  Long-term, I'd like to attach
tab helpers in this method, but that will happen through a call to the
AttachTabHelpers() function.

For now, I don't want to bring up every helper (which would also create Tab
objects), so I'm manually creating the ones we need.

[marq (ping after 24h), 2017-04-05 14:59:11]

LGTM!

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm
(right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:26:
self.omnibox =
On 2017/04/05 14:37:05, rohitrao wrote:
> On 2017/04/05 09:07:19, marq wrote:
> > Don't assign to properties in the init; use _omnibox instead.
> 
> Done.  We are not consistent about this in ios/clean, but I agree that we
> shouldn't use properties in init.

We should be consistent with this. I'll do a cleanup pass.

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2761343002/diff/180001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:69:
locationBarCoordinator.viewController;
On 2017/04/05 14:37:05, rohitrao wrote:
> On 2017/04/05 09:07:19, marq wrote:
> > Should this part be in -childCoordinatorDidStart?
> 
> Moved.  I don't think we ever decided on the pattern to use for children that
> are always "started".  I'd prefer to always hook up VCs in
> childCoordinatorDidStart (as opposed to what I originally wrote here).

Yes, we should do a cleanup pass.

https://codereview.chromium.org/2761343002/diff/200001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm
(right):

https://codereview.chromium.org/2761343002/diff/200001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:35:
self.view = [[UIView alloc] initWithFrame:CGRectZero];
Add a comment indicating why this is needed instead of the default -loadView.

[rohitrao (ping after 24h), 2017-04-05 15:17:09]

https://codereview.chromium.org/2761343002/diff/200001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm
(right):

https://codereview.chromium.org/2761343002/diff/200001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/omnibox/location_bar_view_controller.mm:35:
self.view = [[UIView alloc] initWithFrame:CGRectZero];
On 2017/04/05 14:59:11, marq wrote:
> Add a comment indicating why this is needed instead of the default -loadView.

I didn't know we got a plain old UIView by default.  This is not needed,
removed.

[rohitrao (ping after 24h), 2017-04-05 15:17:50]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2017-04-05 15:17:51]

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/2761343002/#ps220001
(title: "Remove loadView.")

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

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 15:21:49]

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

[commit-bot: I haz the power, 2017-04-05 15:21:50]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_...)

[rohitrao (ping after 24h), 2017-04-05 15:32:50]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2017-04-05 15:32:50]

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/2761343002/#ps240001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-04-05 15:33:22]

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 16:57:14]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491406370009430,
"parent_rev": "6770bd0d834bd1c837e7f24875173df0eaed8d24", "commit_rev":
"74849bc2fdb3ebde47584620b9a1c8da0c72bcd0"}

[commit-bot: I haz the power, 2017-04-05 16:58:00]

Description was changed from

==========
[ios] Adds LocationBarCoordinator.

This CL adds the basics of an omnibox to the clean skeleton app.  The toolbar
now contains a real OmniboxTextFieldIOS with its associated machinery
(OmniboxViewIOS and LocationBarController).  The URL in the omnibox is kept up
to date during navigation and across tab switches.  Typing in the omnibox and
pressing Go will also navigate to the default match.

The omnibox popup is not yet implemented, and neither are the animations when
the omnibox gains or loses focus.

BUG=708341
==========

to

==========
[ios] Adds LocationBarCoordinator.

This CL adds the basics of an omnibox to the clean skeleton app.  The toolbar
now contains a real OmniboxTextFieldIOS with its associated machinery
(OmniboxViewIOS and LocationBarController).  The URL in the omnibox is kept up
to date during navigation and across tab switches.  Typing in the omnibox and
pressing Go will also navigate to the default match.

The omnibox popup is not yet implemented, and neither are the animations when
the omnibox gains or loses focus.

BUG=708341

Review-Url: https://codereview.chromium.org/2761343002
Cr-Commit-Position: refs/heads/master@{#462119}
Committed:
https://chromium.googlesource.com/chromium/src/+/74849bc2fdb3ebde47584620b9a1...
==========

[commit-bot: I haz the power, 2017-04-05 16:58:01]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/74849bc2fdb3ebde47584620b9a1...