[Autofill] Determine Heuristics based on Overall Prediction

Problem:

On the last run to determine heuristics we do not consider the
overall server type detections, and we only rely on field types
detected by heuristics. Since the ultimate filling decisions
are based on the overall detection, we need to use overall type
detection. As in section identification this can sometimes
cause an incorrect output.

Exp:
In the cabelas.com payment section, the heuristics detect card holder
name as the address name, whereas the overall type is detected
correctly. The sections are thus wrong.

Solution:
Update the form from the cache, before determining the heuristics for
the last time.

Test added.

Bug: 831739
Change-Id: Ifdecfa442a21d38b53169c8dafc981ca5447629b
Reviewed-on: https://chromium-review.googlesource.com/1012980Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551076}

components/autofill/core/browser/autofill_manager.cc

@@ -749,8 +749,8 @@ void AutofillManager::FillOrPreviewCreditCardForm(
   }
 
   FillOrPreviewDataModelForm(
-      action, query_id, form, field, credit_card, true /* is_credit_card */,
-      base::string16() /* cvc */, form_structure, autofill_field);
+      action, query_id, form, field, credit_card, /*is_credit_card=*/true,
+      /*cvc=*/base::string16(), form_structure, autofill_field);
 }
 
 void AutofillManager::FillOrPreviewProfileForm(
@@ -780,8 +780,8 @@ void AutofillManager::FillOrPreviewProfileForm(
   }
 
   FillOrPreviewDataModelForm(
-      action, query_id, form, field, profile, false /* is_credit_card */,
-      base::string16() /* cvc */, form_structure, autofill_field);
+      action, query_id, form, field, profile, /*is_credit_card=*/false,
+      /*cvc=*/base::string16(), form_structure, autofill_field);
 }
 
 void AutofillManager::FillOrPreviewForm(
@@ -823,7 +823,7 @@ void AutofillManager::FillCreditCardForm(int query_id,
 
   FillOrPreviewDataModelForm(
       AutofillDriver::FORM_DATA_ACTION_FILL, query_id, form, field, credit_card,
-      true /* is_credit_card */, cvc, form_structure, autofill_field);
+      /*is_credit_card=*/true, cvc, form_structure, autofill_field);
 }
 
 void AutofillManager::OnFocusNoLongerOnForm() {
@@ -1033,7 +1033,7 @@ void AutofillManager::OnSetDataList(const std::vector<base::string16>& values,
 
 void AutofillManager::SelectFieldOptionsDidChange(const FormData& form) {
   FormStructure* form_structure = nullptr;
-  if (!ParseForm(form, &form_structure))
+  if (!ParseForm(form, /*cached_form=*/nullptr, &form_structure))
     return;
 
   if (ShouldTriggerRefill(*form_structure))
@@ -1176,7 +1176,7 @@ void AutofillManager::UploadFormData(const FormStructure& submitted_form,
 
   download_manager_->StartUploadRequest(
       submitted_form, was_autofilled, non_empty_types,
-      std::string() /* login_form_signature */, observed_submission);
+      /*login_form_signature=*/std::string(), observed_submission);
 }
 
 void AutofillManager::Reset() {
@@ -1189,9 +1189,9 @@ void AutofillManager::Reset() {
       new AutofillMetrics::FormInteractionsUkmLogger(
           client_->GetUkmRecorder()));
   address_form_event_logger_.reset(new AutofillMetrics::FormEventLogger(
-      false /* is_for_credit_card */, form_interactions_ukm_logger_.get()));
+      /*is_for_credit_card=*/false, form_interactions_ukm_logger_.get()));
   credit_card_form_event_logger_.reset(new AutofillMetrics::FormEventLogger(
-      true /* is_for_credit_card */, form_interactions_ukm_logger_.get()));
+      /*is_for_credit_card=*/true, form_interactions_ukm_logger_.get()));
 #if defined(OS_ANDROID) || defined(OS_IOS)
   autofill_assistant_.Reset();
 #endif
@@ -1243,11 +1243,11 @@ AutofillManager::AutofillManager(
               client->GetUkmRecorder())),
       address_form_event_logger_(
           std::make_unique<AutofillMetrics::FormEventLogger>(
-              false /* is_for_credit_card */,
+              /*is_for_credit_card=*/false,
               form_interactions_ukm_logger_.get())),
       credit_card_form_event_logger_(
           std::make_unique<AutofillMetrics::FormEventLogger>(
-              true /* is_for_credit_card */,
+              /*is_for_credit_card=*/true,
               form_interactions_ukm_logger_.get())),
 #if defined(OS_ANDROID) || defined(OS_IOS)
       autofill_assistant_(this),
