Preserving first/middle/last names when an Autofill profile is submitted

chrome/browser/ui/webui/options/autofill_options_handler.cc

 // Copyright (c) 2012 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/webui/options/autofill_options_handler.h"
 
 #include <stddef.h>
 
 #include <vector>
 
@@ skipping 190 matching lines @@
   localized_strings->Set("autofillDefaultCountryComponents",
                          default_country_components.release());
   localized_strings->SetString("autofillDefaultCountryLanguageCode",
                                default_country_language_code);
 }
 
 }  // namespace
 
 namespace options {
 
 AutofillOptionsHandler::AutofillOptionsHandler() : personal_data_(NULL) {}

[Mathieu, 2016/04/11 15:01:21]

nit: NULL -> nullptr

everywhere in this file.

[tmartino, 2016/04/12 20:48:27]

Done.

 
 AutofillOptionsHandler::~AutofillOptionsHandler() {
   if (personal_data_)
     personal_data_->RemoveObserver(this);
 }
 
 /////////////////////////////////////////////////////////////////////////////
 // OptionsPageUIHandler implementation:
 void AutofillOptionsHandler::GetLocalizedValues(
     base::DictionaryValue* localized_strings) {
@@ skipping 165 matching lines @@
 
 void AutofillOptionsHandler::LoadAddressEditor(const base::ListValue* args) {
   DCHECK(IsPersonalDataLoaded());
 
   std::string guid;
   if (!args->GetString(0, &guid)) {
     NOTREACHED();
     return;
   }
 
-  AutofillProfile* profile = personal_data_->GetProfileByGUID(guid);
-  if (!profile) {
+  prior_profile_ = personal_data_->GetProfileByGUID(guid);
+  if (!prior_profile_) {
     // There is a race where a user can click once on the close button and
     // quickly click again on the list item before the item is removed (since
     // the list is not updated until the model tells the list an item has been
     // removed). This will activate the editor for a profile that has been
     // removed. Do nothing in that case.
     return;
   }
 
   base::DictionaryValue address;
-  AutofillProfileToDictionary(*profile, &address);
+  AutofillProfileToDictionary(*prior_profile_, &address);
 
   web_ui()->CallJavascriptFunction("AutofillOptions.editAddress", address);
 }
 
 void AutofillOptionsHandler::LoadAddressEditorComponents(
     const base::ListValue* args) {
   std::string country_code;
   if (!args->GetString(0, &country_code)) {
     NOTREACHED();
     return;
@@ skipping 56 matching lines @@
   std::string guid;
   if (!args->GetString(arg_counter++, &guid)) {
     NOTREACHED();
     return;
   }
 
   AutofillProfile profile(guid, kSettingsOrigin);
 
   base::string16 value;
   if (args->GetString(arg_counter++, &value)) {
     profile.SetInfo(AutofillType(autofill::NAME_FULL), value,

[Mathieu, 2016/04/11 15:01:21]

The origin of this bug is here. When profile.SetInfo is called, it ends up
calling NameInfo::SetFullName[1], which splits the name very unintelligently.
Note that at this point though, |value| is the proper full name as passed to the
editor and back. I believe to make it a little less confusing, we should pass
|value| as full name instead of |profile| to ShouldTransferNameComponents, and
move this logic here.

Also, if the name was unedited, I'm not sure we should call
profile.SetInfo(NAME_FULL) since as we see it has bad behavior. Perhaps
profile.SetRawInfo(NAME_FULL, value) since we know it hasn't changed from last
time?
 

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[tmartino, 2016/04/12 20:48:27]

With the new flow I don't think it should be necessary to check if it's changed.
SetInfo is only called if ProfileMatchesFullName is false, and that either means
we have weird, crufty old data, or the user actually did change something.

WDYT?

                     g_browser_process->GetApplicationLocale());
   }
 
   if (args->GetString(arg_counter++, &value))
     profile.SetRawInfo(autofill::COMPANY_NAME, value);
 
   if (args->GetString(arg_counter++, &value))
     profile.SetRawInfo(autofill::ADDRESS_HOME_STREET_ADDRESS, value);
 
   if (args->GetString(arg_counter++, &value))
@@ skipping 16 matching lines @@
 
   if (args->GetString(arg_counter++, &value))
     profile.SetRawInfo(autofill::PHONE_HOME_WHOLE_NUMBER, value);
 
   if (args->GetString(arg_counter++, &value))
     profile.SetRawInfo(autofill::EMAIL_ADDRESS, value);
 
   if (args->GetString(arg_counter++, &value))
     profile.set_language_code(base::UTF16ToUTF8(value));
 
+  // Although First/Middle/Last are not displayed on the form, we transfer this
+  // information when they match the full name given. This is because it may not
+  // be possible later to correctly tokenize the concatenated full name--e.g.,
+  // when the last name contains a space, the first word would be treated as a
+  // middle name.
+  if (prior_profile_ &&
+      ShouldTransferNameComponents(profile, *prior_profile_)) {
+    profile.SetRawInfo(autofill::NAME_FIRST,
+                       prior_profile_->GetRawInfo(autofill::NAME_FIRST));
+    profile.SetRawInfo(autofill::NAME_MIDDLE,
+                       prior_profile_->GetRawInfo(autofill::NAME_MIDDLE));
+    profile.SetRawInfo(autofill::NAME_LAST,
+                       prior_profile_->GetRawInfo(autofill::NAME_LAST));
+    profile.SetRawInfo(
+        autofill::NAME_MIDDLE_INITIAL,

[Mathieu, 2016/04/11 15:01:21]

curious: I'm not familiar with middle initial. In your test, is it always filled
alongside NAME_MIDDLE (i.e. the first letter is taken)?

[tmartino, 2016/04/12 20:48:27]

It appears that it's not stored anywhere:

https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

I removed this setter call, but kept the references in ProfileMatchesFullName.

+        prior_profile_->GetRawInfo(autofill::NAME_MIDDLE_INITIAL));
+  }
+
   if (!base::IsValidGUID(profile.guid())) {
     profile.set_guid(base::GenerateGUID());
     personal_data_->AddProfile(profile);
   } else {
     personal_data_->UpdateProfile(profile);
   }
 }
 
 void AutofillOptionsHandler::SetCreditCard(const base::ListValue* args) {
   if (!IsPersonalDataLoaded())
@@ skipping 75 matching lines @@
 
   scoped_ptr<base::ListValue> components(new base::ListValue);
   GetAddressComponents(
       base::UTF16ToUTF8(profile.GetRawInfo(autofill::ADDRESS_HOME_COUNTRY)),
       profile.language_code(),
       components.get(),
       NULL);
   address->Set(kComponents, components.release());
 }
 
+// Concatenates two strings, adding a space and, optionally, a period.
+base::string16 StringConcatenationHelper(base::string16 first,
+                                         base::string16 second,
+                                         bool period) {
+  base::string16 delimiter =
+      period ? base::UTF8ToUTF16(". ") : base::UTF8ToUTF16(" ");
+  return first + delimiter + second;
+}
+
+// static
+bool AutofillOptionsHandler::ShouldTransferNameComponents(

[Mathieu, 2016/04/11 15:01:21]

How about renaming this to ProfileExpressesFullName[1] and integrating it into
autofill_data_util as a static function? 

[1] Note: you may pick a better name :)

[tmartino, 2016/04/12 20:48:27]

Done.

+    const autofill::AutofillProfile& profile,
+    const autofill::AutofillProfile& prior_profile) {
+  base::string16 full_name =
+      profile.GetInfo(AutofillType(autofill::NAME_FULL),
+                      g_browser_process->GetApplicationLocale());
+  base::string16 first_last = StringConcatenationHelper(

[Mathieu, 2016/04/11 15:01:21]

should probably return early if the comparison is true to avoid extra work.

if (!full_name.compare(first_last))
  return true;

etc.

[tmartino, 2016/04/12 20:48:27]

Done.

+      prior_profile.GetRawInfo(autofill::NAME_FIRST),
+      prior_profile.GetRawInfo(autofill::NAME_LAST), false);
+  base::string16 first_middle_last = StringConcatenationHelper(
+      StringConcatenationHelper(prior_profile.GetRawInfo(autofill::NAME_FIRST),
+                                prior_profile.GetRawInfo(autofill::NAME_MIDDLE),
+                                false),
+      prior_profile.GetRawInfo(autofill::NAME_LAST), false);
+  base::string16 first_m_last = StringConcatenationHelper(
+      StringConcatenationHelper(
+          prior_profile.GetRawInfo(autofill::NAME_FIRST),
+          prior_profile.GetRawInfo(autofill::NAME_MIDDLE_INITIAL), false),
+      prior_profile.GetRawInfo(autofill::NAME_LAST), false);
+  base::string16 first_m_period_last = StringConcatenationHelper(
+      StringConcatenationHelper(
+          prior_profile.GetRawInfo(autofill::NAME_FIRST),
+          prior_profile.GetRawInfo(autofill::NAME_MIDDLE_INITIAL), false),
+      prior_profile.GetRawInfo(autofill::NAME_LAST), true);
+  return !(full_name.compare(first_last) &&
+           full_name.compare(first_middle_last) &&
+           full_name.compare(first_m_last) &&
+           full_name.compare(first_m_period_last));
+}
+
 }  // namespace options