[Autofill] Disambiguate similar types before uploading.

components/autofill/core/browser/autofill_manager.cc

Index: components/autofill/core/browser/autofill_manager.cc
diff --git a/components/autofill/core/browser/autofill_manager.cc b/components/autofill/core/browser/autofill_manager.cc
index 0f0c699ebb83ce23032bb6134f47db85d52608ea..57dbb401c7333f8b1de85cf16c6bf0e7bdae9244 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -135,6 +135,45 @@ base::string16 RemoveMiddleInitial(const base::string16& name) {
   return name;
 }
 
+// Returns whether the |field| is predicted as being any kind of name.
+bool IsNameType(const AutofillField& field) {
+  return field.Type().group() == NAME || field.Type().group() == NAME_BILLING ||
+         field.Type().GetStorableType() == CREDIT_CARD_NAME_FULL ||
+         field.Type().GetStorableType() == CREDIT_CARD_NAME_FIRST ||
+         field.Type().GetStorableType() == CREDIT_CARD_NAME_LAST;
+}
+
+// Selects the right name type from the |old_types| to insert into the
+// |new_types| based on |is_credit_card|.
+void SelectRightNameType(const ServerFieldTypeSet& old_types,
+                         ServerFieldTypeSet* new_types,
+                         bool is_credit_card) {
+  ServerFieldTypeSet upload_types;
+  if (old_types.count(NAME_FIRST) && old_types.count(CREDIT_CARD_NAME_FIRST)) {
+    if (is_credit_card) {
+      new_types->insert(CREDIT_CARD_NAME_FIRST);
+    } else {
+      new_types->insert(NAME_FIRST);
+    }
+  } else if (old_types.count(NAME_LAST) &&
+             old_types.count(CREDIT_CARD_NAME_LAST)) {
+    if (is_credit_card) {
+      new_types->insert(CREDIT_CARD_NAME_LAST);
+    } else {
+      new_types->insert(NAME_LAST);
+    }
+  } else if (old_types.count(NAME_FULL) &&
+             old_types.count(CREDIT_CARD_NAME_FULL)) {
+    if (is_credit_card) {
+      new_types->insert(CREDIT_CARD_NAME_FULL);
+    } else {
+      new_types->insert(NAME_FULL);
+    }
+  } else {
+    *new_types = old_types;
+  }
+}
+
 }  // namespace
 
 AutofillManager::AutofillManager(
@@ -1793,6 +1832,140 @@ void AutofillManager::DeterminePossibleFieldTypesForUpload(
 
     field->set_possible_types(matching_types);
   }
+
+  AutofillManager::DisambiguateUploadTypes(submitted_form);
+}
+
+// static
+void AutofillManager::DisambiguateUploadTypes(FormStructure* form) {
+  for (size_t i = 0; i < form->field_count(); ++i) {
+    AutofillField* field = form->field(i);
+    const ServerFieldTypeSet& upload_types = field->possible_types();
+
+    if (upload_types.size() == 2) {
+      if (upload_types.count(ADDRESS_HOME_LINE1) &&
+          upload_types.count(ADDRESS_HOME_STREET_ADDRESS)) {
+        AutofillManager::DisambiguateAddressUploadTypes(form, i);
+      } else if (upload_types.count(PHONE_HOME_CITY_AND_NUMBER) &&
+                 upload_types.count(PHONE_HOME_WHOLE_NUMBER)) {
+        AutofillManager::DisambiguatePhoneUploadTypes(form, i);
+      } else if ((upload_types.count(NAME_FULL) &&
+                  upload_types.count(CREDIT_CARD_NAME_FULL)) ||
+                 (upload_types.count(NAME_FIRST) &&
+                  upload_types.count(CREDIT_CARD_NAME_FIRST)) ||
+                 (upload_types.count(NAME_LAST) &&
+                  upload_types.count(CREDIT_CARD_NAME_LAST))) {
+        AutofillManager::DisambiguateNameUploadTypes(form, i, upload_types);
+      }
+    }
+  }
+}
+
+// static
+void AutofillManager::DisambiguateAddressUploadTypes(FormStructure* form,
+                                                     size_t current_index) {
+  // This case happens when the profile has only one address line.
+  // Therefore the address line one and the street address (the whole
+  // address) have the same value and match.
+
+  // If the field is followed by a field that is predicted to be an
+  // address line two and is empty, we can safely assume that this field
+  // is an address line one field. Otherwise it's a whole address field.
+  ServerFieldTypeSet matching_types;
+  size_t next_index = current_index + 1;
+  if (next_index < form->field_count() &&
+      form->field(next_index)->Type().GetStorableType() == ADDRESS_HOME_LINE2 &&
+      form->field(next_index)->possible_types().count(EMPTY_TYPE)) {
+    matching_types.insert(ADDRESS_HOME_LINE1);
+  } else {
+    matching_types.insert(ADDRESS_HOME_STREET_ADDRESS);
+  }
+
+  AutofillField* field = form->field(current_index);
+  field->set_possible_types(matching_types);
+}
+
+// static
+void AutofillManager::DisambiguatePhoneUploadTypes(FormStructure* form,
+                                                   size_t current_index) {
+  // This case happens for profiles that have no country code saved.
+  // Therefore, both the whole number and the city code and number have
+  // the same value and match.
+
+  // Since the form was submitted, it is safe to assume that the form
+  // didn't require a country code. Thus, only PHONE_HOME_CITY_AND_NUMBER
+  // needs to be uploaded.
+  ServerFieldTypeSet matching_types;
+  matching_types.insert(PHONE_HOME_CITY_AND_NUMBER);
+
+  AutofillField* field = form->field(current_index);
+  field->set_possible_types(matching_types);
+}
+
+// static
+void AutofillManager::DisambiguateNameUploadTypes(
+    FormStructure* form,
+    size_t current_index,
+    const ServerFieldTypeSet& upload_types) {
+  // This case happens when both a profile and a credit card have the same
+  // name.
+
+  // If the ambiguous field has either a previous or next field that is
+  // not name related, use that information to determine whether the field
+  // is a name or a credit card name.
+  // If the ambiguous field has both a previous or next field that is not
+  // name related, if they are both from the same group, use that group to
+  // decide this field's type. Otherwise, there is no safe way to
+  // disambiguate.
+
+  // Look for a previous non name related field.
+  bool has_found_previous_type = false;
+  bool is_previous_credit_card = false;
+  bool continue_search = true;

[vabr (Chromium), 2016/03/22 17:08:10]

This variable looks unused (only assigned, never read).

[sebsg, 2016/03/22 18:30:26]

Done.

+  size_t index = current_index;
+  while (index != 0 && !has_found_previous_type) {
+    --index;
+    AutofillField* prev_field = form->field(index);
+    if (!IsNameType(*prev_field)) {
+      has_found_previous_type = true;
+      is_previous_credit_card = prev_field->Type().group() == CREDIT_CARD;
+    }
+  }
+
+  // Look for a next non name related field.
+  bool has_found_next_type = false;
+  bool is_next_credit_card = false;
+  continue_search = true;
+  index = current_index;
+  while (++index < form->field_count() && !has_found_next_type) {
+    AutofillField* next_field = form->field(index);
+    if (!IsNameType(*next_field)) {
+      has_found_next_type = true;
+      is_next_credit_card = next_field->Type().group() == CREDIT_CARD;
+    }
+  }
+
+  if (has_found_previous_type || has_found_next_type) {
+    // If the previous type and next type are not from the same name group
+    // there is no sure way to disambiguate.
+    if (has_found_previous_type && has_found_next_type &&
+        (is_previous_credit_card != is_next_credit_card)) {
+      return;
+    }
+
+    // Otherwise, use the previous or next field group to decide whether
+    // the field is a name or a credit card name.
+    ServerFieldTypeSet matching_types;
+    if (has_found_previous_type) {
+      SelectRightNameType(upload_types, &matching_types,

[vabr (Chromium), 2016/03/22 17:08:10]

Because we already know that is_previous_credit_card == is_next_credit_card,
lines 1960-1 and 1963 are equivalent, aren't they? If so, then we can omit the
if-branching and just make it a single line.

[sebsg, 2016/03/22 18:30:26]

Not necessarily. The previous check (line 1949) only returns if both a previous
and next type are found and are not from the same type group. You can still
reach line 1955 with is_previous_credit_card != is_next_credit_card.

For example a case where the next field is credit card related but there is no
previous field. is_next_credit_card would be true and is_previous_credit_card
would be false (but have no meaning since has_found_previous_type is false).
Therefore that "if" (line 1957) is necessary to send the is_next_credit_card for
which the value is meaningful in this case and not is_previous_credit_card. 

I updated the comment to be clearer thanks!

[vabr (Chromium), 2016/03/23 08:41:22]

Thanks for the explanation, you are right.

+                          is_previous_credit_card);
+    } else {
+      SelectRightNameType(upload_types, &matching_types, is_next_credit_card);
+    }
+
+    AutofillField* field = form->field(current_index);
+    field->set_possible_types(matching_types);
+  }
 }
 
 #ifdef ENABLE_FORM_DEBUG_DUMP