@@ -1466,8 +1466,8 @@ std::unique_ptr<FormStructure> AutofillManager::ValidateSubmittedForm(
     return std::unique_ptr<FormStructure>();
 
   submitted_form->RetrieveFromCache(*cached_submitted_form,
-                                    /* apply_is_autofilled */ false,
-                                    /* only_server_and_autofill_state */ false);
+                                    /*apply_is_autofilled=*/false,
+                                    /*only_server_and_autofill_state=*/false);
   return submitted_form;
 }
 
@@ -1566,14 +1566,9 @@ bool AutofillManager::UpdateCachedForm(const FormData& live_form,
   // 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))
+  if (!ParseForm(live_form, cached_form, updated_form))
     return false;
 
-  // We need to keep the server data.
-  if (cached_form)
-    (*updated_form)
-        ->RetrieveFromCache(*cached_form, /* apply_is_autofilled */ true,
-                            /* only_server_and_autofill_state */ true);
   // Annotate the updated form with its predicted types.
   driver()->SendAutofillTypePredictionsToRenderer({*updated_form});
 
@@ -1659,7 +1654,7 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
     const auto parse_form_start_time = TimeTicks::Now();
 
     FormStructure* form_structure = nullptr;
-    if (!ParseForm(form, &form_structure))
+    if (!ParseForm(form, /*cached_form=*/nullptr, &form_structure))
       continue;
     DCHECK(form_structure);
 
@@ -1737,6 +1732,7 @@ void AutofillManager::ParseForms(const std::vector<FormData>& forms) {
 }
 
 bool AutofillManager::ParseForm(const FormData& form,
+                                const FormStructure* cached_form,
                                 FormStructure** parsed_form_structure) {
   DCHECK(parsed_form_structure);
   if (form_structures_.size() >= kMaxFormCacheSize)
@@ -1747,6 +1743,14 @@ bool AutofillManager::ParseForm(const FormData& form,
   if (!form_structure->ShouldBeParsed())
     return false;
 
+  if (cached_form) {
+    // We need to keep the server data if available. We need to use them while
+    // determining the heuristics.
+    form_structure->RetrieveFromCache(*cached_form,
+                                      /*apply_is_autofilled=*/true,
+                                      /*only_server_and_autofill_state=*/true);
+  }
+
   // 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.

components/autofill/core/browser/autofill_manager.h

@@ -430,8 +430,13 @@ class AutofillManager : public AutofillHandler,
   // Parses the forms using heuristic matching and querying the Autofill server.
   void ParseForms(const std::vector<FormData>& forms);
 
-  // Parses the form and adds it to |form_structures_|.
-  bool ParseForm(const FormData& form, FormStructure** parsed_form_structure);
+  // Parses the |form| with the server data retrieved from the |cached_form|
+  // (if any), and writes it to the |parse_form_structure|. Adds the
+  // |parse_form_structure| to the |form_structures_|. Returns true if the form
+  // is parsed.
+  bool ParseForm(const FormData& form,
+                 const FormStructure* cached_form,
+                 FormStructure** parsed_form_structure);
 
   // If |initial_interaction_timestamp_| is unset or is set to a later time than
   // |interaction_timestamp|, updates the cached timestamp.  The latter check is
@@ -606,6 +611,8 @@ class AutofillManager : public AutofillHandler,
   FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DisambiguateUploadTypes);
   FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest,
                            DisabledAutofillDispatchesError);
+  FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest,
+                           DetermineHeuristicsWithOverallPrediction);
   FRIEND_TEST_ALL_PREFIXES(AutofillMetricsTest, AddressFilledFormEvents);
   FRIEND_TEST_ALL_PREFIXES(AutofillMetricsTest, AddressSubmittedFormEvents);
   FRIEND_TEST_ALL_PREFIXES(AutofillMetricsTest, AddressWillSubmitFormEvents);

components/autofill/core/browser/autofill_manager_unittest.cc

@@ -4261,6 +4261,87 @@ TEST_F(AutofillManagerTest, OnLoadedServerPredictions_ResetManager) {
   histogram_tester.ExpectTotalCount("Autofill.ServerQueryResponse", 0);
 }
 
