Payment request shipping address editor now properly handles countries

chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc

Index: chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
diff --git a/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc b/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
index 94732c0d02ea5cdebfef4dd08cdf11d99f8a05a3..711d7a1885a20692ae70abc619107545d3b16359 100644
--- a/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
+++ b/chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc
@@ -4,8 +4,6 @@
 
 #include "chrome/browser/ui/views/payments/shipping_address_editor_view_controller.h"
 
-#include <utility>
-
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -79,6 +77,40 @@ ShippingAddressEditorViewController::ShippingAddressEditorViewController(
       profile_to_edit_(profile),
       chosen_country_index_(0),
       failed_to_load_region_data_(false) {
+  autofill::CountryComboboxModel model;

[Mathieu, 2017/05/16 20:10:06]

I would put lines 80-112 inside a private function perhaps?

[MAD, 2017/05/17 12:38:11]

Done.

+  model.SetCountries(*state->GetPersonalDataManager(),
+                     base::Callback<bool(const std::string&)>(),
+                     state->GetApplicationLocale());
+  for (size_t i = 0; i < model.countries().size(); ++i) {
+    autofill::AutofillCountry* country(model.countries()[i].get());
+    if (country) {
+      countries_.push_back(
+          std::make_pair(country->country_code(), country->name()));
+    } else {
+      // Separator.
+      countries_.push_back(std::make_pair("", base::UTF8ToUTF16("")));
+    }
+  }
+  // If there is a profile to edit, make sure to use its country for the initial
+  // |chosen_country_index_|.
+  if (profile_to_edit_) {
+    autofill::AutofillType country_type(autofill::ADDRESS_HOME_COUNTRY);
+    base::string16 chosen_country(
+        profile_to_edit_->GetInfo(country_type, state->GetApplicationLocale()));
+    for (chosen_country_index_ = 0; chosen_country_index_ < countries_.size();
+         ++chosen_country_index_) {
+      if (chosen_country == countries_[chosen_country_index_].second)
+        break;
+    }
+    if (chosen_country_index_ >= countries_.size()) {
+      LOG(ERROR) << "Unexpected country: " << chosen_country;
+      CHECK_GT(countries_.size(), 0);

[Mathieu, 2017/05/16 20:10:06]

could this ever happen in the wild? Crashing is pretty extreme... is it a case
where we simply can't go on?

[MAD, 2017/05/17 12:38:10]

It would crash below otherwise... 

But now that you mention it, let's make this more bulletproof. OK?

+      chosen_country_index_ = 0;
+      profile_to_edit_->SetInfo(country_type,
+                                countries_[chosen_country_index_].second,
+                                state->GetApplicationLocale());
+    }
+  }
   UpdateEditorFields();
 }
 
@@ -151,19 +183,12 @@ ShippingAddressEditorViewController::GetComboboxModelForType(
       model->SetCountries(*state()->GetPersonalDataManager(),
                           base::Callback<bool(const std::string&)>(),
                           state()->GetApplicationLocale());
-      country_codes_.clear();
-      for (size_t i = 0; i < model->countries().size(); ++i) {
-        if (model->countries()[i].get())
-          country_codes_.push_back(model->countries()[i]->country_code());
-        else
-          country_codes_.push_back("");  // Separator.
-      }
       return std::move(model);
     }
     case autofill::ADDRESS_HOME_STATE: {
       std::unique_ptr<autofill::RegionComboboxModel> model =
           base::MakeUnique<autofill::RegionComboboxModel>();
-      model->LoadRegionData(country_codes_[chosen_country_index_],
+      model->LoadRegionData(countries_[chosen_country_index_].first,
                             state()->GetRegionDataLoader(),
                             /*timeout_ms=*/5000);
       if (!model->IsPendingRegionDataLoad()) {
@@ -171,7 +196,7 @@ ShippingAddressEditorViewController::GetComboboxModelForType(
         // so we have to check for failure here.
         failed_to_load_region_data_ = model->failed_to_load_data();
         if (failed_to_load_region_data_) {
-          // We can update the view synchronously while building the view.
+          // We can't update the view synchronously while building the view.
           OnDataChanged(/*synchronous=*/false);
         }
       }
@@ -229,8 +254,8 @@ ShippingAddressEditorViewController::CreatePrimaryButton() {
 void ShippingAddressEditorViewController::UpdateEditorFields() {
   editor_fields_.clear();
   std::string chosen_country_code;
-  if (chosen_country_index_ < country_codes_.size())
-    chosen_country_code = country_codes_[chosen_country_index_];
+  if (chosen_country_index_ < countries_.size())
+    chosen_country_code = countries_[chosen_country_index_].first;
 
   std::unique_ptr<base::ListValue> components(new base::ListValue);
   std::string unused;
@@ -323,6 +348,25 @@ bool ShippingAddressEditorViewController::SaveFieldsToProfile(
     autofill::AutofillProfile* profile,
     bool ignore_errors) {
   const std::string& locale = state()->GetApplicationLocale();
+  // The country must be set first, because the profile uses the country to
+  // interpret some of the data (e.g., phone numbers) passed to SetInfo.
+  views::Combobox* combobox = static_cast<views::Combobox*>(
+      dialog()->GetViewByID(autofill::ADDRESS_HOME_COUNTRY));
+  // The combobox can be null when saving to temporary profile while updating
+  // the view.
+  if (combobox) {
+    base::string16 country(combobox->GetTextForRow(combobox->selected_index()));
+    bool success =
+        profile->SetInfo(autofill::AutofillType(autofill::ADDRESS_HOME_COUNTRY),
+                         country, locale);
+    LOG_IF(ERROR, !success && !ignore_errors)
+        << "Can't set profile country to: " << country;
+    if (!success && !ignore_errors)
+      return false;
+  } else {
+    DCHECK_EQ(temporary_profile_.get(), profile);
+  }
+
   bool success = true;
   for (const auto& field : text_fields()) {
     // Force a blur in case the value was left untouched.
@@ -342,19 +386,15 @@ bool ShippingAddressEditorViewController::SaveFieldsToProfile(
   for (const auto& field : comboboxes()) {
     // ValidatingCombobox* is the key, EditorField is the value.
     ValidatingCombobox* combobox = field.first;
+    // The country has already been dealt with.
+    if (combobox->id() == autofill::ADDRESS_HOME_COUNTRY)
+      continue;
     if (combobox->invalid()) {
       success = false;
     } else {
-      if (combobox->id() == autofill::ADDRESS_HOME_COUNTRY) {
-        success = profile->SetInfo(
-            autofill::AutofillType(field.second.type),
-            base::UTF8ToUTF16(country_codes_[combobox->selected_index()]),
-            locale);
-      } else {
-        success = profile->SetInfo(
-            autofill::AutofillType(field.second.type),
-            combobox->GetTextForRow(combobox->selected_index()), locale);
-      }
+      success = profile->SetInfo(
+          autofill::AutofillType(field.second.type),
+          combobox->GetTextForRow(combobox->selected_index()), locale);
     }
     LOG_IF(ERROR, !success && !ignore_errors)
         << "Can't setinfo(" << field.second.type << ", "
@@ -415,8 +455,8 @@ bool ShippingAddressEditorViewController::ShippingAddressValidationDelegate::
   if (!value.empty()) {
     if (field_.type == autofill::PHONE_HOME_WHOLE_NUMBER &&
         !autofill::IsValidPhoneNumber(
-            value,
-            controller_->country_codes_[controller_->chosen_country_index_])) {
+            value, controller_->countries_[controller_->chosen_country_index_]
+                       .first)) {
       controller_->DisplayErrorMessageForField(
           field_, l10n_util::GetStringUTF16(
                       IDS_PAYMENTS_PHONE_INVALID_VALIDATION_MESSAGE));