Autofill billing and shipping addresses separately.

When autofilling a form that asks for both a shipping and a billing address, we should try to fill them separately, rather than filling both with the same data.

BUG=61678
TEST=unit_tests --gtest_filter=AutoFillManagerTest.FillFormWithMultipleSections

Review URL: http://codereview.chromium.org/6334081

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@73700 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 252 insertions, 87 deletions

diff --git a/chrome/browser/autofill/autofill_manager.cc b/chrome/browser/autofill/autofill_manager.cc
index adad24b..3515706 100644
--- a/chrome/browser/autofill/autofill_manager.cc
+++ b/chrome/browser/autofill/autofill_manager.cc
@@ -82,13 +82,93 @@ void RemoveDuplicateSuggestions(std::vector<string16>* values,
   unique_ids->swap(unique_ids_copy);
 }
 
+// Precondition: |form| should be the cached version of the form that is to be
+// autofilled, and |field| should be the field in the |form| that corresponds to
+// the initiating field. |is_filling_credit_card| should be true if filling
+// credit card data, false otherwise.
+// Fills |section_start| and |section_end| so that [section_start, section_end)
+// gives the bounds of logical section within |form| that includes |field|.
+// Logical sections are identified by two heuristics:
+//  1. The fields in the section must all be profile or credit card fields,
+//     depending on whether |is_filling_credit_card| is true.
+//  2. A logical section should not include multiple fields of the same autofill
+//     type (except for phone/fax numbers, as described below).
+void FindSectionBounds(const FormStructure& form,
+                       const AutoFillField& field,
+                       bool is_filling_credit_card,
+                       size_t* section_start,
+                       size_t* section_end) {
+  DCHECK(section_start);
+  DCHECK(section_end);
+
+  // By default, the relevant section is the entire form.
+  *section_start = 0;
+  *section_end = form.field_count();
+
+  std::set<AutoFillFieldType> seen_types;
+  bool initiating_field_is_in_current_section = false;
+  for (size_t i = 0; i < form.field_count(); ++i) {
+    const AutoFillField* current_field = form.field(i);
+    const AutoFillFieldType current_type =
+        AutoFillType::GetEquivalentFieldType(current_field->type());
+
+    // Fields of unknown type don't help us to distinguish sections.
+    if (current_type == UNKNOWN_TYPE)
+      continue;
+
+    bool already_saw_current_type = seen_types.count(current_type) > 0;
+    // Forms often ask for multiple phone numbers -- e.g. both a daytime and
+    // evening phone number.  Our phone and fax number detection is also
+    // generally a little off.  Hence, ignore both field types as a signal here.
+    AutoFillType::FieldTypeGroup current_type_group =
+        AutoFillType(current_type).group();
+    if (current_type_group == AutoFillType::PHONE_HOME ||
+        current_type_group == AutoFillType::PHONE_FAX)
+      already_saw_current_type = false;
+
+    // If we are filling credit card data, the relevant section should include
+    // only credit card fields; and similarly for profile data.
+    bool is_credit_card_field = current_type_group == AutoFillType::CREDIT_CARD;
+    bool is_appropriate_type = is_credit_card_field == is_filling_credit_card;
+
+    if (already_saw_current_type || !is_appropriate_type) {
+      if (initiating_field_is_in_current_section) {
+        // We reached the end of the section containing the initiating field.
+        *section_end = i;
+        break;
+      }
+
+      // We reached the end of a section, so start a new section.
+      seen_types.clear();
+
+      // Only include the current field in the new section if it matches the
+      // type of data we are filling.
+      if (is_appropriate_type) {
+        *section_start = i;
+      } else {
+        *section_start = i + 1;
+        continue;
+      }
+    }
+
+    seen_types.insert(current_type);
+
+    if (current_field == &field)
+      initiating_field_is_in_current_section = true;
+  }
+
+  // We should have found the initiating field.
+  DCHECK(initiating_field_is_in_current_section);
+}
+
 // Precondition: |form_structure| and |form| should correspond to the same
 // logical form. Returns true if the relevant portion of |form| is auto-filled.
