[Merge 105] Reland "Introduce IsValueNotAutofilledOverExistingValueSa…

…meAsSubmittedValue2 metric" This is a reland of commit aa297fe Original change's description: > Introduce IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2 metric > > With this CL, IsValueNotAutofilledOverExistingValueSameAsSubmittedValue > metric becomes obsolete. Instead IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2 is introduced. This metric records, for each field that had an initial > value and did not get autofilled due to it, in how many cases would > the user submitted field value was same as the one autofill would > have suggested. > > Bug: 1275649 > Change-Id: I33f0bc42f4585b2bbcbbadc9e350547728729f83 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3745389 > Commit-Queue: Vidhan Jain <vidhanj@google.com> > Reviewed-by: Matthias Körber <koerber@google.com> > Cr-Commit-Position: refs/heads/main@{#1026940} (cherry picked from commit ab09241) Bug: 1275649, 1346535 Change-Id: I6784816e62d0ca1be5a9a2e265803f37a456a929 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3780806 Commit-Queue: Vidhan Jain <vidhanj@google.com> Reviewed-by: Matthias Körber <koerber@google.com> Cr-Original-Commit-Position: refs/heads/main@{#1027178} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3785368 Commit-Queue: Matthias Körber <koerber@google.com> Auto-Submit: Vidhan Jain <vidhanj@google.com> Cr-Commit-Position: refs/branch-heads/5195@{#27} Cr-Branched-From: 7aa3f07-refs/heads/main@{#1027018}
blueboxd · Jul 26, 2022 · db52bd2 · db52bd2
1 parent 685b149
commit db52bd2
Show file tree

Hide file tree

Showing 8 changed files with 74 additions and 40 deletions.
diff --git a/components/autofill/core/browser/autofill_form_test_utils.cc b/components/autofill/core/browser/autofill_form_test_utils.cc
@@ -115,6 +115,7 @@ FormData GetFormData(const FormDataDescription& d) {
     ff.is_autofilled = dd.is_autofilled.value_or(false);
     ff.origin = dd.origin.value_or(f.main_frame_origin);
     ff.should_autocomplete = dd.should_autocomplete;
+    ff.properties_mask = dd.properties_mask;
     f.fields.push_back(ff);
   }
   return f;

diff --git a/components/autofill/core/browser/autofill_form_test_utils.h b/components/autofill/core/browser/autofill_form_test_utils.h
@@ -47,6 +47,7 @@ struct FieldDataDescription {
   absl::optional<bool> is_autofilled;
   absl::optional<url::Origin> origin;
   std::vector<SelectOption> select_options = {};
+  FieldPropertiesMask properties_mask = 0;
 };
 
 // Attributes provided to the test form.

diff --git a/components/autofill/core/browser/browser_autofill_manager.cc b/components/autofill/core/browser/browser_autofill_manager.cc
@@ -695,8 +695,12 @@ void BrowserAutofillManager::OnFormSubmittedImpl(const FormData& form,
       form_for_autocomplete.fields[i].should_autocomplete = false;
     }
 
