From 91c836cd88c812ee18f53f16e3d758111b056c60 Mon Sep 17 00:00:00 2001 From: Shanfeng Zhang Date: Thu, 12 Oct 2017 20:16:54 +0000 Subject: [PATCH] Split FillDuration by form type Detailed here: https://docs.google.com/document/d/1bAxMLPgF3X3CCYrBa7gI4n84CObh6Ch_bDizeZG_HWI/edit# Bug: Change-Id: I71dc3d9e3d248aafb609b29b5f655cb0a8f4ab7f Reviewed-on: https://chromium-review.googlesource.com/679556 Reviewed-by: Robert Kaplow Reviewed-by: Roger McFarlane Commit-Queue: Shanfeng Zhang Cr-Commit-Position: refs/heads/master@{#508434} --- .../autofill/core/browser/autofill_metrics.cc | 50 ++-- .../autofill/core/browser/autofill_metrics.h | 17 +- .../core/browser/autofill_metrics_unittest.cc | 220 +++++++++++++++++- .../autofill/core/browser/form_structure.cc | 14 +- .../autofill/core/browser/form_structure.h | 2 +- tools/metrics/histograms/histograms.xml | 6 +- 6 files changed, 261 insertions(+), 48 deletions(-) diff --git a/components/autofill/core/browser/autofill_metrics.cc b/components/autofill/core/browser/autofill_metrics.cc index c6fece71836f33..cf75471486c08d 100644 --- a/components/autofill/core/browser/autofill_metrics.cc +++ b/components/autofill/core/browser/autofill_metrics.cc @@ -9,6 +9,7 @@ #include #include "base/logging.h" +#include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" #include "base/metrics/sparse_histogram.h" #include "base/metrics/user_metrics.h" @@ -742,7 +743,7 @@ void AutofillMetrics::LogUserHappinessMetric( NUM_USER_HAPPINESS_METRICS); } if (base::ContainsKey(form_types, UNKNOWN_FORM_TYPE)) { - UMA_HISTOGRAM_ENUMERATION("Autofill.UserHappiness.UnknownFormType", metric, + UMA_HISTOGRAM_ENUMERATION("Autofill.UserHappiness.Unknown", metric, NUM_USER_HAPPINESS_METRICS); } } @@ -750,35 +751,48 @@ void AutofillMetrics::LogUserHappinessMetric( // static void AutofillMetrics::LogFormFillDurationFromLoadWithAutofill( const base::TimeDelta& duration) { - UMA_HISTOGRAM_CUSTOM_TIMES("Autofill.FillDuration.FromLoad.WithAutofill", - duration, base::TimeDelta::FromMilliseconds(100), - base::TimeDelta::FromMinutes(10), 50); + LogFormFillDuration("Autofill.FillDuration.FromLoad.WithAutofill", duration); } // static void AutofillMetrics::LogFormFillDurationFromLoadWithoutAutofill( const base::TimeDelta& duration) { - UMA_HISTOGRAM_CUSTOM_TIMES("Autofill.FillDuration.FromLoad.WithoutAutofill", - duration, base::TimeDelta::FromMilliseconds(100), - base::TimeDelta::FromMinutes(10), 50); + LogFormFillDuration("Autofill.FillDuration.FromLoad.WithoutAutofill", + duration); } // static -void AutofillMetrics::LogFormFillDurationFromInteractionWithAutofill( +void AutofillMetrics::LogFormFillDurationFromInteraction( + const std::set& form_types, + bool used_autofill, const base::TimeDelta& duration) { - UMA_HISTOGRAM_CUSTOM_TIMES( - "Autofill.FillDuration.FromInteraction.WithAutofill", duration, - base::TimeDelta::FromMilliseconds(100), base::TimeDelta::FromMinutes(10), - 50); + std::string parent_metric; + if (used_autofill) { + parent_metric = "Autofill.FillDuration.FromInteraction.WithAutofill"; + } else { + parent_metric = "Autofill.FillDuration.FromInteraction.WithoutAutofill"; + } + LogFormFillDuration(parent_metric, duration); + if (base::ContainsKey(form_types, CREDIT_CARD_FORM)) { + LogFormFillDuration(parent_metric + ".CreditCard", duration); + } + if (base::ContainsKey(form_types, ADDRESS_FORM)) { + LogFormFillDuration(parent_metric + ".Address", duration); + } + if (base::ContainsKey(form_types, PASSWORD_FORM)) { + LogFormFillDuration(parent_metric + ".Password", duration); + } + if (base::ContainsKey(form_types, UNKNOWN_FORM_TYPE)) { + LogFormFillDuration(parent_metric + ".Unknown", duration); + } } // static -void AutofillMetrics::LogFormFillDurationFromInteractionWithoutAutofill( - const base::TimeDelta& duration) { - UMA_HISTOGRAM_CUSTOM_TIMES( - "Autofill.FillDuration.FromInteraction.WithoutAutofill", duration, - base::TimeDelta::FromMilliseconds(100), base::TimeDelta::FromMinutes(10), - 50); +void AutofillMetrics::LogFormFillDuration(const std::string& metric, + const base::TimeDelta& duration) { + base::UmaHistogramCustomTimes(metric, duration, + base::TimeDelta::FromMilliseconds(100), + base::TimeDelta::FromMinutes(10), 50); } // static diff --git a/components/autofill/core/browser/autofill_metrics.h b/components/autofill/core/browser/autofill_metrics.h index 20a105d81fe79b..99194dbf8cc97b 100644 --- a/components/autofill/core/browser/autofill_metrics.h +++ b/components/autofill/core/browser/autofill_metrics.h @@ -736,17 +736,16 @@ class AutofillMetrics { static void LogFormFillDurationFromLoadWithoutAutofill( const base::TimeDelta& duration); - // This should be called when a form that has been Autofilled is submitted. - // |duration| should be the time elapsed between the initial form interaction - // and submission. - static void LogFormFillDurationFromInteractionWithAutofill( + // This should be called when a form is submitted. |duration| should be the + // time elapsed between the initial form interaction and submission. This + // metric is sliced by |form_type| and |used_autofill|. + static void LogFormFillDurationFromInteraction( + const std::set& form_types, + bool used_autofill, const base::TimeDelta& duration); - // This should be called when a fillable form that has not been Autofilled is - // submitted. |duration| should be the time elapsed between the initial form - // interaction and submission. - static void LogFormFillDurationFromInteractionWithoutAutofill( - const base::TimeDelta& duration); + static void LogFormFillDuration(const std::string& metric, + const base::TimeDelta& duration); // This should be called each time a page containing forms is loaded. static void LogIsAutofillEnabledAtPageLoad(bool enabled); diff --git a/components/autofill/core/browser/autofill_metrics_unittest.cc b/components/autofill/core/browser/autofill_metrics_unittest.cc index e556dc3cc07094..9bb357a69b43df 100644 --- a/components/autofill/core/browser/autofill_metrics_unittest.cc +++ b/components/autofill/core/browser/autofill_metrics_unittest.cc @@ -1426,8 +1426,7 @@ TEST_F(AutofillMetricsTest, UpiVirtualPaymentAddress) { 1); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.CreditCard", 0); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Password", 0); - histogram_tester.ExpectTotalCount("Autofill.UserHappiness.UnknownFormType", - 0); + histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Unknown", 0); } // Verify that when a field is annotated with the autocomplete attribute, its @@ -5000,8 +4999,7 @@ TEST_F(AutofillMetricsTest, LogUserHappinessMetric_PasswordForm) { AutofillMetrics::USER_DID_AUTOFILL, 1); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.CreditCard", 0); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Address", 0); - histogram_tester.ExpectTotalCount("Autofill.UserHappiness.UnknownFormType", - 0); + histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Unknown", 0); } { @@ -5014,8 +5012,7 @@ TEST_F(AutofillMetricsTest, LogUserHappinessMetric_PasswordForm) { AutofillMetrics::USER_DID_AUTOFILL, 1); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.CreditCard", 0); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Address", 0); - histogram_tester.ExpectTotalCount("Autofill.UserHappiness.UnknownFormType", - 0); + histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Unknown", 0); } } @@ -5026,7 +5023,7 @@ TEST_F(AutofillMetricsTest, LogUserHappinessMetric_UnknownForm) { NO_GROUP); histogram_tester.ExpectBucketCount("Autofill.UserHappiness", AutofillMetrics::USER_DID_AUTOFILL, 1); - histogram_tester.ExpectBucketCount("Autofill.UserHappiness.UnknownFormType", + histogram_tester.ExpectBucketCount("Autofill.UserHappiness.Unknown", AutofillMetrics::USER_DID_AUTOFILL, 1); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.CreditCard", 0); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Address", 0); @@ -5039,7 +5036,7 @@ TEST_F(AutofillMetricsTest, LogUserHappinessMetric_UnknownForm) { TRANSACTION); histogram_tester.ExpectBucketCount("Autofill.UserHappiness", AutofillMetrics::USER_DID_AUTOFILL, 1); - histogram_tester.ExpectBucketCount("Autofill.UserHappiness.UnknownFormType", + histogram_tester.ExpectBucketCount("Autofill.UserHappiness.Unknown", AutofillMetrics::USER_DID_AUTOFILL, 1); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.CreditCard", 0); histogram_tester.ExpectTotalCount("Autofill.UserHappiness.Address", 0); @@ -5474,7 +5471,6 @@ TEST_F(AutofillMetricsTest, FormFillDuration) { second_form.fields[1].value = ASCIIToUTF16("theking@gmail.com"); second_form.fields[2].value = ASCIIToUTF16("12345678901"); second_form.fields[3].value = ASCIIToUTF16("51512345678"); - // Expect only form load metrics to be logged if the form is submitted without // user interaction. { @@ -5623,6 +5619,212 @@ TEST_F(AutofillMetricsTest, FormFillDuration) { } } +TEST_F(AutofillMetricsTest, FormFillDurationFromInteraction_CreditCardForm) { + // Should log time duration with autofill for credit card form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {CREDIT_CARD_FORM}, true /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.CreditCard", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.CreditCard", 0); + } + + // Should log time duration without autofill for credit card form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {CREDIT_CARD_FORM}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.CreditCard", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.CreditCard", 0); + } + + // Should not log time duration for credit card form if credit card form is + // not detected. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {UNKNOWN_FORM_TYPE}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.CreditCard", 0); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.CreditCard", 0); + } +} + +TEST_F(AutofillMetricsTest, FormFillDurationFromInteraction_AddressForm) { + // Should log time duration with autofill for address form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {ADDRESS_FORM}, true /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Address", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Address", 0); + } + + // Should log time duration without autofill for address form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {ADDRESS_FORM}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Address", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Address", 0); + } + + // Should not log time duration for address form if address form is not + // detected. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {UNKNOWN_FORM_TYPE}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Address", 0); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Address", 0); + } +} + +TEST_F(AutofillMetricsTest, FormFillDurationFromInteraction_PasswordForm) { + // Should log time duration with autofill for password form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {PASSWORD_FORM}, true /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Password", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Password", 0); + } + + // Should log time duration without autofill for password form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {PASSWORD_FORM}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Password", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Password", 0); + } + + // Should not log time duration for password form if password form is not + // detected. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {UNKNOWN_FORM_TYPE}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Password", 0); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Password", 0); + } +} + +TEST_F(AutofillMetricsTest, FormFillDurationFromInteraction_UnknownForm) { + // Should log time duration with autofill for unknown form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {UNKNOWN_FORM_TYPE}, true /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Unknown", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Unknown", 0); + } + + // Should log time duration without autofill for unknown form. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {UNKNOWN_FORM_TYPE}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Unknown", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Unknown", 0); + } + + // Should not log time duration for unknown form if unknown form is not + // detected. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {ADDRESS_FORM}, false /* used_autofill */, + base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Unknown", 0); + histogram_tester.ExpectTotalCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Unknown", 0); + } +} + +TEST_F(AutofillMetricsTest, FormFillDurationFromInteraction_MultipleForms) { + // Should log time duration with autofill for all forms. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {CREDIT_CARD_FORM, ADDRESS_FORM, PASSWORD_FORM, UNKNOWN_FORM_TYPE}, + true /* used_autofill */, base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.CreditCard", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Address", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Password", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithAutofill.Unknown", + base::TimeDelta::FromMilliseconds(2000), 1); + } + + // Should log time duration without autofill for all forms. + { + base::HistogramTester histogram_tester; + AutofillMetrics::LogFormFillDurationFromInteraction( + {CREDIT_CARD_FORM, ADDRESS_FORM, PASSWORD_FORM, UNKNOWN_FORM_TYPE}, + false /* used_autofill */, base::TimeDelta::FromMilliseconds(2000)); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.CreditCard", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Address", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Password", + base::TimeDelta::FromMilliseconds(2000), 1); + histogram_tester.ExpectTimeBucketCount( + "Autofill.FillDuration.FromInteraction.WithoutAutofill.Unknown", + base::TimeDelta::FromMilliseconds(2000), 1); + } +} + // Verify that we correctly log metrics for profile action on form submission. TEST_F(AutofillMetricsTest, ProfileActionOnFormSubmitted) { base::HistogramTester histogram_tester; diff --git a/components/autofill/core/browser/form_structure.cc b/components/autofill/core/browser/form_structure.cc index abb1e1dac5cf99..9839357107438f 100644 --- a/components/autofill/core/browser/form_structure.cc +++ b/components/autofill/core/browser/form_structure.cc @@ -787,8 +787,7 @@ void FormStructure::LogQualityMetrics( DCHECK(!submission_time.is_null()); // The |load_time| might be unset, in the case that the form was - // dynamically - // added to the DOM. + // dynamically added to the DOM. if (!load_time.is_null()) { // Submission should always chronologically follow form load. DCHECK(submission_time > load_time); @@ -805,13 +804,8 @@ void FormStructure::LogQualityMetrics( // Submission should always chronologically follow interaction. DCHECK(submission_time > interaction_time); base::TimeDelta elapsed = submission_time - interaction_time; - if (did_autofill_some_possible_fields) { - AutofillMetrics::LogFormFillDurationFromInteractionWithAutofill( - elapsed); - } else { - AutofillMetrics::LogFormFillDurationFromInteractionWithoutAutofill( - elapsed); - } + AutofillMetrics::LogFormFillDurationFromInteraction( + GetFormTypes(), did_autofill_some_possible_fields, elapsed); } } if (form_interactions_ukm_logger->url() != source_url()) @@ -1434,7 +1428,7 @@ base::string16 FormStructure::FindLongestCommonPrefix( return filtered_strings[0]; } -std::set FormStructure::GetFormTypes() { +std::set FormStructure::GetFormTypes() const { std::set form_types; for (const auto& field : fields_) { form_types.insert( diff --git a/components/autofill/core/browser/form_structure.h b/components/autofill/core/browser/form_structure.h index 56a5be731bf786..d02b31beeabbe0 100644 --- a/components/autofill/core/browser/form_structure.h +++ b/components/autofill/core/browser/form_structure.h @@ -245,7 +245,7 @@ class FormStructure { FormData ToFormData() const; // Returns the possible form types. - std::set GetFormTypes(); + std::set GetFormTypes() const; bool operator==(const FormData& form) const; bool operator!=(const FormData& form) const; diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index bb78a0daff148d..3feba664ee4b70 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -95435,7 +95435,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. - + + +