+// Test that when server predictions disagree with the heuristic ones, the
+// overall types and sections would be set based on the server one.
+TEST_F(AutofillManagerTest, DetermineHeuristicsWithOverallPrediction) {
+  // Set up our form data.
+  FormData form;
+  FormFieldData field;
+  test::CreateTestFormField("First Name", "firstname", "", "text", &field);
+  form.fields.push_back(field);
+
+  test::CreateTestFormField("Last Name", "lastname", "", "text", &field);
+  form.fields.push_back(field);
+
+  test::CreateTestFormField("Card Number", "cardnumber", "", "text", &field);
+  form.fields.push_back(field);
+
+  test::CreateTestFormField("Expiration Year", "exp_year", "", "text", &field);
+  form.fields.push_back(field);
+
+  test::CreateTestFormField("Expiration Month", "exp_month", "", "text",
+                            &field);
+  form.fields.push_back(field);
+
+  // Simulate having seen this form on page load.
+  // |form_structure| will be owned by |autofill_manager_|.
+  TestFormStructure* form_structure = new TestFormStructure(form);
+  form_structure->DetermineHeuristicTypes(nullptr /* ukm_recorder */);
+  autofill_manager_->AddSeenFormStructure(base::WrapUnique(form_structure));
+
+  AutofillQueryResponseContents response;
+  response.add_field()->set_overall_type_prediction(CREDIT_CARD_NAME_FIRST);
+  response.add_field()->set_overall_type_prediction(CREDIT_CARD_NAME_LAST);
+  response.add_field()->set_overall_type_prediction(CREDIT_CARD_NUMBER);
+  response.add_field()->set_overall_type_prediction(CREDIT_CARD_EXP_MONTH);
+  response.add_field()->set_overall_type_prediction(
+      CREDIT_CARD_EXP_4_DIGIT_YEAR);
+
+  std::string response_string;
+  ASSERT_TRUE(response.SerializeToString(&response_string));
+
+  std::vector<std::string> signatures;
+  signatures.push_back(form_structure->FormSignatureAsStr());
+
+  base::HistogramTester histogram_tester;
+  autofill_manager_->OnLoadedServerPredictions(response_string, signatures);
+  // Verify that FormStructure::ParseQueryResponse was called (here and below).
+  histogram_tester.ExpectBucketCount("Autofill.ServerQueryResponse",
+                                     AutofillMetrics::QUERY_RESPONSE_RECEIVED,
+                                     1);
+  histogram_tester.ExpectBucketCount("Autofill.ServerQueryResponse",
+                                     AutofillMetrics::QUERY_RESPONSE_PARSED, 1);
+
+  // Since the card holder name appears as the first name + last name (rather
+  // than the full name), and since they appears as the first fields of the
+  // section, the heuristics detect them as the address first/last name.
+  EXPECT_EQ(NAME_FIRST, form_structure->field(0)->heuristic_type());
+  EXPECT_EQ(NAME_LAST, form_structure->field(1)->heuristic_type());
+  EXPECT_EQ(CREDIT_CARD_NUMBER, form_structure->field(2)->heuristic_type());
+  EXPECT_EQ(CREDIT_CARD_EXP_MONTH, form_structure->field(3)->heuristic_type());
+  EXPECT_EQ(CREDIT_CARD_EXP_4_DIGIT_YEAR,
+            form_structure->field(4)->heuristic_type());
+
+  // We expect to see the server type as the overall type.
+  EXPECT_EQ(CREDIT_CARD_NAME_FIRST,
+            form_structure->field(0)->Type().GetStorableType());
+  EXPECT_EQ(CREDIT_CARD_NAME_LAST,
+            form_structure->field(1)->Type().GetStorableType());
+  EXPECT_EQ(CREDIT_CARD_NUMBER, form_structure->field(2)->heuristic_type());
+  EXPECT_EQ(CREDIT_CARD_EXP_MONTH, form_structure->field(3)->heuristic_type());
+  EXPECT_EQ(CREDIT_CARD_EXP_4_DIGIT_YEAR,
+            form_structure->field(4)->heuristic_type());
+
+  // Although the heuristic types of the first two fields belongs to the address
+  // section, the final fields' section should be based on the overall
+  // prediction, therefore they should be grouped in one section.
+  const auto section = form_structure->field(0)->section();
+  EXPECT_EQ(section, form_structure->field(1)->section());
+  EXPECT_EQ(section, form_structure->field(2)->section());
+  EXPECT_EQ(section, form_structure->field(3)->section());
+  EXPECT_EQ(section, form_structure->field(4)->section());
+}
+
 // Test that we are able to save form data when forms are submitted and we only
 // have server data for the field types.
 TEST_F(AutofillManagerTest, FormSubmittedServerTypes) {