[Payment Request] Refactors the remaining selector view controllers

ios/chrome/browser/payments/payment_method_selection_coordinator.mm

Index: ios/chrome/browser/payments/payment_method_selection_coordinator.mm
diff --git a/ios/chrome/browser/payments/payment_method_selection_coordinator.mm b/ios/chrome/browser/payments/payment_method_selection_coordinator.mm
index 9385ea0afb7a3022c52f5238ff3ef104fc662821..c92f5eec770081dd62fc6e2fca689af6fa05a6d8 100644
--- a/ios/chrome/browser/payments/payment_method_selection_coordinator.mm
+++ b/ios/chrome/browser/payments/payment_method_selection_coordinator.mm
@@ -5,18 +5,24 @@
 #import "ios/chrome/browser/payments/payment_method_selection_coordinator.h"
 
 #include "components/autofill/core/browser/credit_card.h"
+#include "components/strings/grit/components_strings.h"
+#include "ios/chrome/browser/payments/payment_method_selection_mediator.h"
 #include "ios/chrome/browser/payments/payment_request.h"
+#include "ui/base/l10n/l10n_util.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
-@interface PaymentMethodSelectionCoordinator () {
-  CreditCardEditCoordinator* _creditCardEditCoordinator;
-}
+@interface PaymentMethodSelectionCoordinator ()
+
+@property(nonatomic, strong)
+    CreditCardEditCoordinator* creditCardEditCoordinator;
 
 @property(nonatomic, strong)
-    PaymentMethodSelectionViewController* viewController;
+    PaymentRequestSelectorViewController* viewController;
+
+@property(nonatomic, strong) PaymentMethodSelectionMediator* mediator;
 
 // Called when the user selects a payment method. The cell is checked, the
 // UI is locked so that the user can't interact with it, then the delegate is
@@ -29,85 +35,93 @@
 @implementation PaymentMethodSelectionCoordinator
 @synthesize paymentRequest = _paymentRequest;
 @synthesize delegate = _delegate;
+@synthesize creditCardEditCoordinator = _creditCardEditCoordinator;
 @synthesize viewController = _viewController;
+@synthesize mediator = _mediator;
 
 - (void)start {
-  _viewController = [[PaymentMethodSelectionViewController alloc]
-      initWithPaymentRequest:_paymentRequest];
-  [_viewController setDelegate:self];
-  [_viewController loadModel];
-
-  DCHECK([self baseViewController].navigationController);
-  [[self baseViewController].navigationController
-      pushViewController:_viewController
+  self.mediator = [[PaymentMethodSelectionMediator alloc]

[macourteau, 2017/04/19 18:03:13]

Any reason we're accessing these through the getters/setters and not mutating
the variables directly?

[Moe, 2017/04/19 20:12:43]

It's a matter of Objective-C style guide. It's safer and more "correct" to
access the properties through accessors than access the synthesized properties
directly, ***unless you're in -init or the function you're in can be called from
-init***. The reason is that there could be things going on behind the scenes
such as readonly-ness, copying, etc that you could bypass by accessing the
synthesized properties directly. some reviewers are more strict about this than
others and as you can see the older code doesn't follow this pattern. We should
write new code following this pattern or at the very minimum not mix and match!

[macourteau, 2017/04/20 14:42:59]

Ha, interesting. I would have thought that direct access made more sense from
within the class, but I guess this makes sense. Thanks for the clarification1

+      initWithPaymentRequest:self.paymentRequest];
+
+  self.viewController = [[PaymentRequestSelectorViewController alloc] init];
+  self.viewController.title =
+      l10n_util::GetNSString(IDS_PAYMENTS_METHOD_OF_PAYMENT_LABEL);
+  self.viewController.delegate = self;
+  self.viewController.dataSource = self.mediator;
+  [self.viewController loadModel];
+
+  DCHECK(self.baseViewController.navigationController);
+  [self.baseViewController.navigationController
+      pushViewController:self.viewController
                 animated:YES];
 }
 
 - (void)stop {
-  [[self baseViewController].navigationController
-      popViewControllerAnimated:YES];
-  [_creditCardEditCoordinator stop];
-  _creditCardEditCoordinator = nil;
-  _viewController = nil;
+  [self.baseViewController.navigationController popViewControllerAnimated:YES];
+  [self.creditCardEditCoordinator stop];
+  self.creditCardEditCoordinator = nil;
+  self.viewController = nil;
+  self.mediator = nil;
 }
 
-#pragma mark - PaymentMethodSelectionViewControllerDelegate
+#pragma mark - PaymentRequestSelectorViewControllerDelegate
 
-- (void)paymentMethodSelectionViewController:
-            (PaymentMethodSelectionViewController*)controller
-                      didSelectPaymentMethod:
-                          (autofill::CreditCard*)paymentMethod {
-  [self delayedNotifyDelegateOfSelection:paymentMethod];
-}
+- (void)paymentRequestSelectorViewController:
+            (PaymentRequestSelectorViewController*)controller
+                        didSelectItemAtIndex:(NSUInteger)index {
+  // Update the data source with the selection.
+  self.mediator.selectedItemIndex = index;
 
-- (void)paymentMethodSelectionViewControllerDidReturn:
-    (PaymentMethodSelectionViewController*)controller {
-  [_delegate paymentMethodSelectionCoordinatorDidReturn:self];
+  DCHECK(index < self.paymentRequest->credit_cards().size());
+  [self delayedNotifyDelegateOfSelection:self.paymentRequest
+                                             ->credit_cards()[index]];
 }
 
-- (void)delayedNotifyDelegateOfSelection:(autofill::CreditCard*)paymentMethod {
-  _viewController.view.userInteractionEnabled = NO;
-  __weak PaymentMethodSelectionCoordinator* weakSelf = self;
-  dispatch_after(dispatch_time(DISPATCH_TIME_NOW,
-                               static_cast<int64_t>(0.2 * NSEC_PER_SEC)),
-                 dispatch_get_main_queue(), ^{
-                   PaymentMethodSelectionCoordinator* strongSelf = weakSelf;
-                   // Early return if the coordinator has been deallocated.
-                   if (!strongSelf)
-                     return;
-
-                   strongSelf.viewController.view.userInteractionEnabled = YES;
-                   [strongSelf.delegate
-                       paymentMethodSelectionCoordinator:strongSelf
-                                  didSelectPaymentMethod:paymentMethod];
-                 });
+- (void)paymentRequestSelectorViewControllerDidFinish:
+    (PaymentRequestSelectorViewController*)controller {
+  [self.delegate paymentMethodSelectionCoordinatorDidReturn:self];
 }
 
-- (void)paymentMethodSelectionViewControllerDidSelectAddCard:
-    (PaymentMethodSelectionViewController*)controller {
-  _creditCardEditCoordinator = [[CreditCardEditCoordinator alloc]
-      initWithBaseViewController:_viewController];
-  [_creditCardEditCoordinator setPaymentRequest:_paymentRequest];
-  [_creditCardEditCoordinator setDelegate:self];
-  [_creditCardEditCoordinator start];
+- (void)paymentRequestSelectorViewControllerDidSelectAddItem:
+    (PaymentRequestSelectorViewController*)controller {
+  self.creditCardEditCoordinator = [[CreditCardEditCoordinator alloc]
+      initWithBaseViewController:self.viewController];
+  self.creditCardEditCoordinator.paymentRequest = self.paymentRequest;
+  self.creditCardEditCoordinator.delegate = self;
+  [self.creditCardEditCoordinator start];
 }
 
 #pragma mark - CreditCardEditCoordinatorDelegate
 
 - (void)creditCardEditCoordinator:(CreditCardEditCoordinator*)coordinator
        didFinishEditingCreditCard:(autofill::CreditCard*)creditCard {
-  [_creditCardEditCoordinator stop];
-  _creditCardEditCoordinator = nil;
+  [self.creditCardEditCoordinator stop];
+  self.creditCardEditCoordinator = nil;
 
-  // Inform |_delegate| that this card has been selected.
-  [_delegate paymentMethodSelectionCoordinator:self
-                        didSelectPaymentMethod:creditCard];
+  // Inform |self.delegate| that this card has been selected.
+  [self.delegate paymentMethodSelectionCoordinator:self
+                            didSelectPaymentMethod:creditCard];
 }
 
 - (void)creditCardEditCoordinatorDidCancel:
     (CreditCardEditCoordinator*)coordinator {
-  [_creditCardEditCoordinator stop];
-  _creditCardEditCoordinator = nil;
+  [self.creditCardEditCoordinator stop];
+  self.creditCardEditCoordinator = nil;
+}
+
+#pragma mark - Helper methods
+
+- (void)delayedNotifyDelegateOfSelection:(autofill::CreditCard*)paymentMethod {
+  self.viewController.view.userInteractionEnabled = NO;
+  __weak PaymentMethodSelectionCoordinator* weakSelf = self;
+  dispatch_after(dispatch_time(DISPATCH_TIME_NOW,
+                               static_cast<int64_t>(0.2 * NSEC_PER_SEC)),

[macourteau, 2017/04/19 18:03:13]

nit: maybe make this a constant at top of file?

[Moe, 2017/04/19 20:12:43]

Done.

+                 dispatch_get_main_queue(), ^{
+                   weakSelf.viewController.view.userInteractionEnabled = YES;
+                   [weakSelf.delegate
+                       paymentMethodSelectionCoordinator:weakSelf
+                                  didSelectPaymentMethod:paymentMethod];
+                 });
 }
 
 @end