-// If |is_filling_credit_card|, the relevant portion is the credit card portion;
-// otherwise it is the address and contact info portion.
-bool FormIsAutoFilled(const FormStructure* form_structure,
-                      const webkit_glue::FormData& form,
-                      bool is_filling_credit_card) {
+// The "relevant" fields in |form| are ones corresponding to fields in
+// |form_structure| with indices in the range [section_start, section_end).
+bool SectionIsAutoFilled(const FormStructure* form_structure,
+                         const webkit_glue::FormData& form,
+                         size_t section_start,
+                         size_t section_end) {
   // TODO(isherman): It would be nice to share most of this code with the loop
   // in |FillAutoFillFormData()|, but I don't see a particularly clean way to do
   // that.
@@ -97,8 +177,8 @@ bool FormIsAutoFilled(const FormStructure* form_structure,
   // directly and we can fill these corresponding fields; however, when the
   // |form_structure| and |form.fields| do not match directly we search
   // ahead in the |form_structure| for the matching field.
-  for (size_t i = 0, j = 0;
-       i < form_structure->field_count() && j < form.fields.size();
+  for (size_t i = section_start, j = 0;
+       i < section_end && j < form.fields.size();
        j++) {
     size_t k = i;
 
@@ -113,10 +193,7 @@ bool FormIsAutoFilled(const FormStructure* form_structure,
       continue;
 
     AutoFillType autofill_type(form_structure->field(k)->type());
-    bool is_credit_card_field =
-        autofill_type.group() == AutoFillType::CREDIT_CARD;
-    if (is_filling_credit_card == is_credit_card_field &&
-        form.fields[j].is_autofilled())
+    if (form.fields[j].is_autofilled())
       return true;
 
     // We found a matching field in the |form_structure| so we
@@ -283,11 +360,15 @@ void AutoFillManager::OnQueryFormFieldAutoFill(
         icons.assign(1, string16());
         unique_ids.assign(1, -1);
       } else {
-        // If the form is auto-filled and the renderer is querying for
-        // suggestions, then the user is editing the value of a field. In this
-        // case, mimic autocomplete: don't display labels or icons, as that
-        // information is redundant.
-        if (FormIsAutoFilled(form_structure, form, is_filling_credit_card)) {
+        size_t section_start, section_end;
+        FindSectionBounds(*form_structure, *autofill_field,
+                          is_filling_credit_card, &section_start, &section_end);
+        if (SectionIsAutoFilled(form_structure, form, section_start,
+                                section_end)) {
+          // If the relevant section is auto-filled and the renderer is querying
+          // for suggestions, then the user is editing the value of a field.
+          // In this case, mimic autocomplete: don't display labels or icons,
+          // as that information is redundant.
           labels.assign(labels.size(), string16());
           icons.assign(icons.size(), string16());
         }
@@ -356,28 +437,33 @@ void AutoFillManager::OnFillAutoFillFormData(int query_id,
   if (!profile && !credit_card)
     return;
 
+  // Find the section of the form that we are autofilling.
+  size_t section_start, section_end;
+  FindSectionBounds(*form_structure, *autofill_field, (credit_card != NULL),
+                    &section_start, &section_end);
+
   FormData result = form;
 
-  // If the form is auto-filled, we should fill |field| but not the rest of the
-  // form.
-  if (FormIsAutoFilled(form_structure, form, (credit_card != NULL))) {
+  // If the relevant section is auto-filled, we should fill |field| but not the
+  // rest of the form.
+  if (SectionIsAutoFilled(form_structure, form, section_start, section_end)) {
     for (std::vector<FormField>::iterator iter = result.fields.begin();
          iter != result.fields.end(); ++iter) {
       if ((*iter) == field) {
         AutoFillType autofill_type(autofill_field->type());
-        if (credit_card &&
-            autofill_type.group() == AutoFillType::CREDIT_CARD) {
-          FillCreditCardFormField(credit_card, autofill_type, &(*iter));
-        } else if (profile &&
-                   autofill_type.group() != AutoFillType::CREDIT_CARD) {
+        if (profile) {
+          DCHECK(autofill_type.group() != AutoFillType::CREDIT_CARD);
           FillFormField(profile, autofill_type, &(*iter));
+        } else {
+          DCHECK(autofill_type.group() == AutoFillType::CREDIT_CARD);
+          FillCreditCardFormField(credit_card, autofill_type, &(*iter));
         }
         break;
       }
     }
 
-    host->Send(new AutoFillMsg_FormDataFilled(
-        host->routing_id(), query_id, result));
+    host->Send(new AutoFillMsg_FormDataFilled(host->routing_id(), query_id,
+                                              result));
     return;
   }
 
@@ -387,30 +473,30 @@ void AutoFillManager::OnFillAutoFillFormData(int query_id,
   // ahead in the |form_structure| for the matching field.
   // See unit tests: AutoFillManagerTest.FormChangesRemoveField and
   // AutoFillManagerTest.FormChangesAddField for usage.
-  for (size_t i = 0, j = 0;
-       i < form_structure->field_count() && j < result.fields.size();
+  for (size_t i = section_start, j = 0;
+       i < section_end && j < result.fields.size();
        j++) {
     size_t k = i;
 
     // Search forward in the |form_structure| for a corresponding field.
-    while (k < form_structure->field_count() &&
-           *form_structure->field(k) != result.fields[j]) {
+    while (k < section_end && *form_structure->field(k) != result.fields[j]) {
       k++;
     }
 
     // If we've found a match then fill the |result| field with the found
     // field in the |form_structure|.
-    if (k >= form_structure->field_count())
+    if (k >= section_end)
       continue;
 
-    const AutoFillField* field = form_structure->field(k);
-    AutoFillType autofill_type(field->type());
-    if (credit_card &&
-        autofill_type.group() == AutoFillType::CREDIT_CARD) {
-      FillCreditCardFormField(credit_card, autofill_type, &result.fields[j]);
-    } else if (profile &&
-               autofill_type.group() != AutoFillType::CREDIT_CARD) {
-      FillFormField(profile, autofill_type, &result.fields[j]);
+    AutoFillType autofill_type(form_structure->field(k)->type());
+    if (autofill_type.group() != AutoFillType::NO_GROUP) {
+      if (profile) {
+        DCHECK(autofill_type.group() != AutoFillType::CREDIT_CARD);
+        FillFormField(profile, autofill_type, &result.fields[j]);
+      } else {
+        DCHECK(autofill_type.group() == AutoFillType::CREDIT_CARD);
+        FillCreditCardFormField(credit_card, autofill_type, &result.fields[j]);
+      }
     }
 
     // We found a matching field in the |form_structure| so we
diff --git a/chrome/browser/autofill/autofill_manager.h b/chrome/browser/autofill/autofill_manager.h
index 569ffbc..2757a057 100644
--- a/chrome/browser/autofill/autofill_manager.h
+++ b/chrome/browser/autofill/autofill_manager.h
@@ -231,6 +231,7 @@ class AutoFillManager : public TabContentsObserver,
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillCreditCardFormYearMonth);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillAddressForm);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillAddressAndCreditCardForm);
+  FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillFormWithMultipleSections);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillAutoFilledForm);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillPhoneNumber);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FormChangesRemoveField);
diff --git a/chrome/browser/autofill/autofill_manager_unittest.cc b/chrome/browser/autofill/autofill_manager_unittest.cc
index f58eaf8..b3a1ad1 100644
--- a/chrome/browser/autofill/autofill_manager_unittest.cc
+++ b/chrome/browser/autofill/autofill_manager_unittest.cc
@@ -323,25 +323,25 @@ void ExpectFilledForm(int page_id,
         "Middle Name", "middlename", middle, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[1]));
     autofill_test::CreateTestFormField(
-         "Last Name", "lastname", last, "text", &field);
+        "Last Name", "lastname", last, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[2]));
     autofill_test::CreateTestFormField(
-         "Address Line 1", "addr1", address1, "text", &field);
+        "Address Line 1", "addr1", address1, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[3]));
     autofill_test::CreateTestFormField(
-         "Address Line 2", "addr2", address2, "text", &field);
+        "Address Line 2", "addr2", address2, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[4]));
     autofill_test::CreateTestFormField(
-         "City", "city", city, "text", &field);
+        "City", "city", city, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[5]));
     autofill_test::CreateTestFormField(
-         "State", "state", state, "text", &field);
+        "State", "state", state, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[6]));
     autofill_test::CreateTestFormField(
-         "Postal Code", "zipcode", postal_code, "text", &field);
+        "Postal Code", "zipcode", postal_code, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[7]));
     autofill_test::CreateTestFormField(
-         "Country", "country", country, "text", &field);
+        "Country", "country", country, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[8]));
     autofill_test::CreateTestFormField(
         "Phone Number", "phonenumber", phone, "text", &field);
