UpdatePasswordInfoBarController for update password UI

UpdatePasswordInfoBarController for update password UI

Adds UpdatePasswordInfoBarController as infobar controller for the
update passwords infobar. UpdatePasswordInfoBarController subclasses
ConfirmInfoBarController, adding an additional link to the label for
choosing an account in the case where multiple accounts are possible for
the password update. Tapping on this link presents a picker view for
selecting the appropriate account.

BUG=622244
Committed: https://crrev.com/16245bede16eea10edd85949b1da70c4e033f798
Cr-Commit-Position: refs/heads/master@{#403872}

[Jackie Quinn, 2016-06-30 13:21:13]

jyquinn@chromium.org changed reviewers:
+ marq@chromium.org, vabr@chromium.org

[Jackie Quinn, 2016-06-30 13:21:14]

+vabr@ for password manager, +marq@ for coordinator usage.

[vabr (Chromium), 2016-06-30 13:31:48]

Thanks! LGTM with nits.
Cheers,
Vaclav

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:17: // with
tags from superclasses.
Just wondering: is this based on the fact that every superclass has a different
tag? Is there any guarantee that the choice of 10 will not clash with choices
made in other classes? Why 10 in particular?

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
https://docs.google.com/document/d/1AXwleU_zbOLB_o3KSgNovv0sN04fseieC3M8blqvt...
seems to suggest __unsafe_unretained instead of // weak.

[Jackie Quinn, 2016-06-30 13:52:51]

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:17: // with
tags from superclasses.
On 2016/06/30 13:31:48, vabr (Chromium) wrote:
> Just wondering: is this based on the fact that every superclass has a
different
> tag? Is there any guarantee that the choice of 10 will not clash with choices
> made in other classes? Why 10 in particular?

ConfirmInfoBarController uses 1-4 in its enum for tags. I chose 10 arbitrarily.
The enum from ConfirmInfoBarController is not public, so I don't really have any
way of knowing the occupied tag space here, unless I move the enum to a
protected header, which I could potentially do.

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
On 2016/06/30 13:31:48, vabr (Chromium) wrote:
>
https://docs.google.com/document/d/1AXwleU_zbOLB_o3KSgNovv0sN04fseieC3M8blqvt...
> seems to suggest __unsafe_unretained instead of // weak.

Ah good call! Done.

[Jackie Quinn, 2016-06-30 14:30:00]

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
On 2016/06/30 13:52:50, Jackie Quinn wrote:
> On 2016/06/30 13:31:48, vabr (Chromium) wrote:
> >
>
https://docs.google.com/document/d/1AXwleU_zbOLB_o3KSgNovv0sN04fseieC3M8blqvt...
> > seems to suggest __unsafe_unretained instead of // weak.
> 
> Ah good call! Done.

... nevermind. IOSChromeUpdatePasswordInfoBarDelegate is a C++ class, so
__unsafe_unretained doesn't apply.

[vabr (Chromium), 2016-06-30 15:11:28]

lgtm

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:17: // with
tags from superclasses.
On 2016/06/30 13:52:50, Jackie Quinn wrote:
> On 2016/06/30 13:31:48, vabr (Chromium) wrote:
> > Just wondering: is this based on the fact that every superclass has a
> different
> > tag? Is there any guarantee that the choice of 10 will not clash with
choices
> > made in other classes? Why 10 in particular?
> 
> ConfirmInfoBarController uses 1-4 in its enum for tags. I chose 10
arbitrarily.
> The enum from ConfirmInfoBarController is not public, so I don't really have
any
> way of knowing the occupied tag space here, unless I move the enum to a
> protected header, which I could potentially do.

Thanks for explaining. Optionally, you could squeeze the information (1-4 are
taken, 10 seems a safe guess) into the existing comment, but I'll leave that to
your discretion.

https://codereview.chromium.org/2106413003/diff/20001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
On 2016/06/30 14:29:59, Jackie Quinn wrote:
> On 2016/06/30 13:52:50, Jackie Quinn wrote:
> > On 2016/06/30 13:31:48, vabr (Chromium) wrote:
> > >
> >
>
https://docs.google.com/document/d/1AXwleU_zbOLB_o3KSgNovv0sN04fseieC3M8blqvt...
> > > seems to suggest __unsafe_unretained instead of // weak.
> > 
> > Ah good call! Done.
> 
> ... nevermind. IOSChromeUpdatePasswordInfoBarDelegate is a C++ class, so
> __unsafe_unretained doesn't apply.

Sorry! :)

[sdefresne, 2016-06-30 15:17:40]

drive-by

