[Payments] Selecting incomplete items will open editors

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 <memory>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 58 matching lines @@
     return autofill::ADDRESS_HOME_COUNTRY;
   NOTREACHED();
   return autofill::UNKNOWN_TYPE;
 }
 
 }  // namespace
 
 ShippingAddressEditorViewController::ShippingAddressEditorViewController(
     PaymentRequestSpec* spec,
     PaymentRequestState* state,
-    PaymentRequestDialogView* dialog)
-    : EditorViewController(spec, state, dialog) {
+    PaymentRequestDialogView* dialog,
+    autofill::AutofillProfile* profile)
+    : EditorViewController(spec, state, dialog), profile_to_edit_(profile) {
   UpdateEditorFields();
 }
 
 ShippingAddressEditorViewController::~ShippingAddressEditorViewController() {}
 
 std::unique_ptr<views::View>
 ShippingAddressEditorViewController::CreateHeaderView() {
   return base::MakeUnique<views::View>();
 }
 
 std::vector<EditorField>
 ShippingAddressEditorViewController::GetFieldDefinitions() {
   return editor_fields_;
 }
 
+base::string16 ShippingAddressEditorViewController::GetInitialValueForType(
+    autofill::ServerFieldType type) {
+  if (!profile_to_edit_)
+    return base::string16();
+
+  // TODO(crbug.com/709451): Use GetInfo() here.

[anthonyvd, 2017/04/07 20:08:09]

Why not use it already? I'm not sure what in that bug is preventing us from
doing it if we know it's the correct thing.

[Mathieu, 2017/04/09 00:35:45]

You're right, I can get the app_locale

+  return profile_to_edit_->GetRawInfo(type);
+}
+
 bool ShippingAddressEditorViewController::ValidateModelAndSave() {
+  // To validate the profile first, we use a temporary object.
   autofill::AutofillProfile profile;
-  profile.set_origin(autofill::kSettingsOrigin);
   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())
       return false;
 
     profile.SetRawInfo(field.second.type, field.first->text());
   }
   for (const auto& field : comboboxes()) {
     // ValidatingCombobox* is the key, EditorField is the value.
     ValidatingCombobox* combobox = field.first;
     if (combobox->invalid())
       return false;
 
     if (combobox->id() == autofill::ADDRESS_HOME_COUNTRY) {
       profile.SetRawInfo(
           field.second.type,
           base::UTF8ToUTF16(country_codes_[combobox->selected_index()]));
     } else {
       profile.SetRawInfo(field.second.type,
                          combobox->GetTextForRow(combobox->selected_index()));
     }
   }
 
-  // Add the profile (will not add a duplicate).
-  state()->GetPersonalDataManager()->AddProfile(profile);
+  if (!profile_to_edit_) {
+    // Add the profile (will not add a duplicate).
+    profile.set_origin(autofill::kSettingsOrigin);

[anthonyvd, 2017/04/07 20:08:09]

Do we want a different origin for PaymentRequest?

[Mathieu, 2017/04/09 00:35:45]

I don't think so. "settings" means the profile has been verified by the user in
some way (it then becomes stronger than an inferred profile when merging with
others). Since this is an editor and the person has clicked Save, I think
keeping this origin is correct and equivalent to editing in chrome://settings.

+    state()->GetPersonalDataManager()->AddProfile(profile);
+  } else {
+    // Copy the temporary object's data to the object to be edited. Prefer this
+    // method to copying |profile| into |profile_to_edit_|, because the latter
+    // object needs to retain other properties (use count, use date, guid,
+    // etc.).
+    for (const auto& field : text_fields()) {
+      profile_to_edit_->SetRawInfo(field.second.type,

[anthonyvd, 2017/04/07 20:08:09]

If GetInfo is preferred over GetRawInfo above, shouldn't this be SetInfo in some
cases? 

[Mathieu, 2017/04/09 00:35:45]

Yeah I think we should play it safe and use GetInfo/SetInfo always.

+                                   profile.GetRawInfo(field.second.type));
+    }
+    for (const auto& field : comboboxes()) {
+      profile_to_edit_->SetRawInfo(field.second.type,
+                                   profile.GetRawInfo(field.second.type));
+    }
+    profile_to_edit_->set_origin(autofill::kSettingsOrigin);

[anthonyvd, 2017/04/07 20:08:09]

Same question re: origin.

[Mathieu, 2017/04/09 00:35:45]

Acknowledged.

+    state()->GetPersonalDataManager()->UpdateProfile(*profile_to_edit_);
+  }
 
   return true;
 }
 
 std::unique_ptr<ValidationDelegate>
 ShippingAddressEditorViewController::CreateValidationDelegate(
     const EditorField& field) {
   return base::MakeUnique<
       ShippingAddressEditorViewController::ShippingAddressValidationDelegate>(
       this, field);
@@ skipping 192 matching lines @@
   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