[Payment Request] Adds address normalization on iOS.

ios/chrome/browser/ui/payments/payment_request_coordinator.mm

Index: ios/chrome/browser/ui/payments/payment_request_coordinator.mm
diff --git a/ios/chrome/browser/ui/payments/payment_request_coordinator.mm b/ios/chrome/browser/ui/payments/payment_request_coordinator.mm
index f0e9e6d6f8b7d94acc91ed0a1c017eb57f0f314a..295ced5411a2d5a2146dc12a9eab2e9ac793e80b 100644
--- a/ios/chrome/browser/ui/payments/payment_request_coordinator.mm
+++ b/ios/chrome/browser/ui/payments/payment_request_coordinator.mm
@@ -19,16 +19,20 @@
 #include "components/autofill/core/browser/payments/full_card_request.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.h"
+#include "components/payments/core/address_normalizer_impl.h"
 #include "components/payments/core/payment_address.h"
 #include "components/payments/core/payment_request_data_util.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/strings/grit/components_strings.h"
 #include "ios/chrome/browser/application_context.h"
+#include "ios/chrome/browser/autofill/validation_rules_storage_factory.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/payments/payment_request.h"
 #include "ios/chrome/browser/payments/payment_request_util.h"
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 #include "ios/chrome/browser/ui/autofill/card_unmask_prompt_view_bridge.h"
+#include "third_party/libaddressinput/chromium/chrome_metadata_source.h"
+#include "third_party/libaddressinput/chromium/chrome_storage_impl.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -38,15 +42,51 @@
 namespace {
 using ::payments::data_util::GetBasicCardResponseFromAutofillCreditCard;
 using ::payments::data_util::GetPaymentAddressFromAutofillProfile;
-}  // namespace
+
+constexpr int kAddressNormalizationTimeoutSeconds = 5;
+
+// Receives notifications from the AddressNormalizer, and forwards the details
+// to a callback. This is needed as there is currently no way to know, when
+// the |payments::AddressNormalizer::Delegate| methods are called, which address
+// has been normalized (shipping or contact). The callback block can keep track
+// of which address this delegate was created for.
+class AddressNormalizerDelegate : public payments::AddressNormalizer::Delegate {

[Moe, 2017/04/27 12:19:53]

Please move this to its own files in i/c/b/payments. We should avoid putting
non-UI stuff under ui moving forward.
Could you construct this with a reference to the coordinator similar to
FullCardRequester? it can call a function on the coordinator once the delegate
method is called. That way, calling the normalizer for a profile is just a
single line and you'll have named functions in the coordinators that receive the
normalized profiles which you can copy or keep a reference to. 

[macourteau, 2017/04/27 18:07:52]

This is just a helper class, I think it makes sense to keep it near where it's
used, and in the anonymous namespace.
Doing that would render this helper class useless: if I'm calling a named
function on an object, might as well get rid of this class and just implement
OnAddressNormalized/OnCouldNotNormalize on the coordinator directly. The problem
this helper solves is that we can't know, when these delegate methods are
called, which address it was called for (shipping? contact? other?). Using a
block (or could be a base::Callback) lets us keep state to know exactly which
address we're getting called back for.

I suggested that sebsg@ refactor the AddressNormalizer to receive two
base::Callback's instead of a delegate pointer, so that might change and then we
can get rid of this altogether. We'll be able to base::Bind a pointer to the
AutofillProfile in the callback directly, and update that with the normalized
version.

[Moe, 2017/04/28 09:04:44]

Ah I see. Forgot about the need to keep track of the normalized profile.

[macourteau, 2017/05/01 15:56:22]

Acknowledged.

+ public:
+  // Block to be called with the profile information, once normalized.
+  typedef void (^Callback)(const autofill::AutofillProfile&);
+
+  AddressNormalizerDelegate() {}
+  ~AddressNormalizerDelegate() override {}
+
+  void SetCallback(Callback callback) { callback_ = callback; }
+
+  // payments::AddressNormalizer::Delegate:
+  void OnAddressNormalized(
+      const autofill::AutofillProfile& normalizedProfile) override {
+    DCHECK(callback_ != nil);

[Moe, 2017/04/27 12:19:53]

nit? DCHECK(callback_)

[macourteau, 2017/04/27 18:07:52]

Done.

+    callback_(normalizedProfile);
+  }
+
+  // payments::AddressNormalizer::Delegate:
+  void OnCouldNotNormalize(const autofill::AutofillProfile& profile) override {
+    // Since the phone number is formatted in either case, this profile should
+    // be used.
+    DCHECK(callback_ != nil);
+    callback_(profile);
+  }
+
+ private:
+  Callback callback_ = nil;

[Moe, 2017/04/27 12:19:53]

s/nil/nullptr for C++ objects

[macourteau, 2017/04/27 18:07:52]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(AddressNormalizerDelegate);
+};
 
 // The unmask prompt UI for Payment Request.
 class PRCardUnmaskPromptViewBridge

