Fill in more name fields with requestAutocomplete

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

Index: components/autofill/core/browser/webdata/autofill_table.cc
diff --git a/components/autofill/core/browser/webdata/autofill_table.cc b/components/autofill/core/browser/webdata/autofill_table.cc
index e24ceca1d026d3d75eec6904ebaf145ba0bf1ab4..dc01c5e683501ec909842064421ed8c35d4f196b 100644
--- a/components/autofill/core/browser/webdata/autofill_table.cc
+++ b/components/autofill/core/browser/webdata/autofill_table.cc
@@ -167,10 +167,41 @@ scoped_ptr<CreditCard> CreditCardFromStatement(const sql::Statement& s) {
   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());
@@ -181,11 +212,13 @@ bool AddAutofillProfileNamesToProfile(sql::Connection* db,
   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;
@@ -193,6 +226,7 @@ bool AddAutofillProfileNamesToProfile(sql::Connection* db,
   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;
 }
 
@@ -250,20 +284,38 @@ bool AddAutofillProfileNames(const AutofillProfile& profile,
   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;
   }
@@ -447,6 +499,9 @@ bool AutofillTable::MigrateToVersion(int version,
     case 56:
       *update_compatible_version = true;
       return MigrateToVersion56AddProfileLanguageCodeForFormatting();
+    case 57:
+      *update_compatible_version = true;
+      return MigrateToVersion57AddFullNameField();
   }
   return true;
 }
@@ -1292,7 +1347,8 @@ bool AutofillTable::InitProfileNamesTable() {
                       "guid VARCHAR, "
                       "first_name VARCHAR, "
                       "middle_name VARCHAR, "
-                      "last_name VARCHAR)")) {
+                      "last_name VARCHAR, "
+                      "full_name VARCHAR)")) {
       NOTREACHED();
       return false;
     }
@@ -2005,7 +2061,7 @@ bool AutofillTable::MigrateToVersion37MergeAndCullOlderProfiles() {
     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());
@@ -2272,4 +2328,9 @@ bool AutofillTable::MigrateToVersion56AddProfileLanguageCodeForFormatting() {
                       "ADD COLUMN language_code VARCHAR");
 }
 
+bool AutofillTable::MigrateToVersion57AddFullNameField() {
+  return db_->Execute("ALTER TABLE autofill_profile_names "
+                      "ADD COLUMN full_name VARCHAR");
+}
+
 }  // namespace autofill