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..7c0cf18d941816f1061ca1ede51e11aef22e0947 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,19 @@ bool AutofillManager::UpdateCachedForm(const FormData& live_form,
   if (!needs_update)
     return true;
 
-  if (form_structures_.size() >= kMaxFormCacheSize)
-    return false;
-
-  // Add the new or updated form to our cache.

[sense (YandexTeam), 2017/03/07 07:10:28]

This logic is replaced by the call to ParseForm().

-  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;

[sense (YandexTeam), 2017/03/07 07:10:28]

This update logic is replaced by the call to FormStructure::UpdateFromCache().
This code didn't perform the call to UpdateAutofillCount(). Without it the
update breaks the count of autofillable fields and the whole form becomes
non-autofillable if there were only server defined field types.
Also we should keep form_signature here as does UpdateFromCache().

-    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.
+  // 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, false, updated_form)) {
+    // New form was not added, so no update was performed.
+    return cached_form != nullptr;

[sebsg, 2017/03/08 18:32:08]

Can this change the return value compared to before? Before, if
(form_structures_.size() >= kMaxFormCacheSize) we returned false but here if
(form_structures_.size() >= kMaxFormCacheSize) but cached_form != nullptr we
will return true?

[sense (YandexTeam), 2017/03/09 04:56:36]

Yes, this change is incorrect. We shouldn't return the form if it's need to be
updated but we failed to do that.

   }
 
+  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 +1850,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, true, &form_structure))

[sebsg, 2017/03/08 18:32:08]

I'm not sure why this needs to be true here? It means that this can continue if
we already have the form which is a logic that I do not see in the current code?
Could you explain why it is better to have it? Thanks!

[sense (YandexTeam), 2017/03/09 04:56:36]

The check for duplicates is from our internal improvements. It's not meant to be
in this CL. I'll remove it, sorry for that.

       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 +1902,40 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
   driver_->SendAutofillTypePredictionsToRenderer(non_queryable_forms);
 }
 
+bool AutofillManager::ParseForm(const FormData& form,
+                                const bool check_for_duplicates,
+                                FormStructure** parsed_form_structure) {
+  DCHECK(parsed_form_structure);
+  if (form_structures_.size() >= kMaxFormCacheSize)

[sense (YandexTeam), 2017/03/07 07:10:28]

Previously this check was only in UpdateCachedForm. Now it also applies to the
initial ParseForms() call (OnFormsSeen->ParseForms).

+    return false;
+
+  auto form_structure = base::MakeUnique<FormStructure>(form);
+  form_structure->ParseFieldTypesFromAutocompleteAttributes();
+  if (!form_structure->ShouldBeParsed())
+    return false;
+
+  if (check_for_duplicates) {
+    // Check if we have got this form before since the agent uses a different
+    // form comparison method that can send us duplicates here.
+    for (const auto& existing_form_structure : form_structures_) {
+      // If we already got this form, skip it.
+      if (form_structure->form_signature() ==
+          existing_form_structure->form_signature()) {
+        return false;
+      }
+    }
+  }
+
+  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));
+  *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;