[Moe, 2017/04/27 12:19:53]

I'll move PRCardUnmaskPromptViewBridge and FullCardRequester to i/c/b/payments
myself.

[macourteau, 2017/04/27 18:07:52]

Acknowledged.

     : public autofill::CardUnmaskPromptViewBridge {
  public:
-  explicit PRCardUnmaskPromptViewBridge(
-      autofill::CardUnmaskPromptController* controller,
-      UIViewController* base_view_controller)
+  PRCardUnmaskPromptViewBridge(autofill::CardUnmaskPromptController* controller,
+                               UIViewController* base_view_controller)
       : autofill::CardUnmaskPromptViewBridge(controller),
         base_view_controller_(base_view_controller) {}
 
@@ -70,9 +110,9 @@ class FullCardRequester
       public autofill::payments::FullCardRequest::UIDelegate,
       public base::SupportsWeakPtr<FullCardRequester> {
  public:
-  explicit FullCardRequester(PaymentRequestCoordinator* owner,
-                             UIViewController* base_view_controller,
-                             ios::ChromeBrowserState* browser_state)
+  FullCardRequester(PaymentRequestCoordinator* owner,
+                    UIViewController* base_view_controller,
+                    ios::ChromeBrowserState* browser_state)
       : owner_(owner),
         base_view_controller_(base_view_controller),
         unmask_controller_(browser_state->GetPrefs(),
@@ -127,6 +167,8 @@ class FullCardRequester
   DISALLOW_COPY_AND_ASSIGN(FullCardRequester);
 };
 
+}  // namespace
+
 @implementation PaymentRequestCoordinator {
   UINavigationController* _navigationController;
   PaymentRequestViewController* _viewController;
@@ -142,6 +184,23 @@ class FullCardRequester
 
   // The selected shipping address, pending approval from the page.
   autofill::AutofillProfile* _pendingShippingAddress;
+
+  // The address normalizer, to normalize contact & shipping addresses. Also,
+  // delegates to handle callbacks for normalization of each address.
+  std::unique_ptr<payments::AddressNormalizer> _addressNormalizer;
+  AddressNormalizerDelegate _contactAddressNormalizerDelegate;
+  AddressNormalizerDelegate _shippingAddressNormalizerDelegate;
+
+  // Member variables to keep track of which operations are still pending.
+  BOOL _contactAddressNormalizationPending;
+  BOOL _shippingAddressNormalizationPending;
+  BOOL _fullCardRequestPending;
+
+  // Storage for the full card.
+  struct {
+    autofill::CreditCard creditCard;
+    base::string16 cvc;
+  } _fullCard;

[Moe, 2017/04/27 12:19:52]

please typedef this.

[macourteau, 2017/04/27 18:07:52]

Done.

 }
 
 @synthesize paymentRequest = _paymentRequest;
@@ -152,6 +211,33 @@ class FullCardRequester
 @synthesize pageHost = _pageHost;
 @synthesize delegate = _delegate;
 
+- (void)initAddressNormalizer {
+  autofill::PersonalDataManager* personalDataManager =
+      _paymentRequest->GetPersonalDataManager();
+
+  std::unique_ptr<i18n::addressinput::Source> addressNormalizerSource =
+      base::WrapUnique(new autofill::ChromeMetadataSource(

[Moe, 2017/04/27 12:19:53]

MakeUnique is preferred.

[macourteau, 2017/04/27 18:07:52]

Done.

+          I18N_ADDRESS_VALIDATION_DATA_URL,
+          personalDataManager->GetURLRequestContextGetter()));
+
+  std::unique_ptr<i18n::addressinput::Storage> addressNormalizerStorage =
+      autofill::ValidationRulesStorageFactory::CreateStorage();
+
+  _addressNormalizer.reset(new payments::AddressNormalizerImpl(
+      std::move(addressNormalizerSource), std::move(addressNormalizerStorage)));
+
+  // Kickoff the process of loading the rules (which is asynchronous) for each
+  // profile's country, to get faster address normalization later.
+  for (const autofill::AutofillProfile* profile :
+       personalDataManager->GetProfilesToSuggest()) {
+    std::string countryCode =
+        base::UTF16ToUTF8(profile->GetRawInfo(autofill::ADDRESS_HOME_COUNTRY));
+    if (autofill::data_util::IsValidCountryCode(countryCode)) {
+      _addressNormalizer->LoadRulesForRegion(countryCode);
+    }

[sebsg, 2017/04/26 20:52:51]

I don't know if you have access to that here but it could also be useful to load
the rules of the app locale.

+  }
+}
+
 - (void)start {
   _viewController = [[PaymentRequestViewController alloc]
       initWithPaymentRequest:_paymentRequest];
@@ -176,6 +262,8 @@ class FullCardRequester
   [[self baseViewController] presentViewController:_navigationController
                                           animated:YES
                                         completion:nil];
+
+  [self initAddressNormalizer];
 }
 
 - (void)stop {
@@ -196,87 +284,148 @@ class FullCardRequester
   _navigationController = nil;
 }
 
+- (void)normalizeProfile:(autofill::AutofillProfile*)profileToNormalize
+                delegate:(AddressNormalizerDelegate*)delegate
+                    flag:(BOOL*)normalizationRequestPendingFlag {
+  DCHECK(normalizationRequestPendingFlag);
+
+  __weak PaymentRequestCoordinator* weakSelf = self;
+
+  delegate->SetCallback(^(const autofill::AutofillProfile& normalizedProfile) {
+    // Check that PaymentRequestCoordinator still exists as |profileToNormalize|
+    // and |normalizationRequestPendingFlag| are member variables of that
+    // object.
+    PaymentRequestCoordinator* strongSelf = weakSelf;
+    if (!strongSelf)
+      return;
+
+    *profileToNormalize = normalizedProfile;
+    *normalizationRequestPendingFlag = NO;
+    [strongSelf maybeDone];
+  });
+
+  const std::string country_code = base::UTF16ToUTF8(
+      profileToNormalize->GetRawInfo(autofill::ADDRESS_HOME_COUNTRY));
+  if (autofill::data_util::IsValidCountryCode(country_code)) {

[sebsg, 2017/04/26 20:52:51]

If the country code is not valid, you could fallback to the one inferred from
the app_locale.

To get the country code from the app_locale:
autofill::AutofillCountry::CountryCodeForLocale(app_locale_);

[macourteau, 2017/04/27 18:07:52]

Done (with iOS APIs).

+    _addressNormalizer->StartAddressNormalization(
+        *profileToNormalize, country_code, kAddressNormalizationTimeoutSeconds,
+        delegate);
+  } else {
+    *normalizationRequestPendingFlag = NO;
+  }
+}
+
 - (void)sendPaymentResponse {
+  // Set these before starting any potentially asynchronous calls, to ensure we
+  // wait for completion of all operations before proceeding.
+  _contactAddressNormalizationPending = YES;
+  _shippingAddressNormalizationPending = _paymentRequest->request_shipping();
+  _fullCardRequestPending = YES;
+
   DCHECK(_paymentRequest->selected_credit_card());
   autofill::CreditCard* card = _paymentRequest->selected_credit_card();
   _fullCardRequester = base::MakeUnique<FullCardRequester>(
       self, _navigationController, _browserState);
   _fullCardRequester->GetFullCard(card, _autofillManager);
-}
-
-- (void)fullCardRequestDidSucceedWithCard:(const autofill::CreditCard&)card
-                                      CVC:(const base::string16&)cvc {
-  web::PaymentResponse paymentResponse;
-
-  // If the merchant specified the card network as part of the "basic-card"
-  // payment method, return "basic-card" as the method_name. Otherwise, return
-  // the name of the network directly.
-  std::string basic_card_type =
-      autofill::data_util::GetPaymentRequestData(card.type())
-          .basic_card_payment_type;
-  paymentResponse.method_name =
-      _paymentRequest->basic_card_specified_networks().find(basic_card_type) !=
-              _paymentRequest->basic_card_specified_networks().end()
-          ? base::ASCIIToUTF16("basic-card")
-          : base::ASCIIToUTF16(basic_card_type);
-
-  paymentResponse.details = GetBasicCardResponseFromAutofillCreditCard(
-      card, cvc, _paymentRequest->billing_profiles(),
-      GetApplicationContext()->GetApplicationLocale());
 
   if (_paymentRequest->request_shipping()) {
-    autofill::AutofillProfile* shippingAddress =
-        _paymentRequest->selected_shipping_profile();
-    // TODO(crbug.com/602666): User should get here only if they have selected
-    // a shipping address.
-    DCHECK(shippingAddress);
-    paymentResponse.shipping_address = GetPaymentAddressFromAutofillProfile(
-        *shippingAddress, GetApplicationContext()->GetApplicationLocale());
-
-    web::PaymentShippingOption* shippingOption =
-        _paymentRequest->selected_shipping_option();
-    DCHECK(shippingOption);
-    paymentResponse.shipping_option = shippingOption->id;
+    [self normalizeProfile:_paymentRequest->selected_shipping_profile()
+                  delegate:&_shippingAddressNormalizerDelegate
+                      flag:&_shippingAddressNormalizationPending];
   }
 
-  if (_paymentRequest->request_payer_name()) {
-    autofill::AutofillProfile* contactInfo =
-        _paymentRequest->selected_contact_profile();
-    // TODO(crbug.com/602666): User should get here only if they have selected
-    // a contact info.
-    DCHECK(contactInfo);
-    paymentResponse.payer_name =
-        contactInfo->GetInfo(autofill::AutofillType(autofill::NAME_FULL),
-                             GetApplicationContext()->GetApplicationLocale());
-  }
-
-  if (_paymentRequest->request_payer_email()) {
-    autofill::AutofillProfile* contactInfo =
-        _paymentRequest->selected_contact_profile();
-    // TODO(crbug.com/602666): User should get here only if they have selected
-    // a contact info.
-    DCHECK(contactInfo);
-    paymentResponse.payer_email =
-        contactInfo->GetRawInfo(autofill::EMAIL_ADDRESS);
-  }
+  [self normalizeProfile:_paymentRequest->selected_contact_profile()
+                delegate:&_contactAddressNormalizerDelegate
+                    flag:&_contactAddressNormalizationPending];
+}
 
-  if (_paymentRequest->request_payer_phone()) {
-    autofill::AutofillProfile* contactInfo =
-        _paymentRequest->selected_contact_profile();
-    // TODO(crbug.com/602666): User should get here only if they have selected
-    // a contact info.
-    DCHECK(contactInfo);
-    paymentResponse.payer_phone =
-        contactInfo->GetRawInfo(autofill::PHONE_HOME_WHOLE_NUMBER);
-  }
+- (void)fullCardRequestDidSucceedWithCard:(const autofill::CreditCard&)card
+                                      CVC:(const base::string16&)cvc {
+  _fullCard.creditCard = card;
+  _fullCard.cvc = cvc;
+  _fullCardRequestPending = NO;
 
   _viewController.view.userInteractionEnabled = NO;
   [_viewController setPending:YES];
   [_viewController loadModel];
   [[_viewController collectionView] reloadData];
 
-  [_delegate paymentRequestCoordinator:self
-         didConfirmWithPaymentResponse:paymentResponse];
+  [self maybeDone];
+}
+
+- (void)maybeDone {

[Moe, 2017/04/27 12:19:53]

nit: please change to something more descriptive like
sendPaymentResponseIfAllAsyncCallsCompleted
dont' be shy to be verbose in Objective-C :)

[macourteau, 2017/04/27 18:07:52]

Done. That *is* quite verbose 😳

+  if (_contactAddressNormalizationPending == NO &&
+      _shippingAddressNormalizationPending == NO &&
+      _fullCardRequestPending == NO) {
+    web::PaymentResponse paymentResponse;
+
+    // If the merchant specified the card network as part of the "basic-card"
+    // payment method, return "basic-card" as the method_name. Otherwise, return
+    // the name of the network directly.
+    std::string basic_card_type =
+        autofill::data_util::GetPaymentRequestData(_fullCard.creditCard.type())
+            .basic_card_payment_type;
+    paymentResponse.method_name =
+        _paymentRequest->basic_card_specified_networks().find(
+            basic_card_type) !=
+                _paymentRequest->basic_card_specified_networks().end()
+            ? base::ASCIIToUTF16("basic-card")
+            : base::ASCIIToUTF16(basic_card_type);
+
+    paymentResponse.details = GetBasicCardResponseFromAutofillCreditCard(
+        _fullCard.creditCard, _fullCard.cvc,
+        _paymentRequest->billing_profiles(),

[sebsg, 2017/04/26 20:52:51]

Also, you will eventually want to normalize that billing address too, shouldn't
be complicated with the nice framework for normalization you created.

[sebsg, 2017/04/26 20:52:51]

Just a heads-up I just landed a CL which adds a bit more behavior here,
basically finding the associated billing address and only sending that one to
GetBasicCardResponseFromAutofillCreditCard.

[macourteau, 2017/04/27 18:07:52]

Acknowledged, taken into account.

[macourteau, 2017/04/27 18:07:52]

Done.

+        GetApplicationContext()->GetApplicationLocale());
+
+    if (_paymentRequest->request_shipping()) {
+      autofill::AutofillProfile* shippingAddress =
+          _paymentRequest->selected_shipping_profile();
+      // TODO(crbug.com/602666): User should get here only if they have selected
+      // a shipping address.
+      DCHECK(shippingAddress);
+      paymentResponse.shipping_address = GetPaymentAddressFromAutofillProfile(
+          *shippingAddress, GetApplicationContext()->GetApplicationLocale());
+
+      web::PaymentShippingOption* shippingOption =
+          _paymentRequest->selected_shipping_option();
+      DCHECK(shippingOption);
+      paymentResponse.shipping_option = shippingOption->id;
+    }
+
+    if (_paymentRequest->request_payer_name()) {
+      autofill::AutofillProfile* contactInfo =
+          _paymentRequest->selected_contact_profile();
+      // TODO(crbug.com/602666): User should get here only if they have selected
+      // a contact info.
+      DCHECK(contactInfo);
+      paymentResponse.payer_name =
+          contactInfo->GetInfo(autofill::AutofillType(autofill::NAME_FULL),
+                               GetApplicationContext()->GetApplicationLocale());
+    }
+
+    if (_paymentRequest->request_payer_email()) {
+      autofill::AutofillProfile* contactInfo =
+          _paymentRequest->selected_contact_profile();
+      // TODO(crbug.com/602666): User should get here only if they have selected
+      // a contact info.
+      DCHECK(contactInfo);
+      paymentResponse.payer_email =
+          contactInfo->GetRawInfo(autofill::EMAIL_ADDRESS);
+    }
+
+    if (_paymentRequest->request_payer_phone()) {
+      autofill::AutofillProfile* contactInfo =
+          _paymentRequest->selected_contact_profile();
+      // TODO(crbug.com/602666): User should get here only if they have selected
+      // a contact info.
+      DCHECK(contactInfo);
+      paymentResponse.payer_phone =
+          contactInfo->GetRawInfo(autofill::PHONE_HOME_WHOLE_NUMBER);
+    }
+
+    [_delegate paymentRequestCoordinator:self
+           didConfirmWithPaymentResponse:paymentResponse];
+  }
 }
 
 - (void)updatePaymentDetails:(web::PaymentDetails)paymentDetails {