Add InfoBarPickerController for displaying a picker view from an infobar

Add InfoBarPickerController for displaying a picker view from an infobar

Add InfoBarPickerController class for displaying a picker view upon tapping
a link in an infobar. InfoBarPickerController presents a picker view and a
navigation bar with "Cancel" and "Done" buttons. This is modeled after
the picker view displayed in BeforeTranslateInfoBarController, and is
meant to replace the implementation there and be shared with the
upcoming update passwords infobar.

BUG=622244

[Jackie Quinn, 2016-06-28 10:52:13]

jyquinn@chromium.org changed reviewers:
+ jif@chromium.org

[marq (ping after 24h), 2016-06-28 13:43:17]

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

[marq (ping after 24h), 2016-06-28 13:43:18]

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
File ios/chrome/browser/infobars/infobar_picker_view.h (right):

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
ios/chrome/browser/infobars/infobar_picker_view.h:12: @protocol
InfoBarPickerViewDelegate<NSObject>
Instead of having a protocol for passing UI events up to a controller, I'd like
to see the target/action pattern used here.

The owning VC would implement these methods (which would have to follow the
target/action pattern and take no parameter or (id)sender, which will be the
control that triggers them).

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
ios/chrome/browser/infobars/infobar_picker_view.h:20: @property(nonatomic, weak)
id<UIPickerViewDelegate> pickerDelegate;
If the picker view is public, then its delegate and data source are, too, so
there's no real need for these properties.

My current thinking for the coordinator pattern is that the view controller
owned by the coordinator will handle view-to-controller communications and
initial view setup ("binding" model values to the view), but it will signal to
the coordinator (by means of a protocol) any actions that should mutate model
state. Additionally, if the mapping from whatever model code provides the
content for the VC's views is nontrivial, it should be handled by a mediator
object. In any case, since the coordinator is creating the view controller, it
should provide it with any model or mediator objects it needs to populate its
views.

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
File ios/chrome/browser/infobars/infobar_picker_view.mm (right):

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
ios/chrome/browser/infobars/infobar_picker_view.mm:33: target:self
Better to set a target of nil, so the action will bubble up the responder chain.

[Jackie Quinn, 2016-06-28 13:51:45]

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
File ios/chrome/browser/infobars/infobar_picker_view.h (right):

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
ios/chrome/browser/infobars/infobar_picker_view.h:20: @property(nonatomic, weak)
id<UIPickerViewDelegate> pickerDelegate;
On 2016/06/28 13:43:18, marq wrote:
> If the picker view is public, then its delegate and data source are, too, so
> there's no real need for these properties.
> 
> My current thinking for the coordinator pattern is that the view controller
> owned by the coordinator will handle view-to-controller communications and
> initial view setup ("binding" model values to the view), but it will signal to
> the coordinator (by means of a protocol) any actions that should mutate model
> state. Additionally, if the mapping from whatever model code provides the
> content for the VC's views is nontrivial, it should be handled by a mediator
> object. In any case, since the coordinator is creating the view controller, it
> should provide it with any model or mediator objects it needs to populate its
> views.

That makes sense.

Would it make more sense to just write this class as  a view controller instead
of a custom view? All it's really doing is view setup and binding.

[marq (ping after 24h), 2016-06-28 14:43:21]

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
File ios/chrome/browser/infobars/infobar_picker_view.h (right):

https://codereview.chromium.org/2107743002/diff/1/ios/chrome/browser/infobars...
ios/chrome/browser/infobars/infobar_picker_view.h:20: @property(nonatomic, weak)
id<UIPickerViewDelegate> pickerDelegate;
On 2016/06/28 13:51:45, Jackie Quinn wrote:
> On 2016/06/28 13:43:18, marq wrote:
> > If the picker view is public, then its delegate and data source are, too, so
> > there's no real need for these properties.
> > 
> > My current thinking for the coordinator pattern is that the view controller
> > owned by the coordinator will handle view-to-controller communications and
> > initial view setup ("binding" model values to the view), but it will signal
to
> > the coordinator (by means of a protocol) any actions that should mutate
model
> > state. Additionally, if the mapping from whatever model code provides the
> > content for the VC's views is nontrivial, it should be handled by a mediator
> > object. In any case, since the coordinator is creating the view controller,
it
> > should provide it with any model or mediator objects it needs to populate
its
> > views.
> 
> That makes sense.
> 
> Would it make more sense to just write this class as  a view controller
instead
> of a custom view? All it's really doing is view setup and binding.

Well, maybe. The view layout isn't too involved here, but I'm also trying to get
as many things off the plate of view controllers as possible, and laying out
views is one of the areas that's still kind of muddy. This case is probably
simple enough that it can all be in a view controller without too much scope
creep. I would still want to see target/action set up generically, however, so
that the view code can easily be transplanted out.

[Jackie Quinn, 2016-06-28 16:48:31]

Description was changed from

==========
Add InfoBarPickerView for displaying a picker view from an infobar

Add InfoBarPickerView class for displaying a picker view upon tapping
a link in an infobar. InfoBarPickerView includes a picker view and a
navigation bar with "Cancel" and "Done" buttons. This is modeled after
the picker view displayed in BeforeTranslateInfoBarController, and is
meant to replace the implementation there and be shared with the
upcoming update passwords infobar.

BUG=622244
==========

to

==========
Add InfoBarPickerController for displaying a picker view from an infobar

