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

ios/chrome/browser/payments/payment_request_coordinator.mm

 // 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/payments/payment_request_coordinator.h"
 
 #include <unordered_set>
 #include <vector>
 
 #import "base/ios/weak_nsobject.h"
 #include "base/mac/objc_property_releaser.h"
 #include "base/mac/scoped_nsobject.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autofill_data_util.h"
 #include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/field_types.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "ios/chrome/browser/application_context.h"
 
 @interface PaymentRequestCoordinator () {
   autofill::PersonalDataManager* _personalDataManager;  // weak
   base::WeakNSProtocol<id<PaymentRequestCoordinatorDelegate>> _delegate;
   base::scoped_nsobject<UINavigationController> _navigationController;
   base::scoped_nsobject<PaymentRequestViewController> _viewController;
   base::scoped_nsobject<PaymentItemsDisplayCoordinator>
       _itemsDisplayCoordinator;
   base::scoped_nsobject<ShippingAddressSelectionCoordinator>
       _shippingAddressSelectionCoordinator;
+  base::scoped_nsobject<ShippingOptionSelectionCoordinator>
+      _shippingOptionSelectionCoordinator;
   base::scoped_nsobject<PaymentMethodSelectionCoordinator>
       _methodSelectionCoordinator;
 
   base::mac::ObjCPropertyReleaser _propertyReleaser_PaymentRequestCoordinator;
 }
 
 // Returns the credit cards available from |_personalDataManager| that match
 // a supported type specified in |_paymentRequest|.
 - (std::vector<autofill::CreditCard*>)supportedMethods;
 
 @end
 
 @implementation PaymentRequestCoordinator
 
 @synthesize paymentRequest = _paymentRequest;
 @synthesize pageFavicon = _pageFavicon;
 @synthesize pageTitle = _pageTitle;
 @synthesize pageHost = _pageHost;
 @synthesize selectedShippingAddress = _selectedShippingAddress;
+@synthesize selectedShippingOption = _selectedShippingOption;
+
 @synthesize selectedPaymentMethod = _selectedPaymentMethod;
 
 - (instancetype)initWithBaseViewController:(UIViewController*)baseViewController
                        personalDataManager:
                            (autofill::PersonalDataManager*)personalDataManager {
   if ((self = [super initWithBaseViewController:baseViewController])) {
     _propertyReleaser_PaymentRequestCoordinator.Init(
         self, [PaymentRequestCoordinator class]);
     _personalDataManager = personalDataManager;
   }
   return self;
 }
 
 - (id<PaymentRequestCoordinatorDelegate>)delegate {
   return _delegate.get();
 }
 
 - (void)setDelegate:(id<PaymentRequestCoordinatorDelegate>)delegate {
   _delegate.reset(delegate);
 }
 
 - (void)start {
   const std::vector<autofill::AutofillProfile*> addresses =
       _personalDataManager->GetProfilesToSuggest();
   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];
 
   _viewController.reset([[PaymentRequestViewController alloc] init]);
   [_viewController setPaymentRequest:_paymentRequest];
   [_viewController setPageFavicon:_pageFavicon];
   [_viewController setPageTitle:_pageTitle];
   [_viewController setPageHost:_pageHost];
   [_viewController setSelectedShippingAddress:_selectedShippingAddress];
+  [_viewController setSelectedShippingOption:_selectedShippingOption];
   [_viewController setSelectedPaymentMethod:_selectedPaymentMethod];
   [_viewController setDelegate:self];
   [_viewController loadModel];
 
   _navigationController.reset([[UINavigationController alloc]
       initWithRootViewController:_viewController]);
   [_navigationController setNavigationBarHidden:YES];
 
   [[self baseViewController] presentViewController:_navigationController
                                           animated:YES
                                         completion:nil];
 }
 
 - (void)stop {
   [_navigationController dismissViewControllerAnimated:YES completion:nil];
   _itemsDisplayCoordinator.reset();
   _shippingAddressSelectionCoordinator.reset();
+  _shippingOptionSelectionCoordinator.reset();
   _methodSelectionCoordinator.reset();
   _navigationController.reset();
   _viewController.reset();
 }
 
 - (std::vector<autofill::CreditCard*>)supportedMethods {
   std::vector<autofill::CreditCard*> supported_methods;
 
   std::unordered_set<base::string16> supported_method_types;
   for (web::PaymentMethodData method_data : _paymentRequest.method_data) {
@@ skipping 98 matching lines @@
   const std::vector<autofill::AutofillProfile*> addresses =
       _personalDataManager->GetProfilesToSuggest();
   [_shippingAddressSelectionCoordinator setShippingAddresses:addresses];
   [_shippingAddressSelectionCoordinator
       setSelectedShippingAddress:_selectedShippingAddress];
   [_shippingAddressSelectionCoordinator setDelegate:self];
 
   [_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]);
   [_methodSelectionCoordinator setPaymentMethods:[self supportedMethods]];
   [_methodSelectionCoordinator setSelectedPaymentMethod:_selectedPaymentMethod];
   [_methodSelectionCoordinator setDelegate:self];
 
   [_methodSelectionCoordinator start];
 }
 
@@ skipping 22 matching lines @@
   [_shippingAddressSelectionCoordinator stop];
   _shippingAddressSelectionCoordinator.reset();
 }
 
 - (void)shippingAddressSelectionCoordinatorDidReturn:
     (ShippingAddressSelectionCoordinator*)coordinator {
   [_shippingAddressSelectionCoordinator stop];
   _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:
             (PaymentMethodSelectionCoordinator*)coordinator
                     selectedPaymentMethod:(autofill::CreditCard*)creditCard {
   _selectedPaymentMethod = creditCard;
 
   [_viewController setSelectedPaymentMethod:creditCard];
   [_viewController loadModel];
   [[_viewController collectionView] reloadData];
 
   [_methodSelectionCoordinator stop];
   _methodSelectionCoordinator.reset();
 }
 
 - (void)paymentMethodSelectionCoordinatorDidReturn:
     (PaymentMethodSelectionCoordinator*)coordinator {
   [_methodSelectionCoordinator stop];
   _methodSelectionCoordinator.reset();
 }
 
 @end