Selected shipping option in payment summary view + shipping option selection view

ios/chrome/browser/payments/payment_request_coordinator.mm

Index: ios/chrome/browser/payments/payment_request_coordinator.mm
diff --git a/ios/chrome/browser/payments/payment_request_coordinator.mm b/ios/chrome/browser/payments/payment_request_coordinator.mm
index 525b491ffef810dcae403d19afd65a7241ae5db3..29bb468c25547f12c56c7157997149f82e39f437 100644
--- a/ios/chrome/browser/payments/payment_request_coordinator.mm
+++ b/ios/chrome/browser/payments/payment_request_coordinator.mm
@@ -27,6 +27,8 @@
       _itemsDisplayCoordinator;
   base::scoped_nsobject<ShippingAddressSelectionCoordinator>
       _shippingAddressSelectionCoordinator;
+  base::scoped_nsobject<ShippingOptionSelectionCoordinator>
+      _shippingOptionSelectionCoordinator;
   base::scoped_nsobject<PaymentMethodSelectionCoordinator>
       _methodSelectionCoordinator;
 
@@ -46,6 +48,8 @@
 @synthesize pageTitle = _pageTitle;
 @synthesize pageHost = _pageHost;
 @synthesize selectedShippingAddress = _selectedShippingAddress;
+@synthesize selectedShippingOption = _selectedShippingOption;
+
 @synthesize selectedPaymentMethod = _selectedPaymentMethod;
 
 - (instancetype)initWithBaseViewController:(UIViewController*)baseViewController
@@ -73,6 +77,16 @@
   if (addresses.size() > 0)
     _selectedShippingAddress = addresses[0];
 
+  for (size_t i = 0; i < _paymentRequest.details.shipping_options.size(); ++i) {
+    web::PaymentShippingOption* shippingOption =
+        &_paymentRequest.details.shipping_options[i];
+    if (shippingOption->selected) {
+      // The selected shipping option must be the last shipping option with

[Justin Donnelly, 2017/01/10 17:11:53]

At first, I thought you were saying here that the last option in the list had to
have selected set. How about wording this more like how the spec puts it?
Something like, "If more than one option has |selected| set, the last one in the
sequence should be treated as the selected item."

[Moe, 2017/01/12 00:06:18]

Done.

+      // selected set to true.
+      _selectedShippingOption = shippingOption;
+    }
+  }
+
   const std::vector<autofill::CreditCard*> cards = [self supportedMethods];
   if (cards.size() > 0)
     _selectedPaymentMethod = cards[0];
@@ -83,6 +97,7 @@
   [_viewController setPageTitle:_pageTitle];
   [_viewController setPageHost:_pageHost];
   [_viewController setSelectedShippingAddress:_selectedShippingAddress];
+  [_viewController setSelectedShippingOption:_selectedShippingOption];
   [_viewController setSelectedPaymentMethod:_selectedPaymentMethod];
   [_viewController setDelegate:self];
   [_viewController loadModel];
@@ -100,6 +115,7 @@
   [_navigationController dismissViewControllerAnimated:YES completion:nil];
   _itemsDisplayCoordinator.reset();
   _shippingAddressSelectionCoordinator.reset();
+  _shippingOptionSelectionCoordinator.reset();
   _methodSelectionCoordinator.reset();
   _navigationController.reset();
   _viewController.reset();
@@ -218,6 +234,26 @@
   [_shippingAddressSelectionCoordinator start];
 }
 
+- (void)paymentRequestViewControllerSelectShippingOption {
+  _shippingOptionSelectionCoordinator.reset(
+      [[ShippingOptionSelectionCoordinator alloc]
+          initWithBaseViewController:_viewController]);
+
+  std::vector<web::PaymentShippingOption*> shippingOptions;
+  shippingOptions.reserve(_paymentRequest.details.shipping_options.size());
+  std::transform(std::begin(_paymentRequest.details.shipping_options),
+                 std::end(_paymentRequest.details.shipping_options),
+                 std::back_inserter(shippingOptions),
+                 [](web::PaymentShippingOption& option) { return &option; });
+
+  [_shippingOptionSelectionCoordinator setShippingOptions:shippingOptions];

[Justin Donnelly, 2017/01/10 17:11:53]

Why not just pass _paymentRequest.details.shipping_options here (as we do with
_paymentRequest.details.display_items above)? If you're just trying to pass
pointers instead of copies of these objects, I'd strongly prefer to avoid that.
It complicates the code for no meaningful efficiency gain.

[Moe, 2017/01/12 00:06:18]

The purpose for that wasn't efficiency gain. I wanted to keep the implementation
as close as possible to the shipping address selection view/logic since the two
are extremely similar. I think it makes the maintenance easier too as the two
will likely have similar bugs and features can literally be copy/pasted across.
Also I think this a is slightly different case than the display items because
that doesn't deal with selection and communicating it back. I find it more
straightforward to work with pointers than values when keeping track of a an
item in a list of items. If you agree, ever so slightly, with those reasons
let's keep it this way? :) 

+  [_shippingOptionSelectionCoordinator
+      setSelectedShippingOption:_selectedShippingOption];
+  [_shippingOptionSelectionCoordinator setDelegate:self];
+
+  [_shippingOptionSelectionCoordinator start];
+}
+
 - (void)paymentRequestViewControllerSelectPaymentMethod {
   _methodSelectionCoordinator.reset([[PaymentMethodSelectionCoordinator alloc]
       initWithBaseViewController:_viewController]);
@@ -260,6 +296,25 @@
   _shippingAddressSelectionCoordinator.reset();
 }
 
+#pragma mark - ShippingOptionSelectionCoordinatorDelegate
+
+- (void)shippingOptionSelectionCoordinator:
+            (ShippingOptionSelectionCoordinator*)coordinator
+                    selectedShippingOption:
+                        (web::PaymentShippingOption*)shippingOption {
+  _selectedShippingOption = shippingOption;
+  [_viewController updateSelectedShippingOption:shippingOption];
+
+  [_shippingOptionSelectionCoordinator stop];
+  _shippingOptionSelectionCoordinator.reset();
+}
+
+- (void)shippingOptionSelectionCoordinatorDidReturn:
+    (ShippingAddressSelectionCoordinator*)coordinator {
+  [_shippingOptionSelectionCoordinator stop];
+  _shippingOptionSelectionCoordinator.reset();
+}
+
 #pragma mark - PaymentMethodSelectionCoordinatorDelegate
 
 - (void)paymentMethodSelectionCoordinator: