Payment request shipping address editor now properly handles countries

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

 // Copyright 2017 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.
 
 #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"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
 #include "chrome/browser/ui/views/payments/validating_combobox.h"
 #include "chrome/browser/ui/views/payments/validating_textfield.h"
@@ skipping 53 matching lines @@
     BackNavigationType back_navigation_type,
     base::OnceClosure on_edited,
     base::OnceCallback<void(const autofill::AutofillProfile&)> on_added,
     autofill::AutofillProfile* profile)
     : EditorViewController(spec, state, dialog, back_navigation_type),
       on_edited_(std::move(on_edited)),
       on_added_(std::move(on_added)),
       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();
 }
 
 ShippingAddressEditorViewController::~ShippingAddressEditorViewController() {}
 
 std::vector<EditorField>
 ShippingAddressEditorViewController::GetFieldDefinitions() {
   return editor_fields_;
 }
 
@@ skipping 52 matching lines @@
 std::unique_ptr<ui::ComboboxModel>
 ShippingAddressEditorViewController::GetComboboxModelForType(
     const autofill::ServerFieldType& type) {
   switch (type) {
     case autofill::ADDRESS_HOME_COUNTRY: {
       std::unique_ptr<autofill::CountryComboboxModel> model =
           base::MakeUnique<autofill::CountryComboboxModel>();
       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()) {
         // If the data was already pre-loaded, the observer won't get notified
         // 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);
         }
       }
       return std::move(model);
     }
     default:
       NOTREACHED();
       break;
   }
   return std::unique_ptr<ui::ComboboxModel>();
@@ skipping 37 matching lines @@
 ShippingAddressEditorViewController::CreatePrimaryButton() {
   std::unique_ptr<views::Button> button(
       EditorViewController::CreatePrimaryButton());
   button->set_id(static_cast<int>(DialogViewID::SAVE_ADDRESS_BUTTON));
   return button;
 }
 
 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;
   autofill::GetAddressComponents(chosen_country_code,
                                  state()->GetApplicationLocale(),
                                  components.get(), &unused);
   for (size_t line_index = 0; line_index < components->GetSize();
        ++line_index) {
     const base::ListValue* line = nullptr;
     if (!components->GetList(line_index, &line)) {
@@ skipping 72 matching lines @@
         FROM_HERE,
         base::BindOnce(&ShippingAddressEditorViewController::UpdateEditorView,
                        base::Unretained(this)));
   }
 }
 
 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.
     field.first->OnBlur();
     // ValidatingTextfield* is the key, EditorField is the value.
     if (field.first->invalid()) {
       success = false;
     } else {
       success = profile->SetInfo(autofill::AutofillType(field.second.type),
                                  field.first->text(), locale);
     }
     LOG_IF(ERROR, !success && !ignore_errors)
         << "Can't setinfo(" << field.second.type << ", " << field.first->text();
     if (!success && !ignore_errors)
       return false;
   }
   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 << ", "
         << combobox->GetTextForRow(combobox->selected_index());
     if (!success && !ignore_errors)
       return false;
   }
   return success;
 }
 
@@ skipping 40 matching lines @@
 void ShippingAddressEditorViewController::ShippingAddressValidationDelegate::
     ComboboxModelChanged(views::Combobox* combobox) {
   controller_->OnComboboxModelChanged(combobox);
 }
 
 bool ShippingAddressEditorViewController::ShippingAddressValidationDelegate::
     ValidateValue(const base::string16& value) {
   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));
       return false;
     }
     // As long as other field types are non-empty, they are valid.
     controller_->DisplayErrorMessageForField(field_, base::ASCIIToUTF16(""));
     return true;
   }
   bool is_required_valid = !field_.required;
   const base::string16 displayed_message =
       is_required_valid ? base::ASCIIToUTF16("")
                         : l10n_util::GetStringUTF16(
                               IDS_PAYMENTS_FIELD_REQUIRED_VALIDATION_MESSAGE);
   controller_->DisplayErrorMessageForField(field_, displayed_message);
   return is_required_valid;
 }
 
 }  // namespace payments