UpdatePasswordInfoBarController for update password UI

ios/chrome/browser/passwords/update_password_infobar_controller.mm

Index: ios/chrome/browser/passwords/update_password_infobar_controller.mm
diff --git a/ios/chrome/browser/passwords/update_password_infobar_controller.mm b/ios/chrome/browser/passwords/update_password_infobar_controller.mm
new file mode 100644
index 0000000000000000000000000000000000000000..f1df6ac6abb39cf2d0b9829c0b14bfd2e8b9da21
--- /dev/null
+++ b/ios/chrome/browser/passwords/update_password_infobar_controller.mm
@@ -0,0 +1,119 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/passwords/update_password_infobar_controller.h"
+
+#include "base/strings/string_util.h"
+#include "base/strings/sys_string_conversions.h"
+#include "ios/chrome/browser/infobars/confirm_infobar_controller+protected.h"
+#import "ios/chrome/browser/infobars/infobar_picker_controller.h"
+#import "ios/chrome/browser/infobars/infobar_picker_coordinator.h"

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

Should the coordinator be in this CL? I haven't seen it anywhere else.

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

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

+#include "ios/chrome/browser/passwords/ios_chrome_update_password_infobar_delegate.h"
+#import "ios/public/provider/chrome/browser/ui/infobar_view_protocol.h"
+
+namespace {
+// Tag for the account link in the info bar message. Set to 10 to avoid conflict
+// with tags from superclass ConfirmInfoBarController, which uses tags 1-4.
+NSUInteger kAccountTag = 10;

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

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.

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

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.

+}
+
+@interface UpdatePasswordInfoBarController ()<InfoBarPickerControllerDelegate> {
+  __unsafe_unretained IOSChromeUpdatePasswordInfoBarDelegate* _delegate;
+  // Coordinator for presenting and dismissing the picker view for choosing
+  // the account for which to update password.
+  base::scoped_nsobject<InfoBarPickerCoordinator> _pickerCoordinator;
+  // Array of accounts that the password might be updated for.
+  base::scoped_nsobject<NSArray<NSString*>> _accounts;
+}
+// Action for when the "Done" button is pressed on the displayed picker view.
+- (void)pickerDonePressed;
+// Action for when the "Cancel" button is pressed on the displayed picker view.
+- (void)pickerCancelPressed;
+@end
+
+@implementation UpdatePasswordInfoBarController
+
+- (base::scoped_nsobject<UIView<InfoBarViewProtocol>>)
+viewForDelegate:(IOSChromeUpdatePasswordInfoBarDelegate*)delegate
+          frame:(CGRect)frame {
+  _delegate = delegate;

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

The superclass stores the delegate already, just under a different type. Seems
awkward.

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

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.

+  return [super viewForDelegate:delegate frame:frame];
+}
+
+- (void)updateInfobarLabel:(UIView<InfoBarViewProtocol>*)view {
+  [super updateInfobarLabel:view];
+
+  // Get the message text with current links marked.
+  base::string16 messageText = base::SysNSStringToUTF16(view.markedLabel);
+  // If there are multiple possible credentials, turn the account string into a
+  // link.
+  if (_delegate->ShowMultipleAccounts()) {
+    base::string16 usernameLink = base::SysNSStringToUTF16([[view class]
+        stringAsLink:base::SysUTF16ToNSString(_delegate->selected_account())
+                 tag:kAccountTag]);
+    base::ReplaceFirstSubstringAfterOffset(
+        &messageText, 0, _delegate->selected_account(), usernameLink);
+  }
+
+  [view addLabel:base::SysUTF16ToNSString(messageText)
+          target:self
+          action:@selector(infobarLinkDidPress:)];
+}
+
+- (void)infobarLinkDidPress:(NSNumber*)tag {
+  [super infobarLinkDidPress:tag];
+  if ([tag unsignedIntegerValue] != kAccountTag)
+    return;
+
+  _accounts.reset([_delegate->GetAccounts() copy]);
+  if (!_pickerCoordinator) {
+    UIViewController* baseViewController =
+        [[UIApplication sharedApplication] keyWindow].rootViewController;
+    _pickerCoordinator.reset([[InfoBarPickerCoordinator alloc]
+        initWithBaseViewController:baseViewController]);
+    InfoBarPickerController* pickerController =
+        [_pickerCoordinator infoBarPickerController];
+    pickerController.delegate = self;
+    [pickerController setDoneTarget:self action:@selector(pickerDonePressed)];
+    [pickerController setCancelTarget:self
+                               action:@selector(pickerCancelPressed)];
+  }
+  [_pickerCoordinator start];
+}
+
+#pragma mark InfoBarPickerControllerDelegate

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

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.

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

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?

+
+- (NSString*)infoBarPickerController:(InfoBarPickerController*)controller
+                          textForRow:(NSInteger)row {
+  return _accounts[row];
+}
+
+- (NSInteger)infoBarPickerControllerNumberOfRows:
+    (InfoBarPickerController*)controller {
+  return [_accounts count];
+}
+
+- (NSInteger)infoBarPickerControllerInitialRow:
+    (InfoBarPickerController*)controller {
+  return [_accounts
+      indexOfObject:base::SysUTF16ToNSString(_delegate->selected_account())];
+}
+
+#pragma mark Private methods
+
+- (void)pickerDonePressed {

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

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]

Acknowledged.

+  NSInteger selectedRow =
+      [[_pickerCoordinator infoBarPickerController].pickerView
+          selectedRowInComponent:0];
+  _delegate->set_selected_account(
+      base::SysNSStringToUTF16(_accounts[selectedRow]));
+  [self updateInfobarLabel:self.view];
+  [_pickerCoordinator stop];
+}
+
+- (void)pickerCancelPressed {
+  [_pickerCoordinator stop];
+}
+
+@end