diff options
15 files changed, 78 insertions, 27 deletions
diff --git a/android_webview/native/aw_autofill_client.cc b/android_webview/native/aw_autofill_client.cc index d21fe72..613d22b 100644 --- a/android_webview/native/aw_autofill_client.cc +++ b/android_webview/native/aw_autofill_client.cc @@ -185,7 +185,8 @@ void AwAutofillClient::SuggestionSelected(JNIEnv* env, jint position) { if (delegate_) { delegate_->DidAcceptSuggestion(suggestions_[position].value, - suggestions_[position].frontend_id); + suggestions_[position].frontend_id, + position); } } diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc index 3267adb..e454bc7 100644 --- a/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc +++ b/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc @@ -704,7 +704,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, FillInputFromAutofill) { view->ActivateInput(triggering_type); ASSERT_EQ(triggering_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); // All inputs should be filled. AutofillProfileWrapper wrapper(&full_profile); @@ -754,7 +754,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, FillInputFromAutofill) { value.substr(0, value.size() / 2)); view->ActivateInput(triggering_type); ASSERT_EQ(triggering_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); for (size_t i = 0; i < inputs.size(); ++i) { if (controller()->ComboboxModelForAutofillType(inputs[i].type)) @@ -799,7 +799,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, view->ActivateInput(triggering_type); ASSERT_EQ(triggering_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); // All inputs should be filled. AutofillCreditCardWrapper wrapper1(&card1); @@ -815,7 +815,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, value.substr(0, value.size() / 2)); view->ActivateInput(triggering_type); ASSERT_EQ(triggering_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); AutofillCreditCardWrapper wrapper2(&card2); for (size_t i = 0; i < inputs.size(); ++i) { @@ -843,7 +843,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, view->ActivateInput(billing_triggering_type); ASSERT_EQ(billing_triggering_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); for (size_t i = 0; i < inputs.size(); ++i) { const ServerFieldType type = inputs[i].type; @@ -1679,7 +1679,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, view->SetTextContentsOfInput(input_type, name.substr(0, name.size() / 2)); view->ActivateInput(input_type); ASSERT_EQ(input_type, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); EXPECT_EQ(ASCIIToUTF16("Germany"), view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); @@ -1706,7 +1706,7 @@ IN_PROC_BROWSER_TEST_F(AutofillDialogControllerTest, view->SetTextContentsOfInput(NAME_FULL, name.substr(0, name.size() / 2)); view->ActivateInput(NAME_FULL); ASSERT_EQ(NAME_FULL, controller()->popup_input_type()); - controller()->DidAcceptSuggestion(base::string16(), 0); + controller()->DidAcceptSuggestion(base::string16(), 0, 1); EXPECT_EQ(ASCIIToUTF16("France"), view->GetTextContentsOfInput(ADDRESS_BILLING_COUNTRY)); diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc index 737a99d..d700e8b 100644 --- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc +++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc @@ -2383,7 +2383,8 @@ void AutofillDialogControllerImpl::DidSelectSuggestion( void AutofillDialogControllerImpl::DidAcceptSuggestion( const base::string16& value, - int identifier) { + int identifier, + int position) { DCHECK_NE(UNKNOWN_TYPE, popup_input_type_); // Because |HidePopup()| can be called from |UpdateSection()|, remember the // type of the input for later here. diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.h b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.h index 80757b7..9c5c740 100644 --- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.h +++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.h @@ -168,7 +168,8 @@ class AutofillDialogControllerImpl void DidSelectSuggestion(const base::string16& value, int identifier) override; void DidAcceptSuggestion(const base::string16& value, - int identifier) override; + int identifier, + int position) override; bool GetDeletionConfirmationText(const base::string16& value, int identifier, base::string16* title, diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc index 2ad6cbe..d9d2907 100644 --- a/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc +++ b/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc @@ -343,7 +343,8 @@ void AutofillPopupControllerImpl::SelectionCleared() { void AutofillPopupControllerImpl::AcceptSuggestion(size_t index) { const autofill::Suggestion& suggestion = suggestions_[index]; - delegate_->DidAcceptSuggestion(suggestion.value, suggestion.frontend_id); + delegate_->DidAcceptSuggestion(suggestion.value, suggestion.frontend_id, + index); } int AutofillPopupControllerImpl::GetIconResourceID( diff --git a/components/autofill/core/browser/autofill_external_delegate.cc b/components/autofill/core/browser/autofill_external_delegate.cc index 5545f99..eda826c 100644 --- a/components/autofill/core/browser/autofill_external_delegate.cc +++ b/components/autofill/core/browser/autofill_external_delegate.cc @@ -199,7 +199,8 @@ void AutofillExternalDelegate::DidSelectSuggestion( } void AutofillExternalDelegate::DidAcceptSuggestion(const base::string16& value, - int identifier) { + int identifier, + int position) { if (identifier == POPUP_ITEM_ID_AUTOFILL_OPTIONS) { // User selected 'Autofill Options'. manager_->ShowAutofillSettings(); @@ -254,6 +255,9 @@ void AutofillExternalDelegate::DidAcceptSuggestion(const base::string16& value, manager_->client()->ScanCreditCard(base::Bind( &AutofillExternalDelegate::OnCreditCardScanned, GetWeakPtr())); } else { + if (identifier > 0) // Denotes an Autofill suggestion. + AutofillMetrics::LogSuggestionAcceptedIndex(position); + FillAutofillFormData(identifier, false); } diff --git a/components/autofill/core/browser/autofill_external_delegate.h b/components/autofill/core/browser/autofill_external_delegate.h index 3803e24..7f09d74 100644 --- a/components/autofill/core/browser/autofill_external_delegate.h +++ b/components/autofill/core/browser/autofill_external_delegate.h @@ -41,7 +41,8 @@ class AutofillExternalDelegate : public AutofillPopupDelegate { void DidSelectSuggestion(const base::string16& value, int identifier) override; void DidAcceptSuggestion(const base::string16& value, - int identifier) override; + int identifier, + int position) override; bool GetDeletionConfirmationText(const base::string16& value, int identifier, base::string16* title, diff --git a/components/autofill/core/browser/autofill_external_delegate_unittest.cc b/components/autofill/core/browser/autofill_external_delegate_unittest.cc index e9bef06..18939bb 100644 --- a/components/autofill/core/browser/autofill_external_delegate_unittest.cc +++ b/components/autofill/core/browser/autofill_external_delegate_unittest.cc @@ -181,7 +181,8 @@ TEST_F(AutofillExternalDelegateUnitTest, TestExternalDelegateVirtualCalls) { // This should trigger a call to hide the popup since we've selected an // option. external_delegate_->DidAcceptSuggestion(autofill_item[0].value, - autofill_item[0].frontend_id); + autofill_item[0].frontend_id, + 0); } // Test that data list elements for a node will appear in the Autofill popup. @@ -325,7 +326,7 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateInvalidUniqueId) { // Ensure it doesn't try to fill the form in with the negative id. EXPECT_CALL(autofill_client_, HideAutofillPopup()); EXPECT_CALL(*autofill_manager_, FillOrPreviewForm(_, _, _, _, _)).Times(0); - external_delegate_->DidAcceptSuggestion(base::string16(), -1); + external_delegate_->DidAcceptSuggestion(base::string16(), -1, 0); } // Test that the ClearPreview call is only sent if the form was being previewed @@ -363,13 +364,32 @@ TEST_F(AutofillExternalDelegateUnitTest, // Test that the driver is directed to accept the data list after being notified // that the user accepted the data list suggestion. -TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateAcceptSuggestion) { +TEST_F(AutofillExternalDelegateUnitTest, + ExternalDelegateAcceptDatalistSuggestion) { EXPECT_CALL(autofill_client_, HideAutofillPopup()); base::string16 dummy_string(ASCIIToUTF16("baz qux")); EXPECT_CALL(*autofill_driver_, RendererShouldAcceptDataListSuggestion(dummy_string)); external_delegate_->DidAcceptSuggestion(dummy_string, - POPUP_ITEM_ID_DATALIST_ENTRY); + POPUP_ITEM_ID_DATALIST_ENTRY, + 0); +} + +// Test that an accepted autofill suggestion will fill the form and log the +// proper metric. +TEST_F(AutofillExternalDelegateUnitTest, + ExternalDelegateAcceptAutofillSuggestion) { + EXPECT_CALL(autofill_client_, HideAutofillPopup()); + base::string16 dummy_string(ASCIIToUTF16("John Legend")); + EXPECT_CALL(*autofill_manager_, + FillOrPreviewForm( + AutofillDriver::FORM_DATA_ACTION_FILL, _, _, _, + kAutofillProfileId)); + base::HistogramTester histogram; + external_delegate_->DidAcceptSuggestion(dummy_string, + kAutofillProfileId, + 2); // Row 2 + histogram.ExpectUniqueSample("Autofill.SuggestionAcceptedIndex", 2, 1); } // Test that the driver is directed to clear the form after being notified that @@ -379,7 +399,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateClearForm) { EXPECT_CALL(*autofill_driver_, RendererShouldClearFilledForm()); external_delegate_->DidAcceptSuggestion(base::string16(), - POPUP_ITEM_ID_CLEAR_FORM); + POPUP_ITEM_ID_CLEAR_FORM, + 0); } // Test that autofill client will scan a credit card after use accepted the @@ -388,7 +409,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ScanCreditCardMenuItem) { EXPECT_CALL(autofill_client_, ScanCreditCard(_)); EXPECT_CALL(autofill_client_, HideAutofillPopup()); external_delegate_->DidAcceptSuggestion(base::string16(), - POPUP_ITEM_ID_SCAN_CREDIT_CARD); + POPUP_ITEM_ID_SCAN_CREDIT_CARD, + 0); } TEST_F(AutofillExternalDelegateUnitTest, ScanCreditCardPromptMetricsTest) { @@ -411,7 +433,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ScanCreditCardPromptMetricsTest) { IssueOnQuery(kQueryId); IssueOnSuggestionsReturned(); external_delegate_->DidAcceptSuggestion(base::string16(), - POPUP_ITEM_ID_SCAN_CREDIT_CARD); + POPUP_ITEM_ID_SCAN_CREDIT_CARD, + 0); histogram.ExpectBucketCount("Autofill.ScanCreditCardPrompt", AutofillMetrics::SCAN_CARD_ITEM_SHOWN, 1); histogram.ExpectBucketCount("Autofill.ScanCreditCardPrompt", @@ -428,7 +451,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ScanCreditCardPromptMetricsTest) { IssueOnQuery(kQueryId); IssueOnSuggestionsReturned(); external_delegate_->DidAcceptSuggestion(base::string16(), - POPUP_ITEM_ID_CLEAR_FORM); + POPUP_ITEM_ID_CLEAR_FORM, + 0); histogram.ExpectBucketCount("Autofill.ScanCreditCardPrompt", AutofillMetrics::SCAN_CARD_ITEM_SHOWN, 1); histogram.ExpectBucketCount("Autofill.ScanCreditCardPrompt", @@ -445,7 +469,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ScanCreditCardPromptMetricsTest) { IssueOnQuery(kQueryId); IssueOnSuggestionsReturned(); external_delegate_->DidAcceptSuggestion(base::string16(), - POPUP_ITEM_ID_CLEAR_FORM); + POPUP_ITEM_ID_CLEAR_FORM, + 0); histogram.ExpectTotalCount("Autofill.ScanCreditCardPrompt", 0); } } @@ -490,7 +515,8 @@ TEST_F(AutofillExternalDelegateUnitTest, ExternalDelegateFillFieldWithValue) { EXPECT_CALL(*autofill_driver_, RendererShouldFillFieldWithValue(dummy_string)); external_delegate_->DidAcceptSuggestion(dummy_string, - POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY); + POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY, + 0); } } // namespace autofill diff --git a/components/autofill/core/browser/autofill_metrics.cc b/components/autofill/core/browser/autofill_metrics.cc index b761247..fae07e8 100644 --- a/components/autofill/core/browser/autofill_metrics.cc +++ b/components/autofill/core/browser/autofill_metrics.cc @@ -606,6 +606,11 @@ void AutofillMetrics::LogAddressSuggestionsCount(size_t num_suggestions) { UMA_HISTOGRAM_COUNTS("Autofill.AddressSuggestionsCount", num_suggestions); } +// static +void AutofillMetrics::LogSuggestionAcceptedIndex(int index) { + UMA_HISTOGRAM_SPARSE_SLOWLY("Autofill.SuggestionAcceptedIndex", index); +} + void AutofillMetrics::LogPasswordFormQueryVolume( PasswordFormQueryVolumeMetric metric) { UMA_HISTOGRAM_ENUMERATION("Autofill.PasswordFormQueryVolume", metric, diff --git a/components/autofill/core/browser/autofill_metrics.h b/components/autofill/core/browser/autofill_metrics.h index 3d67990..c7420e4 100644 --- a/components/autofill/core/browser/autofill_metrics.h +++ b/components/autofill/core/browser/autofill_metrics.h @@ -559,6 +559,9 @@ class AutofillMetrics { // form. static void LogAddressSuggestionsCount(size_t num_suggestions); + // Log the index of the selected suggestion in the Autofill popup. + static void LogSuggestionAcceptedIndex(int index); + // Log password form query: current and if one-to-two fields password forms // were allowed. static void LogPasswordFormQueryVolume(PasswordFormQueryVolumeMetric metric); diff --git a/components/autofill/core/browser/autofill_popup_delegate.h b/components/autofill/core/browser/autofill_popup_delegate.h index 8c77edb..99a6675 100644 --- a/components/autofill/core/browser/autofill_popup_delegate.h +++ b/components/autofill/core/browser/autofill_popup_delegate.h @@ -26,7 +26,8 @@ class AutofillPopupDelegate { // Inform the delegate that a row in the popup has been chosen. virtual void DidAcceptSuggestion(const base::string16& value, - int identifier) = 0; + int identifier, + int position) = 0; // Returns whether the given value can be deleted, and if true, // fills out |title| and |body|. diff --git a/components/password_manager/core/browser/password_autofill_manager.cc b/components/password_manager/core/browser/password_autofill_manager.cc index a9b6644..c8d8378 100644 --- a/components/password_manager/core/browser/password_autofill_manager.cc +++ b/components/password_manager/core/browser/password_autofill_manager.cc @@ -212,7 +212,8 @@ void PasswordAutofillManager::DidSelectSuggestion(const base::string16& value, } void PasswordAutofillManager::DidAcceptSuggestion(const base::string16& value, - int identifier) { + int identifier, + int position) { bool success = FillSuggestion(form_data_key_, value); DCHECK(success); autofill_client_->HideAutofillPopup(); diff --git a/components/password_manager/core/browser/password_autofill_manager.h b/components/password_manager/core/browser/password_autofill_manager.h index 96e998f..0b52172 100644 --- a/components/password_manager/core/browser/password_autofill_manager.h +++ b/components/password_manager/core/browser/password_autofill_manager.h @@ -34,7 +34,8 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate { void DidSelectSuggestion(const base::string16& value, int identifier) override; void DidAcceptSuggestion(const base::string16& value, - int identifier) override; + int identifier, + int position) override; bool GetDeletionConfirmationText(const base::string16& value, int identifier, base::string16* title, diff --git a/components/password_manager/core/browser/password_autofill_manager_unittest.cc b/components/password_manager/core/browser/password_autofill_manager_unittest.cc index d811161..7dd87dc 100644 --- a/components/password_manager/core/browser/password_autofill_manager_unittest.cc +++ b/components/password_manager/core/browser/password_autofill_manager_unittest.cc @@ -199,7 +199,7 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) { // Accepting a suggestion should trigger a call to hide the popup. EXPECT_CALL(*autofill_client, HideAutofillPopup()); password_autofill_manager_->DidAcceptSuggestion( - test_username_, autofill::POPUP_ITEM_ID_PASSWORD_ENTRY); + test_username_, autofill::POPUP_ITEM_ID_PASSWORD_ENTRY, 1); } // Test that OnShowPasswordSuggestions correctly matches the given FormFieldData diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index e3d2824..d266636 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -2341,6 +2341,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. </summary> </histogram> +<histogram name="Autofill.SuggestionAcceptedIndex"> + <owner>mathp@chromium.org</owner> + <summary>The index of the accepted suggestion in the Autofill popup.</summary> +</histogram> + <histogram name="Autofill.UnmaskPrompt.Duration" units="ms"> <owner>waltercacau@chromium.org</owner> <summary> |