Commit a55316f4 authored by Dominic Battre's avatar Dominic Battre Committed by Commit Bot

Foundation for recording UKMs for the Password Manager

Bug: 732846
Change-Id: I12291bfc4484a11cd068a50fd3603525d6577c18
Reviewed-on: https://chromium-review.googlesource.com/533217
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: default avatarRobert Kaplow <rkaplow@chromium.org>
Reviewed-by: default avatarVaclav Brozek <vabr@chromium.org>
Reviewed-by: default avatarBalazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481910}
parent 2173679a
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include "base/metrics/field_trial.h" #include "base/metrics/field_trial.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "build/build_config.h" #include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browsing_data/browsing_data_helper.h" #include "chrome/browser/browsing_data/browsing_data_helper.h"
#include "chrome/browser/metrics/chrome_metrics_service_accessor.h" #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
#include "chrome/browser/password_manager/password_store_factory.h" #include "chrome/browser/password_manager/password_store_factory.h"
...@@ -51,6 +52,7 @@ ...@@ -51,6 +52,7 @@
#include "components/prefs/pref_service.h" #include "components/prefs/pref_service.h"
#include "components/sessions/content/content_record_password_state.h" #include "components/sessions/content/content_record_password_state.h"
#include "components/signin/core/browser/signin_manager.h" #include "components/signin/core/browser/signin_manager.h"
#include "components/ukm/public/ukm_recorder.h"
#include "components/version_info/version_info.h" #include "components/version_info/version_info.h"
#include "content/public/browser/browser_thread.h" #include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_security_policy.h" #include "content/public/browser/child_process_security_policy.h"
...@@ -436,6 +438,22 @@ void ChromePasswordManagerClient::CheckProtectedPasswordEntry( ...@@ -436,6 +438,22 @@ void ChromePasswordManagerClient::CheckProtectedPasswordEntry(
} }
#endif #endif
ukm::UkmRecorder* ChromePasswordManagerClient::GetUkmRecorder() {
return g_browser_process->ukm_recorder();
}
ukm::SourceId ChromePasswordManagerClient::GetUkmSourceId() {
// TODO(crbug.com/732846): The UKM Source should be recycled (e.g. from the
// web contents), once the UKM framework provides a mechanism for that.
if (!ukm_source_id_) {
ukm_source_id_ = ukm::UkmRecorder::GetNewSourceID();
ukm::UkmRecorder* ukm_recorder = GetUkmRecorder();
if (ukm_recorder)
ukm_recorder->UpdateSourceURL(*ukm_source_id_, GetMainFrameURL());
}
return *ukm_source_id_;
}
// TODO(crbug.com/706392): Fix password reuse detection for Android. // TODO(crbug.com/706392): Fix password reuse detection for Android.
#if !defined(OS_ANDROID) #if !defined(OS_ANDROID)
void ChromePasswordManagerClient::DidFinishNavigation( void ChromePasswordManagerClient::DidFinishNavigation(
...@@ -443,6 +461,9 @@ void ChromePasswordManagerClient::DidFinishNavigation( ...@@ -443,6 +461,9 @@ void ChromePasswordManagerClient::DidFinishNavigation(
if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted()) if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
return; return;
if (!navigation_handle->IsSameDocument())
ukm_source_id_.reset();
password_reuse_detection_manager_.DidNavigateMainFrame(GetMainFrameURL()); password_reuse_detection_manager_.DidNavigateMainFrame(GetMainFrameURL());
// After some navigations RenderViewHost persists and just adding the observer // After some navigations RenderViewHost persists and just adding the observer
// will cause multiple call of OnInputEvent. Since Widget API doesn't allow to // will cause multiple call of OnInputEvent. Since Widget API doesn't allow to
......
...@@ -10,6 +10,7 @@ ...@@ -10,6 +10,7 @@
#include "base/compiler_specific.h" #include "base/compiler_specific.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/optional.h"
#include "components/autofill/content/common/autofill_driver.mojom.h" #include "components/autofill/content/common/autofill_driver.mojom.h"
#include "components/password_manager/content/browser/content_password_manager_driver_factory.h" #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
#include "components/password_manager/content/browser/credential_manager_impl.h" #include "components/password_manager/content/browser/credential_manager_impl.h"
...@@ -115,6 +116,9 @@ class ChromePasswordManagerClient ...@@ -115,6 +116,9 @@ class ChromePasswordManagerClient
bool password_field_exists) override; bool password_field_exists) override;
#endif #endif
ukm::UkmRecorder* GetUkmRecorder() override;
ukm::SourceId GetUkmSourceId() override;
static void CreateForWebContentsWithAutofillClient( static void CreateForWebContentsWithAutofillClient(
content::WebContents* contents, content::WebContents* contents,
autofill::AutofillClient* autofill_client); autofill::AutofillClient* autofill_client);
...@@ -215,6 +219,10 @@ class ChromePasswordManagerClient ...@@ -215,6 +219,10 @@ class ChromePasswordManagerClient
// form for potential use during 'NotifySuccessfulLoginWithExistingPassword'. // form for potential use during 'NotifySuccessfulLoginWithExistingPassword'.
std::unique_ptr<autofill::PasswordForm> possible_auto_sign_in_; std::unique_ptr<autofill::PasswordForm> possible_auto_sign_in_;
// If set, this stores a ukm::SourceId that is bound to the last committed
// navigation of the tab owning this ChromePasswordManagerClient.
base::Optional<ukm::SourceId> ukm_source_id_;
DISALLOW_COPY_AND_ASSIGN(ChromePasswordManagerClient); DISALLOW_COPY_AND_ASSIGN(ChromePasswordManagerClient);
}; };
......
...@@ -167,6 +167,7 @@ static_library("browser") { ...@@ -167,6 +167,7 @@ static_library("browser") {
"//components/security_state/core", "//components/security_state/core",
"//components/strings", "//components/strings",
"//components/sync", "//components/sync",
"//components/ukm",
"//components/url_formatter", "//components/url_formatter",
"//components/variations", "//components/variations",
"//components/webdata/common", "//components/webdata/common",
...@@ -236,6 +237,7 @@ static_library("test_support") { ...@@ -236,6 +237,7 @@ static_library("test_support") {
public_deps = [ public_deps = [
":browser", ":browser",
"//components/ukm",
"//testing/gmock", "//testing/gmock",
"//url:url", "//url:url",
] ]
...@@ -359,6 +361,7 @@ source_set("unit_tests") { ...@@ -359,6 +361,7 @@ source_set("unit_tests") {
"//components/strings", "//components/strings",
"//components/sync:test_support_driver", "//components/sync:test_support_driver",
"//components/sync:test_support_model", "//components/sync:test_support_model",
"//components/ukm:test_support",
"//components/variations", "//components/variations",
"//net:test_support", "//net:test_support",
"//sql:test_support", "//sql:test_support",
......
...@@ -5,6 +5,7 @@ include_rules = [ ...@@ -5,6 +5,7 @@ include_rules = [
"+components/security_state", "+components/security_state",
"+components/sync/base", "+components/sync/base",
"+components/sync/driver", "+components/sync/driver",
"+components/ukm",
"+components/url_formatter", "+components/url_formatter",
"+components/variations", "+components/variations",
"+components/webdata/common", "+components/webdata/common",
......
...@@ -239,7 +239,10 @@ PasswordFormManager::PasswordFormManager( ...@@ -239,7 +239,10 @@ PasswordFormManager::PasswordFormManager(
true /* should_query_suppressed_https_forms */)), true /* should_query_suppressed_https_forms */)),
form_fetcher_(form_fetcher ? form_fetcher : owned_form_fetcher_.get()), form_fetcher_(form_fetcher ? form_fetcher : owned_form_fetcher_.get()),
is_main_frame_secure_(client->IsMainFrameSecure()), is_main_frame_secure_(client->IsMainFrameSecure()),
metrics_recorder_(client->IsMainFrameSecure()) { metrics_recorder_(client->IsMainFrameSecure(),
PasswordFormMetricsRecorder::CreateUkmEntryBuilder(
client->GetUkmRecorder(),
client->GetUkmSourceId())) {
if (owned_form_fetcher_) if (owned_form_fetcher_)
owned_form_fetcher_->Fetch(); owned_form_fetcher_->Fetch();
DCHECK_EQ(observed_form.scheme == PasswordForm::SCHEME_HTML, DCHECK_EQ(observed_form.scheme == PasswordForm::SCHEME_HTML,
......
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
#include "base/logging.h" #include "base/logging.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "base/metrics/user_metrics.h" #include "base/metrics/user_metrics.h"
#include "base/numerics/safe_conversions.h"
#include "components/password_manager/core/browser/form_fetcher.h" #include "components/password_manager/core/browser/form_fetcher.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h"
...@@ -17,8 +18,10 @@ using autofill::PasswordForm; ...@@ -17,8 +18,10 @@ using autofill::PasswordForm;
namespace password_manager { namespace password_manager {
PasswordFormMetricsRecorder::PasswordFormMetricsRecorder( PasswordFormMetricsRecorder::PasswordFormMetricsRecorder(
bool is_main_frame_secure) bool is_main_frame_secure,
: is_main_frame_secure_(is_main_frame_secure) {} std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder)
: is_main_frame_secure_(is_main_frame_secure),
ukm_entry_builder_(std::move(ukm_entry_builder)) {}
PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() { PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() {
UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTakenV3", GetActionsTaken(), UMA_HISTOGRAM_ENUMERATION("PasswordManager.ActionsTakenV3", GetActionsTaken(),
...@@ -40,6 +43,7 @@ PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() { ...@@ -40,6 +43,7 @@ PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() {
metrics_util::LogPasswordGenerationAvailableSubmissionEvent( metrics_util::LogPasswordGenerationAvailableSubmissionEvent(
metrics_util::PASSWORD_NOT_SUBMITTED); metrics_util::PASSWORD_NOT_SUBMITTED);
} }
RecordUkmMetric(internal::kUkmSubmissionObserved, 0 /*false*/);
} }
if (submitted_form_type_ != kSubmittedFormTypeUnspecified) { if (submitted_form_type_ != kSubmittedFormTypeUnspecified) {
...@@ -49,9 +53,21 @@ PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() { ...@@ -49,9 +53,21 @@ PasswordFormMetricsRecorder::~PasswordFormMetricsRecorder() {
UMA_HISTOGRAM_ENUMERATION("PasswordManager.SubmittedNonSecureFormType", UMA_HISTOGRAM_ENUMERATION("PasswordManager.SubmittedNonSecureFormType",
submitted_form_type_, kSubmittedFormTypeMax); submitted_form_type_, kSubmittedFormTypeMax);
} }
RecordUkmMetric(internal::kUkmSubmissionFormType, submitted_form_type_);
} }
} }
// static
std::unique_ptr<ukm::UkmEntryBuilder>
PasswordFormMetricsRecorder::CreateUkmEntryBuilder(
ukm::UkmRecorder* ukm_recorder,
ukm::SourceId source_id) {
if (!ukm_recorder)
return nullptr;
return ukm_recorder->GetEntryBuilder(source_id, "PasswordForm");
}
void PasswordFormMetricsRecorder::MarkGenerationAvailable() { void PasswordFormMetricsRecorder::MarkGenerationAvailable() {
generation_available_ = true; generation_available_ = true;
} }
...@@ -96,6 +112,8 @@ void PasswordFormMetricsRecorder::LogSubmitPassed() { ...@@ -96,6 +112,8 @@ void PasswordFormMetricsRecorder::LogSubmitPassed() {
} }
} }
base::RecordAction(base::UserMetricsAction("PasswordManager_LoginPassed")); base::RecordAction(base::UserMetricsAction("PasswordManager_LoginPassed"));
RecordUkmMetric(internal::kUkmSubmissionObserved, 1 /*true*/);
RecordUkmMetric(internal::kUkmSubmissionResult, kSubmitResultPassed);
submit_result_ = kSubmitResultPassed; submit_result_ = kSubmitResultPassed;
} }
...@@ -108,6 +126,8 @@ void PasswordFormMetricsRecorder::LogSubmitFailed() { ...@@ -108,6 +126,8 @@ void PasswordFormMetricsRecorder::LogSubmitFailed() {
metrics_util::PASSWORD_SUBMISSION_FAILED); metrics_util::PASSWORD_SUBMISSION_FAILED);
} }
base::RecordAction(base::UserMetricsAction("PasswordManager_LoginFailed")); base::RecordAction(base::UserMetricsAction("PasswordManager_LoginFailed"));
RecordUkmMetric(internal::kUkmSubmissionObserved, 1 /*true*/);
RecordUkmMetric(internal::kUkmSubmissionResult, kSubmitResultFailed);
submit_result_ = kSubmitResultFailed; submit_result_ = kSubmitResultFailed;
} }
...@@ -218,4 +238,10 @@ int PasswordFormMetricsRecorder::GetHistogramSampleForSuppressedAccounts( ...@@ -218,4 +238,10 @@ int PasswordFormMetricsRecorder::GetHistogramSampleForSuppressedAccounts(
return mixed_base_encoding; return mixed_base_encoding;
} }
void PasswordFormMetricsRecorder::RecordUkmMetric(const char* metric_name,
int64_t value) {
if (ukm_entry_builder_)
ukm_entry_builder_->AddMetric(metric_name, value);
}
} // namespace password_manager } // namespace password_manager
...@@ -5,14 +5,38 @@ ...@@ -5,14 +5,38 @@
#ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_METRICS_RECORDER_H_ #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_METRICS_RECORDER_H_
#define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_METRICS_RECORDER_H_ #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PASSWORD_FORM_METRICS_RECORDER_H_
#include <stdint.h>
#include <memory>
#include <vector> #include <vector>
#include "base/macros.h" #include "base/macros.h"
#include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/password_form.h"
#include "components/password_manager/core/browser/password_form_user_action.h" #include "components/password_manager/core/browser/password_form_user_action.h"
#include "components/ukm/public/ukm_recorder.h"
namespace password_manager { namespace password_manager {
// Internal namespace is intended for component wide access only.
namespace internal {
// UKM Metric names. Exposed in internal namespace for unittesting.
// This metric records whether a submission of a password form has been
// observed. The values 0 and 1 correspond to false and true respectively.
constexpr char kUkmSubmissionObserved[] = "Submission.Observed";
// This metric records the outcome of a password form submission. The values are
// numbered according to PasswordFormMetricsRecorder::SubmitResult.
// Note that no metric is recorded for kSubmitResultNotSubmitted.
constexpr char kUkmSubmissionResult[] = "Submission.SubmissionResult";
// This metric records the classification of a form at submission time. The
// values correspond to PasswordFormMetricsRecorder::SubmittedFormType.
// Note that no metric is recorded for kSubmittedFormTypeUnspecified.
constexpr char kUkmSubmissionFormType[] = "Submission.SubmittedFormType";
} // namespace internal
class FormFetcher; class FormFetcher;
// The pupose of this class is to record various types of metrics about the // The pupose of this class is to record various types of metrics about the
...@@ -20,9 +44,21 @@ class FormFetcher; ...@@ -20,9 +44,21 @@ class FormFetcher;
// page. // page.
class PasswordFormMetricsRecorder { class PasswordFormMetricsRecorder {
public: public:
explicit PasswordFormMetricsRecorder(bool is_main_frame_secure); // |ukm_entry_builder| is the destination into which UKM metrics are recorded.
// It may be nullptr, in which case no UKM metrics are recorded. This should
// be created via the static CreateUkmEntryBuilder() method of this class.
PasswordFormMetricsRecorder(
bool is_main_frame_secure,
std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder);
~PasswordFormMetricsRecorder(); ~PasswordFormMetricsRecorder();
// Creates a UkmEntryBuilder that can be used to record metrics into the event
// "PasswordForm". |source_id| should be bound the the correct URL in the
// |ukm_recorder| when this function is called.
static std::unique_ptr<ukm::UkmEntryBuilder> CreateUkmEntryBuilder(
ukm::UkmRecorder* ukm_recorder,
ukm::SourceId source_id);
// ManagerAction - What does the PasswordFormManager do with this form? Either // ManagerAction - What does the PasswordFormManager do with this form? Either
// it fills it, or it doesn't. If it doesn't fill it, that's either // it fills it, or it doesn't. If it doesn't fill it, that's either
// because it has no match or it is disabled via the AUTOCOMPLETE=off // because it has no match or it is disabled via the AUTOCOMPLETE=off
...@@ -159,6 +195,9 @@ class PasswordFormMetricsRecorder { ...@@ -159,6 +195,9 @@ class PasswordFormMetricsRecorder {
autofill::PasswordForm::Type manual_or_generated, autofill::PasswordForm::Type manual_or_generated,
const autofill::PasswordForm& pending_credentials) const; const autofill::PasswordForm& pending_credentials) const;
// Records a metric into |ukm_entry_builder_| if it is not nullptr.
void RecordUkmMetric(const char* metric_name, int64_t value);
// True if the main frame's visible URL, at the time this PasswordFormManager // True if the main frame's visible URL, at the time this PasswordFormManager
// was created, is secure. // was created, is secure.
const bool is_main_frame_secure_; const bool is_main_frame_secure_;
...@@ -181,6 +220,10 @@ class PasswordFormMetricsRecorder { ...@@ -181,6 +220,10 @@ class PasswordFormMetricsRecorder {
// data the user has entered. // data the user has entered.
SubmittedFormType submitted_form_type_ = kSubmittedFormTypeUnspecified; SubmittedFormType submitted_form_type_ = kSubmittedFormTypeUnspecified;
// Records URL keyed metrics (UKMs) and submits them on its destruction. May
// be a nullptr in which case no recording is expected.
std::unique_ptr<ukm::UkmEntryBuilder> ukm_entry_builder_;
DISALLOW_COPY_AND_ASSIGN(PasswordFormMetricsRecorder); DISALLOW_COPY_AND_ASSIGN(PasswordFormMetricsRecorder);
}; };
......
...@@ -5,14 +5,53 @@ ...@@ -5,14 +5,53 @@
#include "components/password_manager/core/browser/password_form_metrics_recorder.h" #include "components/password_manager/core/browser/password_form_metrics_recorder.h"
#include "base/logging.h" #include "base/logging.h"
#include "base/metrics/metrics_hashes.h"
#include "base/test/histogram_tester.h" #include "base/test/histogram_tester.h"
#include "base/test/user_action_tester.h" #include "base/test/user_action_tester.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/ukm/public/ukm_recorder.h"
#include "components/ukm/test_ukm_recorder.h"
#include "components/ukm/ukm_source.h"
#include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
namespace {
constexpr char kTestUrl[] = "https://www.example.com/";
}
namespace password_manager { namespace password_manager {
// Create a UkmEntryBuilder with a SourceId that is initialized for kTestUrl.
std::unique_ptr<ukm::UkmEntryBuilder> CreateUkmEntryBuilder(
ukm::TestUkmRecorder* test_ukm_recorder) {
ukm::SourceId source_id = test_ukm_recorder->GetNewSourceID();
static_cast<ukm::UkmRecorder*>(test_ukm_recorder)
->UpdateSourceURL(source_id, GURL(kTestUrl));
return PasswordFormMetricsRecorder::CreateUkmEntryBuilder(test_ukm_recorder,
source_id);
}
// Verifies that the metric |metric_name| was recorded with value |value| in the
// single entry of |test_ukm_recorder_| exactly |expected_count| times.
void ExpectUkmValueCount(ukm::TestUkmRecorder* test_ukm_recorder,
const char* metric_name,
int64_t value,
int64_t expected_count) {
const ukm::UkmSource* source = test_ukm_recorder->GetSourceForUrl(kTestUrl);
ASSERT_NE(nullptr, source);
ASSERT_EQ(1U, test_ukm_recorder->entries_count());
const ukm::mojom::UkmEntry* entry = test_ukm_recorder->GetEntry(0);
int occurrences = 0;
for (const ukm::mojom::UkmMetricPtr& metric : entry->metrics) {
if (metric->metric_hash == base::HashMetricName(metric_name) &&
metric->value == value)
++occurrences;
}
EXPECT_EQ(expected_count, occurrences) << metric_name << ": " << value;
}
// Test the metrics recorded around password generation and the user's // Test the metrics recorded around password generation and the user's
// interaction with the offer to generate passwords. // interaction with the offer to generate passwords.
TEST(PasswordFormMetricsRecorder, Generation) { TEST(PasswordFormMetricsRecorder, Generation) {
...@@ -36,15 +75,18 @@ TEST(PasswordFormMetricsRecorder, Generation) { ...@@ -36,15 +75,18 @@ TEST(PasswordFormMetricsRecorder, Generation) {
SCOPED_TRACE(testing::Message() SCOPED_TRACE(testing::Message()
<< "generation_available=" << test.generation_available << "generation_available=" << test.generation_available
<< ", has_generated_password=" << test.has_generated_password << ", has_generated_password=" << test.has_generated_password
<< ", submission" << test.submission); << ", submission=" << test.submission);
ukm::TestUkmRecorder test_ukm_recorder;
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
base::UserActionTester user_action_tester; base::UserActionTester user_action_tester;
// Use a scoped PasswordFromMetricsRecorder because some metrics are recored // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
// on destruction. // on destruction.
{ {
PasswordFormMetricsRecorder recorder(/*is_main_frame_secure*/ true); PasswordFormMetricsRecorder recorder(
/*is_main_frame_secure*/ true,
CreateUkmEntryBuilder(&test_ukm_recorder));
if (test.generation_available) if (test.generation_available)
recorder.MarkGenerationAvailable(); recorder.MarkGenerationAvailable();
recorder.SetHasGeneratedPassword(test.has_generated_password); recorder.SetHasGeneratedPassword(test.has_generated_password);
...@@ -63,15 +105,31 @@ TEST(PasswordFormMetricsRecorder, Generation) { ...@@ -63,15 +105,31 @@ TEST(PasswordFormMetricsRecorder, Generation) {
} }
} }
EXPECT_EQ( ExpectUkmValueCount(
&test_ukm_recorder, internal::kUkmSubmissionObserved,
test.submission !=
PasswordFormMetricsRecorder::kSubmitResultNotSubmitted
? 1
: 0,
1);
int expected_login_failed =
test.submission == PasswordFormMetricsRecorder::kSubmitResultFailed ? 1 test.submission == PasswordFormMetricsRecorder::kSubmitResultFailed ? 1
: 0, : 0;
user_action_tester.GetActionCount("PasswordManager_LoginFailed")); EXPECT_EQ(expected_login_failed,
user_action_tester.GetActionCount("PasswordManager_LoginFailed"));
ExpectUkmValueCount(&test_ukm_recorder, internal::kUkmSubmissionResult,
PasswordFormMetricsRecorder::kSubmitResultFailed,
expected_login_failed);
EXPECT_EQ( int expected_login_passed =
test.submission == PasswordFormMetricsRecorder::kSubmitResultPassed ? 1 test.submission == PasswordFormMetricsRecorder::kSubmitResultPassed ? 1
: 0, : 0;
user_action_tester.GetActionCount("PasswordManager_LoginPassed")); EXPECT_EQ(expected_login_passed,
user_action_tester.GetActionCount("PasswordManager_LoginPassed"));
ExpectUkmValueCount(&test_ukm_recorder, internal::kUkmSubmissionResult,
PasswordFormMetricsRecorder::kSubmitResultPassed,
expected_login_passed);
if (test.has_generated_password) { if (test.has_generated_password) {
switch (test.submission) { switch (test.submission) {
...@@ -176,7 +234,7 @@ TEST(PasswordFormMetricsRecorder, Actions) { ...@@ -176,7 +234,7 @@ TEST(PasswordFormMetricsRecorder, Actions) {
SCOPED_TRACE(testing::Message() SCOPED_TRACE(testing::Message()
<< "is_main_frame_secure=" << test.is_main_frame_secure << "is_main_frame_secure=" << test.is_main_frame_secure
<< ", manager_action=" << test.manager_action << ", manager_action=" << test.manager_action
<< ", user_action" << static_cast<int>(test.user_action) << ", user_action=" << static_cast<int>(test.user_action)
<< ", submit_result=" << test.submit_result); << ", submit_result=" << test.submit_result);
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
...@@ -185,7 +243,7 @@ TEST(PasswordFormMetricsRecorder, Actions) { ...@@ -185,7 +243,7 @@ TEST(PasswordFormMetricsRecorder, Actions) {
// Use a scoped PasswordFromMetricsRecorder because some metrics are recored // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
// on destruction. // on destruction.
{ {
PasswordFormMetricsRecorder recorder(test.is_main_frame_secure); PasswordFormMetricsRecorder recorder(test.is_main_frame_secure, nullptr);
recorder.SetManagerAction(test.manager_action); recorder.SetManagerAction(test.manager_action);
if (test.user_action != UserAction::kNone) if (test.user_action != UserAction::kNone)
...@@ -238,13 +296,16 @@ TEST(PasswordFormMetricsRecorder, Actions) { ...@@ -238,13 +296,16 @@ TEST(PasswordFormMetricsRecorder, Actions) {
// Test that in the case of a sequence of user actions, only the last one is // Test that in the case of a sequence of user actions, only the last one is
// recorded in ActionsV3 but all are recorded as UMA user actions. // recorded in ActionsV3 but all are recorded as UMA user actions.
TEST(PasswordFormMetricsRecorder, ActionSequence) { TEST(PasswordFormMetricsRecorder, ActionSequence) {
ukm::TestUkmRecorder test_ukm_recorder;
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
base::UserActionTester user_action_tester; base::UserActionTester user_action_tester;
// Use a scoped PasswordFromMetricsRecorder because some metrics are recored // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
// on destruction. // on destruction.
{ {
PasswordFormMetricsRecorder recorder(/*is_main_frame_secure*/ true); PasswordFormMetricsRecorder recorder(
/*is_main_frame_secure*/ true,
CreateUkmEntryBuilder(&test_ukm_recorder));
recorder.SetManagerAction( recorder.SetManagerAction(
PasswordFormMetricsRecorder::kManagerActionAutofilled); PasswordFormMetricsRecorder::kManagerActionAutofilled);
recorder.SetUserAction(UserAction::kChoosePslMatch); recorder.SetUserAction(UserAction::kChoosePslMatch);
...@@ -282,15 +343,23 @@ TEST(PasswordFormMetricsRecorder, SubmittedFormType) { ...@@ -282,15 +343,23 @@ TEST(PasswordFormMetricsRecorder, SubmittedFormType) {
<< "is_main_frame_secure=" << test.is_main_frame_secure << "is_main_frame_secure=" << test.is_main_frame_secure
<< ", form_type=" << test.form_type); << ", form_type=" << test.form_type);
ukm::TestUkmRecorder test_ukm_recorder;
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
// Use a scoped PasswordFromMetricsRecorder because some metrics are recored // Use a scoped PasswordFromMetricsRecorder because some metrics are recored
// on destruction. // on destruction.
{ {
PasswordFormMetricsRecorder recorder(test.is_main_frame_secure); PasswordFormMetricsRecorder recorder(
test.is_main_frame_secure, CreateUkmEntryBuilder(&test_ukm_recorder));
recorder.SetSubmittedFormType(test.form_type); recorder.SetSubmittedFormType(test.form_type);
} }
if (test.form_type !=
PasswordFormMetricsRecorder::kSubmittedFormTypeUnspecified) {
ExpectUkmValueCount(&test_ukm_recorder, internal::kUkmSubmissionFormType,
test.form_type, 1);
}
if (test.expected_submitted_form_type) { if (test.expected_submitted_form_type) {
histogram_tester.ExpectBucketCount("PasswordManager.SubmittedFormType", histogram_tester.ExpectBucketCount("PasswordManager.SubmittedFormType",
test.form_type, test.form_type,
......
...@@ -12,6 +12,7 @@ ...@@ -12,6 +12,7 @@
#include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/password_form.h"
#include "components/password_manager/core/browser/credentials_filter.h" #include "components/password_manager/core/browser/credentials_filter.h"
#include "components/password_manager/core/browser/password_store.h" #include "components/password_manager/core/browser/password_store.h"
#include "components/ukm/public/ukm_recorder.h"
class PrefService; class PrefService;
...@@ -215,6 +216,13 @@ class PasswordManagerClient { ...@@ -215,6 +216,13 @@ class PasswordManagerClient {
bool password_field_exists) = 0; bool password_field_exists) = 0;
#endif #endif
// Gets the UKM service associated with this client (for metrics).
virtual ukm::UkmRecorder* GetUkmRecorder() = 0;
// Gets a ukm::SourceId that is associated with the WebContents object
// and its last committed main frame navigation.
virtual ukm::SourceId GetUkmSourceId() = 0;
private: private:
DISALLOW_COPY_AND_ASSIGN(PasswordManagerClient); DISALLOW_COPY_AND_ASSIGN(PasswordManagerClient);
}; };
......
...@@ -11,7 +11,10 @@ ...@@ -11,7 +11,10 @@
namespace password_manager { namespace password_manager {
StubPasswordManagerClient::StubPasswordManagerClient() {} StubPasswordManagerClient::StubPasswordManagerClient()
: ukm_source_id_(ukm::UkmRecorder::Get()
? ukm::UkmRecorder::Get()->GetNewSourceID()
: 0) {}
StubPasswordManagerClient::~StubPasswordManagerClient() {} StubPasswordManagerClient::~StubPasswordManagerClient() {}
...@@ -79,4 +82,12 @@ void StubPasswordManagerClient::CheckProtectedPasswordEntry( ...@@ -79,4 +82,12 @@ void StubPasswordManagerClient::CheckProtectedPasswordEntry(
bool password_field_exists) {} bool password_field_exists) {}
#endif #endif
ukm::UkmRecorder* StubPasswordManagerClient::GetUkmRecorder() {
return ukm::UkmRecorder::Get();
}
ukm::SourceId StubPasswordManagerClient::GetUkmSourceId() {
return ukm_source_id_;
}
} // namespace password_manager } // namespace password_manager
...@@ -51,10 +51,13 @@ class StubPasswordManagerClient : public PasswordManagerClient { ...@@ -51,10 +51,13 @@ class StubPasswordManagerClient : public PasswordManagerClient {
void CheckProtectedPasswordEntry(const std::string& password_saved_domain, void CheckProtectedPasswordEntry(const std::string& password_saved_domain,
bool password_field_exists) override; bool password_field_exists) override;
#endif #endif
ukm::UkmRecorder* GetUkmRecorder() override;
ukm::SourceId GetUkmSourceId() override;
private: private:
const StubCredentialsFilter credentials_filter_; const StubCredentialsFilter credentials_filter_;
StubLogManager log_manager_; StubLogManager log_manager_;
ukm::SourceId ukm_source_id_;
DISALLOW_COPY_AND_ASSIGN(StubPasswordManagerClient); DISALLOW_COPY_AND_ASSIGN(StubPasswordManagerClient);
}; };
......
...@@ -38,6 +38,10 @@ namespace payments { ...@@ -38,6 +38,10 @@ namespace payments {
class JourneyLogger; class JourneyLogger;
} }
namespace password_manager {
class PasswordFormMetricsRecorder;
}
namespace ukm { namespace ukm {
class UkmEntryBuilder; class UkmEntryBuilder;
...@@ -80,6 +84,7 @@ class UKM_EXPORT UkmRecorder { ...@@ -80,6 +84,7 @@ class UKM_EXPORT UkmRecorder {
friend content::MediaInternals; friend content::MediaInternals;
friend content::RenderFrameImpl; friend content::RenderFrameImpl;
friend content::RenderWidgetHostLatencyTracker; friend content::RenderWidgetHostLatencyTracker;
friend password_manager::PasswordFormMetricsRecorder;
FRIEND_TEST_ALL_PREFIXES(UkmServiceTest, AddEntryWithEmptyMetrics); FRIEND_TEST_ALL_PREFIXES(UkmServiceTest, AddEntryWithEmptyMetrics);
FRIEND_TEST_ALL_PREFIXES(UkmServiceTest, EntryBuilderAndSerialization); FRIEND_TEST_ALL_PREFIXES(UkmServiceTest, EntryBuilderAndSerialization);
FRIEND_TEST_ALL_PREFIXES(UkmServiceTest, FRIEND_TEST_ALL_PREFIXES(UkmServiceTest,
......
...@@ -52,6 +52,7 @@ source_set("passwords") { ...@@ -52,6 +52,7 @@ source_set("passwords") {
"//components/signin/core/browser", "//components/signin/core/browser",
"//components/strings", "//components/strings",
"//components/sync", "//components/sync",
"//components/ukm",
"//google_apis", "//google_apis",
"//ios/chrome/app/strings", "//ios/chrome/app/strings",
"//ios/chrome/app/theme", "//ios/chrome/app/theme",
......
...@@ -71,6 +71,8 @@ class IOSChromePasswordManagerClient ...@@ -71,6 +71,8 @@ class IOSChromePasswordManagerClient
const GURL& GetLastCommittedEntryURL() const override; const GURL& GetLastCommittedEntryURL() const override;
const password_manager::CredentialsFilter* GetStoreResultFilter() const password_manager::CredentialsFilter* GetStoreResultFilter()
const override; const override;
ukm::UkmRecorder* GetUkmRecorder() override;
ukm::SourceId GetUkmSourceId() override;
private: private:
id<PasswordManagerClientDelegate> delegate_; // (weak) id<PasswordManagerClientDelegate> delegate_; // (weak)
...@@ -81,6 +83,14 @@ class IOSChromePasswordManagerClient ...@@ -81,6 +83,14 @@ class IOSChromePasswordManagerClient
const password_manager::SyncCredentialsFilter credentials_filter_; const password_manager::SyncCredentialsFilter credentials_filter_;
// The URL to which the ukm_source_id_ was bound.
GURL ukm_source_url_;
// If ukm_source_url_ == delegate_.lastCommittedURL, this stores a
// ukm::SourceId that is bound to the last committed navigation of the tab
// owning this ChromePasswordManagerClient.
ukm::SourceId ukm_source_id_;
DISALLOW_COPY_AND_ASSIGN(IOSChromePasswordManagerClient); DISALLOW_COPY_AND_ASSIGN(IOSChromePasswordManagerClient);
}; };
......
...@@ -14,6 +14,8 @@ ...@@ -14,6 +14,8 @@
#include "components/password_manager/core/browser/password_manager_util.h" #include "components/password_manager/core/browser/password_manager_util.h"
#include "components/password_manager/core/common/password_manager_pref_names.h" #include "components/password_manager/core/common/password_manager_pref_names.h"
#include "components/signin/core/browser/signin_manager_base.h" #include "components/signin/core/browser/signin_manager_base.h"
#include "components/ukm/public/ukm_recorder.h"
#include "ios/chrome/browser/application_context.h"
#include "ios/chrome/browser/browser_state/chrome_browser_state.h" #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
#include "ios/chrome/browser/experimental_flags.h" #include "ios/chrome/browser/experimental_flags.h"
#include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h" #include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
...@@ -50,7 +52,8 @@ IOSChromePasswordManagerClient::IOSChromePasswordManagerClient( ...@@ -50,7 +52,8 @@ IOSChromePasswordManagerClient::IOSChromePasswordManagerClient(
credentials_filter_( credentials_filter_(
this, this,
base::Bind(&GetSyncService, delegate_.browserState), base::Bind(&GetSyncService, delegate_.browserState),
base::Bind(&GetSigninManager, delegate_.browserState)) { base::Bind(&GetSigninManager, delegate_.browserState)),
ukm_source_id_(0) {
saving_passwords_enabled_.Init( saving_passwords_enabled_.Init(
password_manager::prefs::kPasswordManagerSavingEnabled, GetPrefs()); password_manager::prefs::kPasswordManagerSavingEnabled, GetPrefs());
} }
...@@ -137,3 +140,20 @@ const password_manager::CredentialsFilter* ...@@ -137,3 +140,20 @@ const password_manager::CredentialsFilter*
IOSChromePasswordManagerClient::GetStoreResultFilter() const { IOSChromePasswordManagerClient::GetStoreResultFilter() const {
return &credentials_filter_; return &credentials_filter_;
} }
ukm::UkmRecorder* IOSChromePasswordManagerClient::GetUkmRecorder() {
return GetApplicationContext()->GetUkmRecorder();
}
ukm::SourceId IOSChromePasswordManagerClient::GetUkmSourceId() {
// TODO(crbug.com/732846): The UKM Source should be recycled (e.g. from the
// web contents), once the UKM framework provides a mechanism for that.
if (ukm_source_url_ != delegate_.lastCommittedURL) {
ukm_source_url_ = delegate_.lastCommittedURL;
ukm_source_id_ = ukm::UkmRecorder::GetNewSourceID();
ukm::UkmRecorder* ukm_recorder = GetUkmRecorder();
if (ukm_recorder)
ukm_recorder->UpdateSourceURL(ukm_source_id_, ukm_source_url_);
}
return ukm_source_id_;
}
...@@ -445,6 +445,35 @@ be describing additional metrics about the same event. ...@@ -445,6 +445,35 @@ be describing additional metrics about the same event.
</metric> </metric>
</event> </event>
<event name="PasswordForm">
<owner>battre@chromium.org</owner>
<summary>
Metrics about password forms on websites and the user's interactions with
them. A separate event is generated for each password form discovered on a
site.
</summary>
<metric name="Submission.Observed">
<summary>
Records whether a submission of a password form has been observered. The
values 0 and 1 correspond to false and true respectively.
</summary>
</metric>
<metric name="Submission.SubmissionResult">
<summary>
Records the outcome of a password form submission. The values are numbered
according to PasswordFormMetricsRecorder::SubmitResult. Note that no
metric is recorded for kSubmitResultNotSubmitted.
</summary>
</metric>
<metric name="Submission.SubmittedFormType">
<summary>
Records the classification of a form at submission time. The values
correspond to PasswordFormMetricsRecorder::SubmittedFormType. Note that no
metric is recorded for kSubmittedFormTypeUnspecified.
</summary>
</metric>
</event>
<event name="PaymentRequest.CheckoutEvents"> <event name="PaymentRequest.CheckoutEvents">
<owner>sebsg@chromium.org</owner> <owner>sebsg@chromium.org</owner>
<metric name="CompletionsStatus"> <metric name="CompletionsStatus">
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment