Fill in more name fields with requestAutocomplete

components/autofill/core/browser/webdata/autofill_table.cc

 // Copyright 2013 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 "components/autofill/core/browser/webdata/autofill_table.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <map>
@@ skipping 149 matching lines @@
     index++;
   }
   credit_card->SetRawInfo(CREDIT_CARD_NUMBER, credit_card_number);
   // Intentionally skip column 5, which stores the modification date.
   index++;
   credit_card->set_origin(s.ColumnString(index++));
 
   return credit_card.Pass();
 }
 
+// Obsolete version of AddAutofillProfileNamesToProfile, but still needed
+// for MigrateToVersion37MergeAndCullOlderProfiles().
+bool AddAutofillProfileNamesToProfileForVersion37(sql::Connection* db,
+                                                  AutofillProfile* profile) {
+  sql::Statement s(db->GetUniqueStatement(
+      "SELECT guid, first_name, middle_name, last_name "
+      "FROM autofill_profile_names "
+      "WHERE guid=?"));
+  s.BindString(0, profile->guid());
+
+  if (!s.is_valid())
+    return false;
+
+  std::vector<base::string16> first_names;
+  std::vector<base::string16> middle_names;
+  std::vector<base::string16> last_names;
+  while (s.Step()) {
+    DCHECK_EQ(profile->guid(), s.ColumnString(0));
+    first_names.push_back(s.ColumnString16(1));
+    middle_names.push_back(s.ColumnString16(2));
+    last_names.push_back(s.ColumnString16(3));
+  }
+  if (!s.Succeeded())
+    return false;
+
+  profile->SetRawMultiInfo(NAME_FIRST, first_names);
+  profile->SetRawMultiInfo(NAME_MIDDLE, middle_names);
+  profile->SetRawMultiInfo(NAME_LAST, last_names);
+  return true;
+}

[Ilya Sherman, 2014/06/12 23:25:25]

Optional nit: I'd prefer that you inline this code, but given the very clear
name and doc string, it's fine if you prefer to keep it as a separate function. 
(My preference is to try not to mix migration code with production code as much
as possible.)

[Scott Hess - ex-Googler, 2014/06/13 17:43:27]

I support making the minimal changes necessary to maintain things.  Inlining the
code is almost certainly safe, but doesn't really gain you anything (AFAICT the
compiler will already inline this if it makes sense on that front).  And that
migration function is already pretty big as things stand...

[Ilya Sherman, 2014/06/13 19:32:59]

Fair enough.  I'm not worried about inlining for performance reasons -- I just
want all of the migration code to be disentangled from the non-migration code. 
I think your suggestion of having a _migration.cc file would be a better way to
achieve this goal, though it might not make sense given that the migration
functions access private state of the class.  Maybe we could have two anonymous
namespaces in this file -- one for the production code, and one specifically for
migration-only helpers...

[Evan Stade, 2014/06/14 01:17:23]

putting "ForVersionN" at the end is kind of like a namespace