https://codereview.chromium.org/2106413003/diff/60001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
nit: what about removing the "// weak" comment? It does not bring anything. A
strong pointer to a C++ object would be a std::unique_ptr<> while a strong
pointer to an Objective-C object would be base::scoped_nsobject<>, so when I see
a raw pointer I assume it is weak.

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

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.mm
(right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.mm:35:
base::scoped_nsobject<UpdatePasswordInfoBarController> controller(
Per the ARC FAQ, we prefer autorelease to scoped_nsobject for local variables.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.h (right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.h:16: -
(base::scoped_nsobject<UIView<InfoBarViewProtocol>>)
This should just return an autoreleased pointer to the view, but it looks like
that's a decision that was made further up in the class hierarchy.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:11: #import
"ios/chrome/browser/infobars/infobar_picker_coordinator.h"
Should the coordinator be in this CL? I haven't seen it anywhere else.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:18:
NSUInteger kAccountTag = 10;
This is fragile; ideally the superclass would expose the maximum tag it uses (in
an enum, for example), and this class would add to that.

But I'm wary of designs that require view tags.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:40: _delegate
= delegate;
The superclass stores the delegate already, just under a different type. Seems
awkward.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:85: #pragma
mark InfoBarPickerControllerDelegate
Why not just hand the infobar picker view controller the list of items and an
initial row index? That way the picker is freestanding.

Think of it this way -- the coordinator pattern allows you to abstract away the
details of the UI you are presenting. From the point of view of this controller,
you want to be able to ask the user to pick one of a list of values. This
controller doesn't care what UI is presented to do that. All it wants is
whichever item on the list that the user picked.

So this class should hand the coordinator the list of strings and the view
controller to present the UI in, and it should get back the chosen item. No
other details of how the UI handles this should be implemented in or have
dependencies on this class.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:105: -
(void)pickerDonePressed {
This class shouldn't be handling UI events in the picker. That's what the view
controller is for.

[Jackie Quinn, 2016-07-01 12:01:59]

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.mm
(right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.mm:35:
base::scoped_nsobject<UpdatePasswordInfoBarController> controller(
On 2016/06/30 16:17:38, marq wrote:
> Per the ARC FAQ, we prefer autorelease to scoped_nsobject for local variables.

Done.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.h (right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.h:16: -
(base::scoped_nsobject<UIView<InfoBarViewProtocol>>)
On 2016/06/30 16:17:38, marq wrote:
> This should just return an autoreleased pointer to the view, but it looks like
> that's a decision that was made further up in the class hierarchy.

Yup this is done by the base InfoBarController class.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:11: #import
"ios/chrome/browser/infobars/infobar_picker_coordinator.h"
On 2016/06/30 16:17:38, marq wrote:
> Should the coordinator be in this CL? I haven't seen it anywhere else.

Yes, I haven't sent you the CL because I was waiting for upstreamed
ChromeCoordinator :-)

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:18:
NSUInteger kAccountTag = 10;
On 2016/06/30 16:17:38, marq wrote:
> This is fragile; ideally the superclass would expose the maximum tag it uses
(in
> an enum, for example), and this class would add to that.
> 
> But I'm wary of designs that require view tags.

I agree... I was considering moving the enum to a protected header for the
superclass (I have to expose some of the methods there anyways,
https://codereview.chromium.org/2103083002/), but I'm not sure that helps all
that much.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:40: _delegate
= delegate;
On 2016/06/30 16:17:38, marq wrote:
> The superclass stores the delegate already, just under a different type. Seems
> awkward.

I agree, but I need access to the delegate as
IOSChromeUpdatePasswordInfoBarDelegate. I could go around casting it everywhere,
but that also seems awkward. ConfirmInfoBarController follows the same pattern -
keeps around a ConfirmInfoBarDelegate pointer. I could rename this ivar to
_updatePasswordInfoBarDelegate if you think that would help.

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:85: #pragma
mark InfoBarPickerControllerDelegate
On 2016/06/30 16:17:38, marq wrote:
> Why not just hand the infobar picker view controller the list of items and an
> initial row index? That way the picker is freestanding.
> 
> Think of it this way -- the coordinator pattern allows you to abstract away
the
> details of the UI you are presenting. From the point of view of this
controller,
> you want to be able to ask the user to pick one of a list of values. This
> controller doesn't care what UI is presented to do that. All it wants is
> whichever item on the list that the user picked.
> 
> So this class should hand the coordinator the list of strings and the view
> controller to present the UI in, and it should get back the chosen item. No
> other details of how the UI handles this should be implemented in or have
> dependencies on this class.

I was basing this off usage in BeforeTranslateInfoBarController and was hoping
it would be reused there. That class doesn't have an array to pass off to the
picker. Not that it couldn't be rewritten to work that way, but that's the
genesis of this implementation.

WRT above, would that mean that this controller would be a delegate of the
coordinator, and receive a callback with the selected item?

https://codereview.chromium.org/2106413003/diff/40001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:105: -
(void)pickerDonePressed {
On 2016/06/30 16:17:38, marq wrote:
> This class shouldn't be handling UI events in the picker. That's what the view
> controller is for.

Acknowledged.

[Jackie Quinn, 2016-07-05 12:55:48]

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

[Jackie Quinn, 2016-07-05 12:55:49]

+jif for infobars/

Made changes to update for SelectorCoordinator.

[jif-google, 2016-07-05 14:18:14]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-07-05 14:18:15]

lgtm

https://codereview.chromium.org/2106413003/diff/100001/ios/chrome/browser/inf...
File ios/chrome/browser/infobars/confirm_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/100001/ios/chrome/browser/inf...
ios/chrome/browser/infobars/confirm_infobar_controller.mm:47:
ConfirmInfoBarDelegate* _confirmInfobarDelegate;  // weak
like sylvain said, you can remove the "// weak" here (ConfirmInfoBarDelegate is
a C++ class).

[Jackie Quinn, 2016-07-05 15:33:29]

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

[commit-bot: I haz the power, 2016-07-05 15:33:45]

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, 2016-07-05 16:19:17]

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

[commit-bot: I haz the power, 2016-07-05 16:19:18]

Dry run: This issue passed the CQ dry run.

[Jackie Quinn, 2016-07-06 07:18:14]

https://codereview.chromium.org/2106413003/diff/60001/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/update_password_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/60001/ios/chrome/browser/pass...
ios/chrome/browser/passwords/update_password_infobar_controller.mm:22:
IOSChromeUpdatePasswordInfoBarDelegate* _delegate;  // weak
On 2016/06/30 15:17:40, sdefresne wrote:
> nit: what about removing the "// weak" comment? It does not bring anything. A
> strong pointer to a C++ object would be a std::unique_ptr<> while a strong
> pointer to an Objective-C object would be base::scoped_nsobject<>, so when I
see
> a raw pointer I assume it is weak.

Done.

https://codereview.chromium.org/2106413003/diff/100001/ios/chrome/browser/inf...
File ios/chrome/browser/infobars/confirm_infobar_controller.mm (right):

https://codereview.chromium.org/2106413003/diff/100001/ios/chrome/browser/inf...
ios/chrome/browser/infobars/confirm_infobar_controller.mm:47:
ConfirmInfoBarDelegate* _confirmInfobarDelegate;  // weak
On 2016/07/05 14:18:15, jif-google wrote:
> like sylvain said, you can remove the "// weak" here (ConfirmInfoBarDelegate
is
> a C++ class).

Done.

[jif, 2016-07-06 07:27:18]

lgtm

[Jackie Quinn, 2016-07-06 08:05:10]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-07-06 08:05:11]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
jif@google.com
Link to the patchset: https://codereview.chromium.org/2106413003/#ps120001
(title: "remove //weak")

[commit-bot: I haz the power, 2016-07-06 08:05:29]

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, 2016-07-06 09:12:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-06 09:12:25]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-06 09:13:42]

Description was changed from

==========
UpdatePasswordInfoBarController for update password UI

Adds UpdatePasswordInfoBarController as infobar controller for the
update passwords infobar. UpdatePasswordInfoBarController subclasses
ConfirmInfoBarController, adding an additional link to the label for
choosing an account in the case where multiple accounts are possible for
the password update. Tapping on this link presents a picker view for
selecting the appropriate account.

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

to

==========
UpdatePasswordInfoBarController for update password UI

Adds UpdatePasswordInfoBarController as infobar controller for the
update passwords infobar. UpdatePasswordInfoBarController subclasses
ConfirmInfoBarController, adding an additional link to the label for
choosing an account in the case where multiple accounts are possible for
the password update. Tapping on this link presents a picker view for
selecting the appropriate account.

BUG=622244

Committed: https://crrev.com/16245bede16eea10edd85949b1da70c4e033f798
Cr-Commit-Position: refs/heads/master@{#403872}
==========

[commit-bot: I haz the power, 2016-07-06 09:13:44]

Patchset 7 (id:??) landed as
https://crrev.com/16245bede16eea10edd85949b1da70c4e033f798
Cr-Commit-Position: refs/heads/master@{#403872}