@@ -350,7 +350,7 @@ void ExpectFilledForm(int page_id,
         "Fax", "fax", fax, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[10]));
     autofill_test::CreateTestFormField(
-         "Email", "email", email, "text", &field);
+        "Email", "email", email, "text", &field);
     EXPECT_TRUE(field.StrictlyEqualsHack(filled_form.fields[11]));
     autofill_test::CreateTestFormField(
          "Email", "email2", email, "email", &field);
@@ -1467,9 +1467,8 @@ TEST_F(AutoFillManagerTest, FillAddressAndCreditCardForm) {
 
   // First fill the address data.
   std::string guid = autofill_manager_->GetLabeledProfile("Home")->guid();
-  FillAutoFillFormData(
-      kDefaultPageID, form, form.fields[0],
-      autofill_manager_->PackGUIDs(std::string(), guid));
+  FillAutoFillFormData(kDefaultPageID, form, form.fields[0],
+                       autofill_manager_->PackGUIDs(std::string(), guid));
 
   int page_id = 0;
   FormData results;
@@ -1494,6 +1493,77 @@ TEST_F(AutoFillManagerTest, FillAddressAndCreditCardForm) {
   }
 }
 
+// Test that we correctly fill a form that has multiple logical sections, e.g.
+// both a billing and a shipping address.
+TEST_F(AutoFillManagerTest, FillFormWithMultipleSections) {
+  // Set up our form data.
+  FormData form;
+  CreateTestAddressFormData(&form);
+  const size_t kAddressFormSize = form.fields.size();
+  CreateTestAddressFormData(&form);
+  for (size_t i = kAddressFormSize; i < form.fields.size(); ++i) {
+    // Make sure the fields have distinct names.
+    form.fields[i].set_name(form.fields[i].name() + ASCIIToUTF16("_"));
+  }
+  std::vector<FormData> forms(1, form);
+  FormsSeen(forms);
+
+  // Fill the first section.
+  std::string guid = autofill_manager_->GetLabeledProfile("Home")->guid();
+  FillAutoFillFormData(kDefaultPageID, form, form.fields[0],
+                       autofill_manager_->PackGUIDs(std::string(), guid));
+
+  int page_id = 0;
+  FormData results;
+  EXPECT_TRUE(GetAutoFillFormDataFilledMessage(&page_id, &results));
+  {
+    SCOPED_TRACE("Address 1");
+
+    // The second address section should be empty.
+    ASSERT_EQ(results.fields.size(), 2*kAddressFormSize);
+    for (size_t i = kAddressFormSize; i < form.fields.size(); ++i) {
+      EXPECT_EQ(string16(), results.fields[i].value());
+    }
+
+    // The first address section should be filled with Elvis's data.
+    results.fields.resize(kAddressFormSize);
+    ExpectFilledAddressFormElvis(page_id, results, kDefaultPageID, false);
+  }
+
+  // Fill the second section, with the initiating field somewhere in the middle
+  // of the section.
+  const int kPageID2 = 2;
+  guid = autofill_manager_->GetLabeledProfile("Home")->guid();
+  ASSERT_LT(9U, kAddressFormSize);
+  FillAutoFillFormData(kPageID2, form, form.fields[kAddressFormSize + 9],
+                       autofill_manager_->PackGUIDs(std::string(), guid));
+
+  page_id = 0;
+  EXPECT_TRUE(GetAutoFillFormDataFilledMessage(&page_id, &results));
+  {
+    SCOPED_TRACE("Address 2");
+    ASSERT_EQ(results.fields.size(), form.fields.size());
+
+    // The first address section should be empty.
+    ASSERT_EQ(results.fields.size(), 2*kAddressFormSize);
+    for (size_t i = 0; i < kAddressFormSize; ++i) {
+      EXPECT_EQ(string16(), results.fields[i].value());
+    }
+
+    // The second address section should be filled with Elvis's data.
+    FormData secondSection = results;
+    secondSection.fields.erase(secondSection.fields.begin(),
+                               secondSection.fields.begin() + kAddressFormSize);
+    for (size_t i = 0; i < kAddressFormSize; ++i) {
+      // Restore the expected field names.
+      string16 name = secondSection.fields[i].name();
+      string16 original_name = name.substr(0, name.size() - 1);
+      secondSection.fields[i].set_name(original_name);
+    }
+    ExpectFilledAddressFormElvis(page_id, secondSection, kPageID2, false);
+  }
+}
+
 // Test that we correctly fill a previously auto-filled form.
 TEST_F(AutoFillManagerTest, FillAutoFilledForm) {
   // Set up our form data.
diff --git a/chrome/browser/autofill/autofill_profile.cc b/chrome/browser/autofill/autofill_profile.cc
index 1ec7b30..f6bcb13 100644
--- a/chrome/browser/autofill/autofill_profile.cc
+++ b/chrome/browser/autofill/autofill_profile.cc
@@ -11,6 +11,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/address.h"
 #include "chrome/browser/autofill/autofill_manager.h"
+#include "chrome/browser/autofill/autofill_type.h"
 #include "chrome/browser/autofill/contact_info.h"
 #include "chrome/browser/autofill/fax_number.h"
 #include "chrome/browser/autofill/home_address.h"
@@ -28,48 +29,15 @@ void InitPersonalInfo(FormGroupMap* personal_info) {
   (*personal_info)[AutoFillType::ADDRESS_HOME] = new HomeAddress();
 }
 
-// Maps |field_type| to a field type that can be directly stored in a profile
-// (in the sense that it makes sense to call |AutoFillProfile::SetInfo()| with
-// the returned field type as the first parameter.
-AutoFillFieldType GetEquivalentFieldType(AutoFillFieldType field_type) {
-  // When billing information is requested from the profile we map to the
-  // home address equivalents.
-  switch (field_type) {
-    case ADDRESS_BILLING_LINE1:
-      return ADDRESS_HOME_LINE1;
-
-    case ADDRESS_BILLING_LINE2:
-      return ADDRESS_HOME_LINE2;
-
-    case ADDRESS_BILLING_APT_NUM:
-      return ADDRESS_HOME_APT_NUM;
-
-    case ADDRESS_BILLING_CITY:
-      return ADDRESS_HOME_CITY;
-
-    case ADDRESS_BILLING_STATE:
-      return ADDRESS_HOME_STATE;
-
-    case ADDRESS_BILLING_ZIP:
-      return ADDRESS_HOME_ZIP;
-
-    case ADDRESS_BILLING_COUNTRY:
-      return ADDRESS_HOME_COUNTRY;
-
-    default:
-      return field_type;
-  }
-}
-
-// Like |GetEquivalentFieldType()| above, but also returns |NAME_FULL| for
-// first, middle, and last name field types.
+// Like |AutoFillType::GetEquivalentFieldType()|, but also returns |NAME_FULL|
+// for first, middle, and last name field types.
 AutoFillFieldType GetEquivalentFieldTypeCollapsingNames(
     AutoFillFieldType field_type) {
   if (field_type == NAME_FIRST || field_type == NAME_MIDDLE ||
       field_type == NAME_LAST)
     return NAME_FULL;
 
-  return GetEquivalentFieldType(field_type);
+  return AutoFillType::GetEquivalentFieldType(field_type);
 }
 
 // Fills |distinguishing_fields| with a list of fields to use when creating
@@ -182,7 +150,9 @@ void AutoFillProfile::GetAvailableFieldTypes(
 }
 
 string16 AutoFillProfile::GetFieldText(const AutoFillType& type) const {
-  AutoFillType return_type(GetEquivalentFieldType(type.field_type()));
+  AutoFillType return_type(
+      AutoFillType::GetEquivalentFieldType(type.field_type()));
+
   FormGroupMap::const_iterator iter = personal_info_.find(return_type.group());
   if (iter == personal_info_.end() || iter->second == NULL)
     return string16();
diff --git a/chrome/browser/autofill/autofill_type.cc b/chrome/browser/autofill/autofill_type.cc
index 2fd8cf2..972cd57d 100644
--- a/chrome/browser/autofill/autofill_type.cc
+++ b/chrome/browser/autofill/autofill_type.cc
@@ -167,3 +167,36 @@ FieldTypeGroup AutoFillType::group() const {
 FieldTypeSubGroup AutoFillType::subgroup() const {
   return kAutoFillTypeDefinitions[field_type_].subgroup;
 }
+
+// static
+AutoFillFieldType AutoFillType::GetEquivalentFieldType(
+    AutoFillFieldType field_type) {
+  // When billing information is requested from the profile we map to the
+  // home address equivalents.
+  switch (field_type) {
+    case ADDRESS_BILLING_LINE1:
+      return ADDRESS_HOME_LINE1;
+
+    case ADDRESS_BILLING_LINE2:
+      return ADDRESS_HOME_LINE2;
+
+    case ADDRESS_BILLING_APT_NUM:
+      return ADDRESS_HOME_APT_NUM;
+
+    case ADDRESS_BILLING_CITY:
+      return ADDRESS_HOME_CITY;
+
+    case ADDRESS_BILLING_STATE:
+      return ADDRESS_HOME_STATE;
+
+    case ADDRESS_BILLING_ZIP:
+      return ADDRESS_HOME_ZIP;
+
+    case ADDRESS_BILLING_COUNTRY:
+      return ADDRESS_HOME_COUNTRY;
+
+    default:
+      return field_type;
+  }
+}
+
diff --git a/chrome/browser/autofill/autofill_type.h b/chrome/browser/autofill/autofill_type.h
index 2091153..5aac134 100644
--- a/chrome/browser/autofill/autofill_type.h
+++ b/chrome/browser/autofill/autofill_type.h
@@ -58,6 +58,11 @@ class AutoFillType {
   FieldTypeGroup group() const;
   FieldTypeSubGroup subgroup() const;
 
+  // Maps |field_type| to a field type that can be directly stored in a profile
+  // (in the sense that it makes sense to call |AutoFillProfile::SetInfo()| with
+  // the returned field type as the first parameter).
+  static AutoFillFieldType GetEquivalentFieldType(AutoFillFieldType field_type);
+
  private:
   AutoFillFieldType field_type_;
 };