+
 bool AddAutofillProfileNamesToProfile(sql::Connection* db,
                                       AutofillProfile* profile) {
   sql::Statement s(db->GetUniqueStatement(
-      "SELECT guid, first_name, middle_name, last_name "
+      "SELECT guid, first_name, middle_name, last_name, full_name "
       "FROM autofill_profile_names "
       "WHERE guid=?"));
   s.BindString(0, profile->guid());
 
   if (!s.is_valid())
     return false;
 
   std::vector<base::string16> first_names;
   std::vector<base::string16> middle_names;
   std::vector<base::string16> last_names;
+  std::vector<base::string16> full_names;
   while (s.Step()) {
     DCHECK_EQ(profile->guid(), s.ColumnString(0));
     first_names.push_back(s.ColumnString16(1));
     middle_names.push_back(s.ColumnString16(2));
     last_names.push_back(s.ColumnString16(3));
+    full_names.push_back(s.ColumnString16(4));
   }
   if (!s.Succeeded())
     return false;
 
   profile->SetRawMultiInfo(NAME_FIRST, first_names);
   profile->SetRawMultiInfo(NAME_MIDDLE, middle_names);
   profile->SetRawMultiInfo(NAME_LAST, last_names);
+  profile->SetRawMultiInfo(NAME_FULL, full_names);
   return true;
 }
 
 bool AddAutofillProfileEmailsToProfile(sql::Connection* db,
                                        AutofillProfile* profile) {
   sql::Statement s(db->GetUniqueStatement(
       "SELECT guid, email "
       "FROM autofill_profile_emails "
       "WHERE guid=?"));
   s.BindString(0, profile->guid());
@@ skipping 37 matching lines @@
 }
 
 bool AddAutofillProfileNames(const AutofillProfile& profile,
                              sql::Connection* db) {
   std::vector<base::string16> first_names;
   profile.GetRawMultiInfo(NAME_FIRST, &first_names);
   std::vector<base::string16> middle_names;
   profile.GetRawMultiInfo(NAME_MIDDLE, &middle_names);
   std::vector<base::string16> last_names;
   profile.GetRawMultiInfo(NAME_LAST, &last_names);
+  std::vector<base::string16> full_names;
+  profile.GetRawMultiInfo(NAME_FULL, &full_names);
   DCHECK_EQ(first_names.size(), middle_names.size());
-  DCHECK_EQ(middle_names.size(), last_names.size());
+  DCHECK_EQ(first_names.size(), last_names.size());
+  DCHECK_EQ(first_names.size(), full_names.size());
 
+  // TODO(estade): remove the conditionally-constructed SQL query.
+  // See http://crbug.com/382562
+  const bool has_full_name =
+      db->DoesColumnExist("autofill_profile_names", "full_name");

[Ilya Sherman, 2014/06/12 23:25:25]

Are we calling this function from the migration code?  If so, please update the
code not to do so.  Migration code shouldn't depend on non-migration code, since
non-migration code changes over time, but migration code needs to be frozen.

[Scott Hess - ex-Googler, 2014/06/13 17:43:27]

AFAICT, doing the same thing here would require copying this function, and
copying the caller, which pushed me on the fence as to whether I think it's
worthwhile.  I'm willing to support either direction at this point, there are
advantages to keeping the migration code constant, and there are advantages to
keeping the production functions simple, but there are also advantages to not
bloating up the code that people have to look at.

[When I dream, I imagine database code having a _migration.cc file to contain
the migration subset, or some other mechanism of segregating it.]

[Ilya Sherman, 2014/06/13 19:32:59]

This is one case where I prefer code duplication.  My reasoning is that the
migration code we write once, test, and then basically never touch again.  The
production code, on the other hand, changes (at least) with each revision to the
DB.  Thus, I have a strong preference for keeping the production code clean and
simple, even if it comes at cost to the DRYness of the combined production and
migration code.

[Scott Hess - ex-Googler, 2014/06/13 19:42:47]

OK, then, since I think I have a general preference for separation of this sort
from the sql/ side, and Ilya appears to have a strong preference for separation
from the autofill side, I think that adds up to a preference for separation.  A
series of nested namespaces and the like would be awesome, and crazy, I'm
completely satisfied with the in-place approach of making inconveniently-long
function names that include their intended user.

[Evan Stade, 2014/06/14 01:17:23]

Done.

   for (size_t i = 0; i < first_names.size(); ++i) {
     // Add the new name.
-    sql::Statement s(db->GetUniqueStatement(
-      "INSERT INTO autofill_profile_names"
-      " (guid, first_name, middle_name, last_name) "
-      "VALUES (?,?,?,?)"));
+    sql::Statement s;
+    if (has_full_name) {
+      s.Assign(db->GetUniqueStatement(
+          "INSERT INTO autofill_profile_names"
+          " (guid, first_name, middle_name, last_name, full_name) "
+          "VALUES (?,?,?,?,?)"));
+    } else {
+      s.Assign(db->GetUniqueStatement(
+          "INSERT INTO autofill_profile_names"
+          " (guid, first_name, middle_name, last_name) "
+          "VALUES (?,?,?,?)"));
+    }
     s.BindString(0, profile.guid());
     s.BindString16(1, first_names[i]);
     s.BindString16(2, middle_names[i]);
     s.BindString16(3, last_names[i]);
 
+    if (has_full_name)
+      s.BindString16(4, full_names[i]);
+
     if (!s.Run())
       return false;
   }
   return true;
 }
 
 bool AddAutofillProfileEmails(const AutofillProfile& profile,
                               sql::Connection* db) {
   std::vector<base::string16> emails;
   profile.GetRawMultiInfo(EMAIL_ADDRESS, &emails);
@@ skipping 163 matching lines @@
       return MigrateToVersion51AddOriginColumn();
     case 54:
       *update_compatible_version = true;
       return MigrateToVersion54AddI18nFieldsAndRemoveDeprecatedFields();
     case 55:
       *update_compatible_version = true;
       return MigrateToVersion55MergeAutofillDatesTable();
     case 56:
       *update_compatible_version = true;
       return MigrateToVersion56AddProfileLanguageCodeForFormatting();
+    case 57:
+      *update_compatible_version = true;
+      return MigrateToVersion57AddFullNameField();
   }
   return true;
 }
 
 bool AutofillTable::AddFormFieldValues(
     const std::vector<FormFieldData>& elements,
     std::vector<AutofillChange>* changes) {
   return AddFormFieldValuesTime(elements, changes, Time::Now());
 }
 
@@ skipping 825 matching lines @@
   }
   return true;
 }
 
 bool AutofillTable::InitProfileNamesTable() {
   if (!db_->DoesTableExist("autofill_profile_names")) {
     if (!db_->Execute("CREATE TABLE autofill_profile_names ( "
                       "guid VARCHAR, "
                       "first_name VARCHAR, "
                       "middle_name VARCHAR, "
-                      "last_name VARCHAR)")) {
+                      "last_name VARCHAR, "
+                      "full_name VARCHAR)")) {
       NOTREACHED();
       return false;
     }
   }
   return true;
 }
 
 bool AutofillTable::InitProfileEmailsTable() {
   if (!db_->DoesTableExist("autofill_profile_emails")) {
     if (!db_->Execute("CREATE TABLE autofill_profile_emails ( "
@@ skipping 692 matching lines @@
     profile->SetRawInfo(ADDRESS_HOME_ZIP, s.ColumnString16(index++));
     // Intentionally skip column 7, which stores the localized country name.
     index++;
     profile->SetRawInfo(ADDRESS_HOME_COUNTRY, s.ColumnString16(index++));
     // Intentionally skip column 9, which stores the profile's modification
     // date.
     index++;
     profile->set_origin(s.ColumnString(index++));
 
     // Get associated name info.
-    AddAutofillProfileNamesToProfile(db_, profile.get());
+    AddAutofillProfileNamesToProfileForVersion37(db_, profile.get());
 
     // Get associated email info.
     AddAutofillProfileEmailsToProfile(db_, profile.get());
 
     // Get associated phone info.
     AddAutofillProfilePhonesToProfile(db_, profile.get());
 
     if (PersonalDataManager::IsValidLearnableProfile(*profile, app_locale_)) {
       std::vector<AutofillProfile> merged_profiles;
       std::string merged_guid = PersonalDataManager::MergeProfile(
@@ skipping 246 matching lines @@
 
 
   return transaction.Commit();
 }
 
 bool AutofillTable::MigrateToVersion56AddProfileLanguageCodeForFormatting() {
   return db_->Execute("ALTER TABLE autofill_profiles "
                       "ADD COLUMN language_code VARCHAR");
 }
 
+bool AutofillTable::MigrateToVersion57AddFullNameField() {
+  return db_->Execute("ALTER TABLE autofill_profile_names "
+                      "ADD COLUMN full_name VARCHAR");
+}
+
 }  // namespace autofill