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 37 matching lines @@
   if (type == autofill::kPostalCodeField)
     return autofill::ADDRESS_HOME_ZIP;
   if (type == autofill::kSortingCodeField)
     return autofill::ADDRESS_HOME_SORTING_CODE;
   if (type == autofill::kCountryField)
     return autofill::ADDRESS_HOME_COUNTRY;
   NOTREACHED();
   return autofill::UNKNOWN_TYPE;
 }
 
+const size_t kInvalidCountryIndex = static_cast<size_t>(-1);

[Mathieu, 2017/05/17 19:18:30]

isn't size_t unsigned?  What happens here?

[MAD, 2017/05/17 19:54:29]

It created a SIZE_MAX value without having to include the proper linmit.h which
isn't the same on Windows and other platforms :-(.

Added a comments.

+
 }  // namespace
 
 ShippingAddressEditorViewController::ShippingAddressEditorViewController(
     PaymentRequestSpec* spec,
     PaymentRequestState* state,
     PaymentRequestDialogView* dialog,
     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),
+      chosen_country_index_(kInvalidCountryIndex),
       failed_to_load_region_data_(false) {
+  UpdateCountries(nullptr);

[Mathieu, 2017/05/17 19:18:30]

nit: /*model=*/nullptr

[MAD, 2017/05/17 19:54:29]

Done.

   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.
-      }
+      if (model->countries().size() != countries_.size())
+        UpdateCountries(model.get());
       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_],
-                            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.
-          OnDataChanged(/*synchronous=*/false);
+      if (chosen_country_index_ < countries_.size()) {
+        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();
         }
+      } else {
+        failed_to_load_region_data_ = true;
+      }
+      if (failed_to_load_region_data_) {
+        // 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>();
 }
 
 void ShippingAddressEditorViewController::OnPerformAction(
     views::Combobox* sender) {
   EditorViewController::OnPerformAction(sender);
   if (sender->id() != autofill::ADDRESS_HOME_COUNTRY)
     return;
   DCHECK_GE(sender->selected_index(), 0);
   if (chosen_country_index_ != static_cast<size_t>(sender->selected_index())) {
     chosen_country_index_ = sender->selected_index();
     failed_to_load_region_data_ = false;
     // View update must be asynchronous to let the combobox finish performing
     // the action.
     OnDataChanged(/*synchronous=*/false);
   }
 }
 
 void ShippingAddressEditorViewController::UpdateEditorView() {
   EditorViewController::UpdateEditorView();
-  if (chosen_country_index_ > 0UL) {
+  if (chosen_country_index_ > 0UL &&
+      chosen_country_index_ < countries_.size()) {
     views::Combobox* country_combo_box = static_cast<views::Combobox*>(
         dialog()->GetViewByID(autofill::ADDRESS_HOME_COUNTRY));
     DCHECK(country_combo_box);
+    DCHECK_EQ(countries_.size(),
+              static_cast<size_t>(country_combo_box->GetRowCount()));
     country_combo_box->SetSelectedIndex(chosen_country_index_);
+  } else if (countries_.size() > 0UL) {
+    chosen_country_index_ = 0UL;
+  } else {
+    chosen_country_index_ = kInvalidCountryIndex;
   }
   // Ignore temporary profile once the editor view has been updated.
   temporary_profile_.reset(nullptr);

[Mathieu, 2017/05/17 19:18:30]

just reset() is fine here

[MAD, 2017/05/17 19:54:29]

Done.

 }
 
 base::string16 ShippingAddressEditorViewController::GetSheetTitle() {
   // TODO(crbug.com/712074): Editor title should reflect the missing information
   // in the case that one or more fields are missing.
   return profile_to_edit_ ? l10n_util::GetStringUTF16(IDS_PAYMENTS_EDIT_ADDRESS)
                           : l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_ADDRESS);
 }
 
 std::unique_ptr<views::Button>
 ShippingAddressEditorViewController::CreatePrimaryButton() {
   std::unique_ptr<views::Button> button(
       EditorViewController::CreatePrimaryButton());
   button->set_id(static_cast<int>(DialogViewID::SAVE_ADDRESS_BUTTON));
   return button;
 }
 
+void ShippingAddressEditorViewController::UpdateCountries(
+    autofill::CountryComboboxModel* model) {
+  autofill::CountryComboboxModel local_model;
+  if (!model) {
+    local_model.SetCountries(*state()->GetPersonalDataManager(),
+                             base::Callback<bool(const std::string&)>(),
+                             state()->GetApplicationLocale());
+    model = &local_model;
+  }
+
+  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, kept to make sure the size of the vector stays the same.
+      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;
+    }
+    // Make sure the the country was actually found in |countries_|, otherwise
+    // set |chosen_country_index_| as the default country at index 0.
+    if (chosen_country_index_ >= countries_.size()) {
+      // But only if there is at least one country.
+      if (countries_.size() > 0) {
+        LOG(ERROR) << "Unexpected country: " << chosen_country;
+        chosen_country_index_ = 0;
+        profile_to_edit_->SetInfo(country_type,
+                                  countries_[chosen_country_index_].second,
+                                  state()->GetApplicationLocale());
+      } else {
+        LOG(ERROR) << "Unexpected empty country list!";
+        chosen_country_index_ = kInvalidCountryIndex;
+      }
+    }
+  } else if (countries_.size() > 0) {
+    chosen_country_index_ = 0;
+  }
+}
+
 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 39 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 &&
+        controller_->chosen_country_index_ < controller_->countries_.size() &&
         !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