+    // If the field was edited by the user and there existed an autofillable
+    // value for the field, log whether the value on submission is same as the
+    // autofillable value.
     if (submitted_form->field(i)
-            ->value_not_autofilled_over_existing_value_hash()) {
+            ->value_not_autofilled_over_existing_value_hash() &&
+        (submitted_form->field(i)->properties_mask & kUserTyped)) {
       // Compare and record if the currently filled value is same as the
       // non-empty value that was to be autofilled in the field.
       AutofillMetrics::
@@ -1922,6 +1926,8 @@ void BrowserAutofillManager::FillOrPreviewDataModelForm(
     // case.
     if (base::FeatureList::IsEnabled(
             features::kAutofillPreventOverridingPrefilledValues)) {
+      form_structure->field(i)
+          ->set_value_not_autofilled_over_existing_value_hash(absl::nullopt);
       if (form.fields[i].form_control_type != "select-one" &&
           !form.fields[i].value.empty() && !has_override &&
           !FormFieldData::DeepEqual(form.fields[i], field)) {
@@ -1933,18 +1939,16 @@ void BrowserAutofillManager::FillOrPreviewDataModelForm(
             optional_cvc ? *optional_cvc : kEmptyCvc, action,
             &unused_failure_to_fill);
         if (action == mojom::RendererFormDataAction::kFill &&
-            !fill_value.empty() && fill_value != form.fields[i].value) {
-          // Save the value that was supposed to be autofilled for this field.
+            !fill_value.empty() && fill_value != form.fields[i].value &&
+            !form_structure->field(i)->value.empty()) {
+          // Save the value that was supposed to be autofilled for this field if
+          // the field contained an initial value.
           form_structure->field(i)
               ->set_value_not_autofilled_over_existing_value_hash(
                   base::FastHash(base::UTF16ToUTF8(fill_value)));
         }
         continue;
       }
-
-      // Clear out the value in case the autofill happens for the field.
-      form_structure->field(i)
-          ->set_value_not_autofilled_over_existing_value_hash(absl::nullopt);
     }
 
     // Do not fill fields that have been edited by the user, except if the field

diff --git a/components/autofill/core/browser/metrics/autofill_metrics.cc b/components/autofill/core/browser/metrics/autofill_metrics.cc
@@ -3598,7 +3598,7 @@ void AutofillMetrics::LogOtpInputDialogNewOtpRequested() {
 void AutofillMetrics::
     LogIsValueNotAutofilledOverExistingValueSameAsSubmittedValue(bool is_same) {
   base::UmaHistogramBoolean(
-      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue",
+      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2",
       is_same);
 }
 

diff --git a/components/autofill/core/browser/metrics/autofill_metrics_unittest.cc b/components/autofill/core/browser/metrics/autofill_metrics_unittest.cc
@@ -11863,22 +11863,22 @@ TEST_F(AutofillMetricsTest, AutofilledStateFieldSource) {
 
 // Tests the following 4 cases when |kAutofillPreventOverridingPrefilledValues|
 // is enabled:
-// 1. The field is not autofilled since it has a prefilled value but the value
+// 1. The field is not autofilled since it has an initial value but the value
 //    is edited before the form submission and is same as the value that was
 //    to be autofilled in the field.
-//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue|
+//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2|
 //    should emit true for this case.
-// 2. The field is not autofilled since it has a prefilled value but the value
+// 2. The field is not autofilled since it has an initial value but the value
 //    is edited before the form submission and is different than the value that
 //    was to be autofilled in the field.
-//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue|
+//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2|
 //    should emit false for this case.
-// 3. The field had a prefilled value that was similar to the value to be
+// 3. The field had an initial value that was similar to the value to be
 //    autofilled in the field.
-//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue|
+//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2|
 //    should not record anything in this case.
 // 4. Selection fields are always overridden by Autofill.
-//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue|
+//    |Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2|
 //    should not record anything in this case.
 TEST_F(AutofillMetricsTest,
        IsValueNotAutofilledOverExistingValueSameAsSubmittedValue) {
@@ -11889,20 +11889,23 @@ TEST_F(AutofillMetricsTest,
 
   FormData form = test::GetFormData(
       {.description_for_logging = "AutofilledStateFieldSource",
-       .fields = {{.role = ServerFieldType::NAME_FULL},
-                  {.role = ServerFieldType::ADDRESS_HOME_CITY,
-                   .value = u"Sacremento"},  // Case #1
-                  {.role = ServerFieldType::ADDRESS_HOME_STATE,
-                   .value = u"CA",
-                   .form_control_type = "select-one",
-                   .select_options = {{u"TN", u"Tennesse"},
-                                      {u"CA", u"California"},
-                                      {u"WA", u"Washington DC"}}},  // Case #4
-                  {.role = ServerFieldType::ADDRESS_HOME_ZIP,
-                   .value = u"00000"},  // Case #2
-                  {.role = ServerFieldType::PHONE_HOME_WHOLE_NUMBER,
-                   .value = u"12345678901"},  // Case #3
-                  {.role = ServerFieldType::ADDRESS_HOME_COUNTRY}}});
+       .fields = {
+           {.role = ServerFieldType::NAME_FULL},
+           {.role = ServerFieldType::ADDRESS_HOME_CITY,
+            .value = u"Sacremento",
+            .properties_mask = FieldPropertiesFlags::kUserTyped},  // Case #1
+           {.role = ServerFieldType::ADDRESS_HOME_STATE,
+            .value = u"CA",
+            .form_control_type = "select-one",
+            .select_options = {{u"TN", u"Tennesse"},
+                               {u"CA", u"California"},
+                               {u"WA", u"Washington DC"}}},  // Case #4
+           {.role = ServerFieldType::ADDRESS_HOME_ZIP,
+            .value = u"00000",
+            .properties_mask = FieldPropertiesFlags::kUserTyped},  // Case #2
+           {.role = ServerFieldType::PHONE_HOME_WHOLE_NUMBER,
+            .value = u"12345678901"},  // Case #3
+           {.role = ServerFieldType::ADDRESS_HOME_COUNTRY}}});
 
   std::vector<ServerFieldType> heuristic_types = {
       NAME_FULL,        ADDRESS_HOME_CITY,       ADDRESS_HOME_STATE,
@@ -11912,7 +11915,8 @@ TEST_F(AutofillMetricsTest,
       ADDRESS_HOME_ZIP, PHONE_HOME_WHOLE_NUMBER, ADDRESS_HOME_COUNTRY};
 
   // Simulate having seen this form on page load.
-  autofill_manager().AddSeenForm(form, heuristic_types, server_types);
+  autofill_manager().AddSeenForm(form, heuristic_types, server_types,
+                                 /*preserve_values_in_form_structure=*/true);
 
   autofill_manager().OnAskForValuesToFillTest(form, form.fields[0]);
   autofill_manager().DidShowSuggestions(
@@ -11942,10 +11946,10 @@ TEST_F(AutofillMetricsTest,
                                      SubmissionSource::FORM_SUBMISSION);
 
   histogram_tester.ExpectBucketCount(
-      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue",
+      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2",
       true, 1);
   histogram_tester.ExpectBucketCount(
-      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue",
+      "Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2",
       false, 1);
 }
 

diff --git a/components/autofill/core/browser/test_browser_autofill_manager.cc b/components/autofill/core/browser/test_browser_autofill_manager.cc
@@ -107,7 +107,8 @@ int TestBrowserAutofillManager::GetPackedCreditCardID(int credit_card_id) {
 void TestBrowserAutofillManager::AddSeenForm(
     const FormData& form,
     const std::vector<ServerFieldType>& heuristic_types,
-    const std::vector<ServerFieldType>& server_types) {
+    const std::vector<ServerFieldType>& server_types,
+    bool preserve_values_in_form_structure) {
   std::vector<std::vector<std::pair<PatternSource, ServerFieldType>>>
       all_heuristic_types;
 
@@ -118,21 +119,24 @@ void TestBrowserAutofillManager::AddSeenForm(
         return {{GetActivePatternSource(), type}};
       });
 
-  AddSeenForm(form, all_heuristic_types, server_types);
+  AddSeenForm(form, all_heuristic_types, server_types,
+              preserve_values_in_form_structure);
 }
 
 void TestBrowserAutofillManager::AddSeenForm(
     const FormData& form,
     const std::vector<std::vector<std::pair<PatternSource, ServerFieldType>>>&
         heuristic_types,
-    const std::vector<ServerFieldType>& server_types) {
-  FormData empty_form = form;
-  for (auto& field : empty_form.fields) {
+    const std::vector<ServerFieldType>& server_types,
+    bool preserve_values_in_form_structure) {
+  FormData form_with_empty_fields = form;
+  for (auto& field : form_with_empty_fields.fields) {
     field.value = std::u16string();
   }
 
   std::unique_ptr<TestFormStructure> form_structure =
-      std::make_unique<TestFormStructure>(empty_form);
+      std::make_unique<TestFormStructure>(
+          preserve_values_in_form_structure ? form : form_with_empty_fields);
   form_structure->SetFieldTypes(heuristic_types, server_types);
   form_structure->identify_sections_for_testing();
   AddSeenFormStructure(std::move(form_structure));

diff --git a/components/autofill/core/browser/test_browser_autofill_manager.h b/components/autofill/core/browser/test_browser_autofill_manager.h
@@ -58,13 +58,15 @@ class TestBrowserAutofillManager : public BrowserAutofillManager {
 
   void AddSeenForm(const FormData& form,
                    const std::vector<ServerFieldType>& heuristic_types,
-                   const std::vector<ServerFieldType>& server_types);
+                   const std::vector<ServerFieldType>& server_types,
+                   bool preserve_values_in_form_structure = false);
 
   void AddSeenForm(
       const FormData& form,
       const std::vector<std::vector<std::pair<PatternSource, ServerFieldType>>>&
           heuristic_types,
-      const std::vector<ServerFieldType>& server_types);
+      const std::vector<ServerFieldType>& server_types,
+      bool preserve_values_in_form_structure = false);
 
   void AddSeenFormStructure(std::unique_ptr<FormStructure> form_structure);
 

diff --git a/tools/metrics/histograms/metadata/autofill/histograms.xml b/tools/metrics/histograms/metadata/autofill/histograms.xml
@@ -1730,6 +1730,10 @@ chromium-metrics-reviews@google.com.
 <histogram
     name="Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue"
     enum="Boolean" expires_after="2022-12-25">
+  <obsolete>
+    Deprecated in July 2022. Subsumed by
+    Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2.
+  </obsolete>
   <owner>vidhanj@google.com</owner>
   <owner>koerber@google.com</owner>
   <owner>chrome-autofill-alerts@google.com</owner>
@@ -1741,6 +1745,20 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
+<histogram
+    name="Autofill.IsValueNotAutofilledOverExistingValueSameAsSubmittedValue2"
+    enum="Boolean" expires_after="2022-12-25">
+  <owner>vidhanj@google.com</owner>
+  <owner>koerber@google.com</owner>
+  <owner>chrome-autofill-alerts@google.com</owner>
+  <summary>
+    This metric is recorderd on form submission for each field that had an
+    initial value on page load and was edited by the user later on. The result
+    of the equality comparison between the submitted field value and supposedly
+    autofillable value is emitted by this metric.
+  </summary>
+</histogram>
+
 <histogram name="Autofill.KeyMetrics.FillingAcceptance{AutofillFormType}"
     enum="BooleanAutofillFillingAcceptance" expires_after="2022-12-12">
   <owner>battre@chromium.org</owner>