Improve autofill form matching.

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 1e7a2cacfe19b19dcc46d3220b4294f6bb6eb162..92cd95fa3cfbc067a8aa54e6f20b8c449a6fc1e2 100644
--- a/components/autofill/core/browser/autofill_manager.cc
+++ b/components/autofill/core/browser/autofill_manager.cc
@@ -60,6 +60,7 @@
 #include "components/autofill/core/common/form_data_predictions.h"
 #include "components/autofill/core/common/form_field_data.h"
 #include "components/autofill/core/common/password_form_fill_data.h"
+#include "components/autofill/core/common/signatures_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/public/rappor_utils.h"
@@ -1679,7 +1680,7 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm(
   if (!FindCachedForm(form, &cached_submitted_form))
     return std::unique_ptr<FormStructure>();
 
-  submitted_form->UpdateFromCache(*cached_submitted_form);
+  submitted_form->UpdateFromCache(*cached_submitted_form, false);
   return submitted_form;
 }
 
@@ -1692,8 +1693,9 @@ bool AutofillManager::FindCachedForm(const FormData& form,
   // protocol with the crowdsourcing server does not permit us to discard the
   // original versions of the forms.
   *form_structure = NULL;
+  const auto& form_signature = autofill::CalculateFormSignature(form);
   for (auto& cur_form : base::Reversed(form_structures_)) {
-    if (*cur_form == form) {
+    if (cur_form->form_signature() == form_signature || *cur_form == form) {
       *form_structure = cur_form.get();
 
       // The same form might be cached with multiple field counts: in some
@@ -1775,41 +1777,17 @@ bool AutofillManager::UpdateCachedForm(const FormData& live_form,
   if (!needs_update)
     return true;
 
-  if (form_structures_.size() >= kMaxFormCacheSize)
+  // Note: We _must not_ remove the original version of the cached form from
+  // the list of |form_structures_|. Otherwise, we break parsing of the
+  // crowdsourcing server's response to our query.
+  if (!ParseForm(live_form, updated_form))
     return false;
 
-  // Add the new or updated form to our cache.
-  form_structures_.push_back(base::MakeUnique<FormStructure>(live_form));
-  *updated_form = form_structures_.rbegin()->get();
-  (*updated_form)->DetermineHeuristicTypes();
-
-  // If we have cached data, propagate it to the updated form.
-  if (cached_form) {
-    std::map<base::string16, const AutofillField*> cached_fields;
-    for (size_t i = 0; i < cached_form->field_count(); ++i) {
-      const AutofillField* field = cached_form->field(i);
-      cached_fields[field->unique_name()] = field;
-    }
-
-    for (size_t i = 0; i < (*updated_form)->field_count(); ++i) {
-      AutofillField* field = (*updated_form)->field(i);
-      auto cached_field = cached_fields.find(field->unique_name());
-      if (cached_field != cached_fields.end()) {
-        field->set_server_type(cached_field->second->server_type());
-        field->is_autofilled = cached_field->second->is_autofilled;
-        field->set_previously_autofilled(
-            cached_field->second->previously_autofilled());
-      }
-    }
-
-    // Note: We _must not_ remove the original version of the cached form from
-    // the list of |form_structures_|.  Otherwise, we break parsing of the
-    // crowdsourcing server's response to our query.
-  }
+  if (cached_form)
+    (*updated_form)->UpdateFromCache(*cached_form, true);
 
   // Annotate the updated form with its predicted types.
-  std::vector<FormStructure*> forms(1, *updated_form);
-  driver_->SendAutofillTypePredictionsToRenderer(forms);
+  driver_->SendAutofillTypePredictionsToRenderer({*updated_form});
 
   return true;
 }
@@ -1870,23 +1848,17 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
   for (const FormData& form : forms) {
     const auto parse_form_start_time = base::TimeTicks::Now();
 
-    std::unique_ptr<FormStructure> form_structure =
-        base::MakeUnique<FormStructure>(form);
-    form_structure->ParseFieldTypesFromAutocompleteAttributes();
-    if (!form_structure->ShouldBeParsed())
+    FormStructure* form_structure = nullptr;
+    if (!ParseForm(form, &form_structure))
       continue;
+    DCHECK(form_structure);
 
-    form_structure->DetermineHeuristicTypes();
-
-    // Ownership is transferred to |form_structures_| which maintains it until
-    // the manager is Reset() or destroyed. It is safe to use references below
-    // as long as receivers don't take ownership.
-    form_structures_.push_back(std::move(form_structure));
-
-    if (form_structures_.back()->ShouldBeCrowdsourced())
-      queryable_forms.push_back(form_structures_.back().get());
+    // Set aside forms with method GET or author-specified types, so that they
+    // are not included in the query to the server.
+    if (form_structure->ShouldBeCrowdsourced())
+      queryable_forms.push_back(form_structure);
     else
-      non_queryable_forms.push_back(form_structures_.back().get());
+      non_queryable_forms.push_back(form_structure);
 
     AutofillMetrics::LogParseFormTiming(base::TimeTicks::Now() -
                                         parse_form_start_time);
@@ -1928,6 +1900,27 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
   driver_->SendAutofillTypePredictionsToRenderer(non_queryable_forms);
 }
 
+bool AutofillManager::ParseForm(const FormData& form,
+                                FormStructure** parsed_form_structure) {
+  DCHECK(parsed_form_structure);
+  if (form_structures_.size() >= kMaxFormCacheSize)
+    return false;
+
+  auto form_structure = base::MakeUnique<FormStructure>(form);
+  form_structure->ParseFieldTypesFromAutocompleteAttributes();
+  if (!form_structure->ShouldBeParsed())
+    return false;
+
+  form_structure->DetermineHeuristicTypes();

[sebsg, 2017/03/13 12:51:21]

Would you mind moving this after the add like in the original code? Thanks!

+
+  // Ownership is transferred to |form_structures_| which maintains it until
+  // the manager is Reset() or destroyed. It is safe to use references below
+  // as long as receivers don't take ownership.
+  form_structures_.push_back(std::move(form_structure));
+  *parsed_form_structure = form_structures_.back().get();
+  return true;
+}
+
 int AutofillManager::BackendIDToInt(const std::string& backend_id) const {
   if (!base::IsValidGUID(backend_id))
     return 0;