Add InfoBarPickerController class for displaying a picker view upon tapping
a link in an infobar. InfoBarPickerController presents a picker view and a
navigation bar with "Cancel" and "Done" buttons. This is modeled after
the picker view displayed in BeforeTranslateInfoBarController, and is
meant to replace the implementation there and be shared with the
upcoming update passwords infobar.

BUG=622244
==========

[Jackie Quinn, 2016-06-28 16:49:28]

Updated to be a UIViewController, and moved to target/action rather than
delegate.

[Jackie Quinn, 2016-06-30 11:55:21]

Added delegate protocol and incorporated UIPickerViewDataSource and
UIPickerViewDelegate logic. PTAL.

[marq (ping after 24h), 2016-06-30 16:10:11]

First pass, I will pobably have some comments about the pickerView delegate and
data source setup,

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar_picker_controller.h (right):

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.h:31: @interface
InfoBarPickerController : UIViewController
Please name view controller subclasses "<something>ViewController".

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar_picker_controller.mm (right):

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:38: _pickerView =
[[UIPickerView alloc] initWithFrame:CGRectZero];
 View controllers usually create subviews in -viewDidLoad.

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:79:
self.doneButton.target = target;
This (and its companion method) should never be necessary. 

The view controller owns a view, and it should either handle the actions sent by
its views, or they should be forwarded up the responder chain.

The controls should set a target of nil and an action selector that's either
defined and implemented in this class, or is implemented in a view controller
further up in the responder chain.

[Jackie Quinn, 2016-06-30 18:00:39]

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar_picker_controller.h (right):

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.h:31: @interface
InfoBarPickerController : UIViewController
On 2016/06/30 16:10:11, marq wrote:
> Please name view controller subclasses "<something>ViewController".

Done.

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar_picker_controller.mm (right):

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:38: _pickerView =
[[UIPickerView alloc] initWithFrame:CGRectZero];
On 2016/06/30 16:10:11, marq wrote:
>  View controllers usually create subviews in -viewDidLoad.

I created them originally to make sure delegate/target/action setting would
happen properly. With what I suggested below, I'll move them to viewDidLoad. Or
maybe into loadView?

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:79:
self.doneButton.target = target;
On 2016/06/30 16:10:11, marq wrote:
> This (and its companion method) should never be necessary. 
> 
> The view controller owns a view, and it should either handle the actions sent
by
> its views, or they should be forwarded up the responder chain.
> 
> The controls should set a target of nil and an action selector that's either
> defined and implemented in this class, or is implemented in a view controller
> further up in the responder chain. 

I agree... these seemed a bit silly to me. I didn't really understand your
original comments. 

What happens when the ultimate responder isn't a view controller? In this case
it's the infobar controller, which, sadly, is not a view controller. I could
have this class respond and invoke delegate methods that looks more like
infoBarPickerViewController:dismissWithSelection: or something like that, would
that work?

[marq (ping after 24h), 2016-07-01 11:48:44]

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
File ios/chrome/browser/infobars/infobar_picker_controller.mm (right):

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:38: _pickerView =
[[UIPickerView alloc] initWithFrame:CGRectZero];
On 2016/06/30 18:00:39, Jackie Quinn wrote:
> On 2016/06/30 16:10:11, marq wrote:
> >  View controllers usually create subviews in -viewDidLoad.
> 
> I created them originally to make sure delegate/target/action setting would
> happen properly. With what I suggested below, I'll move them to viewDidLoad.
Or
> maybe into loadView?

- loadView is called when a UIViewController's |view| property is accessed and
has a nil value. It should create the root view of the view controller's view
hierarchy and assign it to self.view. If this method isn't implemented and
there's a .xib file for the view controller, the view hierarchy is instantiated
from the .xib (we don't do this). If this method isn't implemented, a plain
UIView instance is created.

- viewDidLoad is called after the view hierarchy is loaded via -loadView, and
it's the expected place to perform "additional customization" of views. The
UIViewController docs recommend, if -loadView is being used to manually create a
root view (as it is here), that additional view initialization happens in
-viewDidLoad.

The additional view creation you're doing in loadView now is fine, since they
need to go inside the StackView, and it would be ugly to cast self.view into a
UIStackView later. But the view initialization in the -init can go into
viewDidLoad.

https://codereview.chromium.org/2107743002/diff/40001/ios/chrome/browser/info...
ios/chrome/browser/infobars/infobar_picker_controller.mm:79:
self.doneButton.target = target;
On 2016/06/30 18:00:39, Jackie Quinn wrote:
> On 2016/06/30 16:10:11, marq wrote:
> > This (and its companion method) should never be necessary. 
> > 
> > The view controller owns a view, and it should either handle the actions
sent
> by
> > its views, or they should be forwarded up the responder chain.
> > 
> > The controls should set a target of nil and an action selector that's either
> > defined and implemented in this class, or is implemented in a view
controller
> > further up in the responder chain. 
> 
> I agree... these seemed a bit silly to me. I didn't really understand your
> original comments. 
> 
> What happens when the ultimate responder isn't a view controller? In this case
> it's the infobar controller, which, sadly, is not a view controller. I could
> have this class respond and invoke delegate methods that looks more like
> infoBarPickerViewController:dismissWithSelection: or something like that,
would
> that work?

This is one problem we have with having these not-really-a-view-controller
controllers around. The next item in the responder chain will actually be the
application's root view controller, since that's what's presenting this VC.

Instead of explaining what I had in mind here, I'll add this as a (very good)
example in the Coordinators document.