Passwords: Add 3-button save dialog initial metrics
This builds on crrev.com/c/6483569 with:
- A new CLICKED_NOT_NOW dialog dismissal reason
- Interactive test coverage for the new (and existing) dismiss buttons
Bug: 411441552
Change-Id: I0fdc142083710d6be873b7f80da351a83489884b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6501099
Reviewed-by: Dana Fried <dfried@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Reviewed-by: Maria Kazinova <kazinova@google.com>
Commit-Queue: Christopher Grant <cjgrant@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1456323}
diff --git a/chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc b/chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc
index 754cb4d1..47021c5 100644
--- a/chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc
+++ b/chrome/browser/ui/passwords/bubble_controllers/save_update_bubble_controller.cc
@@ -154,7 +154,7 @@
void SaveUpdateBubbleController::OnNotNowClicked() {
CHECK_EQ(password_manager::ui::PENDING_PASSWORD_STATE, GetState());
- // TODO(crbug.com/414573697): Log appropriate metrics.
+ SetDismissalReason(metrics_util::CLICKED_NOT_NOW);
if (delegate_) {
delegate_->OnNotNowClicked();
}
diff --git a/chrome/browser/ui/passwords/manage_passwords_test.h b/chrome/browser/ui/passwords/manage_passwords_test.h
index 49e6f4d4..a4d53cf 100644
--- a/chrome/browser/ui/passwords/manage_passwords_test.h
+++ b/chrome/browser/ui/passwords/manage_passwords_test.h
@@ -105,6 +105,14 @@
password_manager::PasswordStoreInterface* profile_store = nullptr,
password_manager::PasswordStoreInterface* account_store = nullptr);
+ auto CheckHistogramUniqueSample(const std::string& name,
+ int sample,
+ int expected_count) {
+ return Do([=, this]() {
+ histogram_tester_.ExpectUniqueSample(name, sample, expected_count);
+ });
+ }
+
private:
password_manager::PasswordForm password_form_;
password_manager::PasswordForm insecure_credential_;
diff --git a/chrome/browser/ui/signin/promos/bubble_signin_promo_interactive_uitest.cc b/chrome/browser/ui/signin/promos/bubble_signin_promo_interactive_uitest.cc
index b3f7c5e5..96e3ee9a 100644
--- a/chrome/browser/ui/signin/promos/bubble_signin_promo_interactive_uitest.cc
+++ b/chrome/browser/ui/signin/promos/bubble_signin_promo_interactive_uitest.cc
@@ -238,16 +238,16 @@
RunTestSequence(
WaitForEvent(BubbleSignInPromoSignInButtonView::kPromoSignInButton,
kBubbleSignInPromoSignInButtonHasCallback),
- EnsurePresent(PasswordSaveUpdateView::kPasswordBubble),
+ EnsurePresent(PasswordSaveUpdateView::kPasswordBubbleElementId),
SetOnIncompatibleAction(
OnIncompatibleAction::kIgnoreAndContinue,
"Screenshot can only run in pixel_tests on Windows."),
- Screenshot(PasswordSaveUpdateView::kPasswordBubble, std::string(),
- "5455375"),
+ Screenshot(PasswordSaveUpdateView::kPasswordBubbleElementId,
+ std::string(), "5455375"),
NameChildViewByType<views::MdTextButton>(
BubbleSignInPromoSignInButtonView::kPromoSignInButton, kButton),
PressButton(kButton).SetMustRemainVisible(false),
- EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubble));
+ EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubbleElementId));
// Check that clicking the sign in button navigated to a sign in page.
EXPECT_TRUE(IsSignInURL());
@@ -322,16 +322,16 @@
RunTestSequence(
WaitForEvent(BubbleSignInPromoSignInButtonView::kPromoSignInButton,
kBubbleSignInPromoSignInButtonHasCallback),
- EnsurePresent(PasswordSaveUpdateView::kPasswordBubble),
+ EnsurePresent(PasswordSaveUpdateView::kPasswordBubbleElementId),
SetOnIncompatibleAction(
OnIncompatibleAction::kIgnoreAndContinue,
"Screenshot can only run in pixel_tests on Windows."),
- Screenshot(PasswordSaveUpdateView::kPasswordBubble, std::string(),
- "5455375"),
+ Screenshot(PasswordSaveUpdateView::kPasswordBubbleElementId,
+ std::string(), "5455375"),
NameChildViewByType<views::MdTextButton>(
BubbleSignInPromoSignInButtonView::kPromoSignInButton, kButton),
PressButton(kButton).SetMustRemainVisible(false),
- EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubble));
+ EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubbleElementId));
// Check that there is no helper attached to the sign in tab, because the
// password was already moved.
@@ -390,16 +390,16 @@
RunTestSequence(
WaitForEvent(BubbleSignInPromoSignInButtonView::kPromoSignInButton,
kBubbleSignInPromoSignInButtonHasCallback),
- EnsurePresent(PasswordSaveUpdateView::kPasswordBubble),
+ EnsurePresent(PasswordSaveUpdateView::kPasswordBubbleElementId),
SetOnIncompatibleAction(
OnIncompatibleAction::kIgnoreAndContinue,
"Screenshot can only run in pixel_tests on Windows."),
- Screenshot(PasswordSaveUpdateView::kPasswordBubble, std::string(),
- "5455375"),
+ Screenshot(PasswordSaveUpdateView::kPasswordBubbleElementId,
+ std::string(), "5455375"),
NameChildViewByType<views::MdTextButton>(
BubbleSignInPromoSignInButtonView::kPromoSignInButton, kButton),
PressButton(kButton).SetMustRemainVisible(false),
- EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubble));
+ EnsureNotPresent(PasswordSaveUpdateView::kPasswordBubbleElementId));
// Check that clicking the sign in button navigated to a sign in page.
EXPECT_TRUE(IsSignInURL());
diff --git a/chrome/browser/ui/views/passwords/password_bubble_interactive_uitest.cc b/chrome/browser/ui/views/passwords/password_bubble_interactive_uitest.cc
index 2139be8..752ff85 100644
--- a/chrome/browser/ui/views/passwords/password_bubble_interactive_uitest.cc
+++ b/chrome/browser/ui/views/passwords/password_bubble_interactive_uitest.cc
@@ -24,6 +24,7 @@
#include "chrome/browser/ui/passwords/manage_passwords_ui_controller.h"
#include "chrome/browser/ui/tab_dialogs.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/controls/rich_hover_button.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/passwords/manage_passwords_details_view.h"
@@ -57,6 +58,7 @@
#include "ui/views/focus/focus_manager.h"
#include "ui/views/test/views_test_utils.h"
#include "ui/views/test/widget_test.h"
+#include "ui/views/window/dialog_client_view.h"
using base::Bucket;
using net::test_server::BasicHttpResponse;
@@ -1342,3 +1344,43 @@
RunTestSequence(Do(setup_shared_passwords),
EnsureNotPresent(SharedPasswordsNotificationView::kTopView));
}
+
+IN_PROC_BROWSER_TEST_F(PasswordBubbleInteractiveUiTest, ClickNever) {
+ SetupPendingPassword();
+ const auto button = views::DialogClientView::kCancelButtonElementId;
+ RunTestSequence(PressButton(button), WaitForHide(button),
+ CheckHistogramUniqueSample(
+ "PasswordManager.SaveUIDismissalReason",
+ password_manager::metrics_util::CLICKED_NEVER, 1));
+}
+class ThreeButtonPasswordBubbleInteractiveUiTest
+ : public PasswordBubbleInteractiveUiTest {
+ public:
+ ThreeButtonPasswordBubbleInteractiveUiTest() {
+ scoped_feature_list_.InitWithFeatureState(
+ features::kThreeButtonPasswordSaveDialog, true);
+ }
+ ~ThreeButtonPasswordBubbleInteractiveUiTest() override = default;
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(ThreeButtonPasswordBubbleInteractiveUiTest,
+ ClickNotNow) {
+ SetupPendingPassword();
+ const auto button = views::DialogClientView::kCancelButtonElementId;
+ RunTestSequence(PressButton(button), WaitForHide(button),
+ CheckHistogramUniqueSample(
+ "PasswordManager.SaveUIDismissalReason",
+ password_manager::metrics_util::CLICKED_NOT_NOW, 1));
+}
+
+IN_PROC_BROWSER_TEST_F(ThreeButtonPasswordBubbleInteractiveUiTest, ClickNever) {
+ SetupPendingPassword();
+ const auto button = PasswordSaveUpdateView::kExtraButtonElementId;
+ RunTestSequence(PressButton(button), WaitForHide(button),
+ CheckHistogramUniqueSample(
+ "PasswordManager.SaveUIDismissalReason",
+ password_manager::metrics_util::CLICKED_NEVER, 1));
+}
diff --git a/chrome/browser/ui/views/passwords/password_save_update_view.cc b/chrome/browser/ui/views/passwords/password_save_update_view.cc
index 31eb552ff..ad68ce2 100644
--- a/chrome/browser/ui/views/passwords/password_save_update_view.cc
+++ b/chrome/browser/ui/views/passwords/password_save_update_view.cc
@@ -141,6 +141,8 @@
if (base::FeatureList::IsEnabled(
features::kThreeButtonPasswordSaveDialog)) {
extra_view_ = SetExtraView(std::make_unique<views::MdTextButton>());
+ extra_view_->SetProperty(views::kElementIdentifierKey,
+ kExtraButtonElementId);
}
}
@@ -241,7 +243,7 @@
AnnounceBubbleChange();
GetBubbleFrameView()->SetProperty(views::kElementIdentifierKey,
- kPasswordBubble);
+ kPasswordBubbleElementId);
return false;
#else
@@ -442,4 +444,7 @@
BEGIN_METADATA(PasswordSaveUpdateView)
END_METADATA
-DEFINE_CLASS_ELEMENT_IDENTIFIER_VALUE(PasswordSaveUpdateView, kPasswordBubble);
+DEFINE_CLASS_ELEMENT_IDENTIFIER_VALUE(PasswordSaveUpdateView,
+ kPasswordBubbleElementId);
+DEFINE_CLASS_ELEMENT_IDENTIFIER_VALUE(PasswordSaveUpdateView,
+ kExtraButtonElementId);
diff --git a/chrome/browser/ui/views/passwords/password_save_update_view.h b/chrome/browser/ui/views/passwords/password_save_update_view.h
index 9a1ddfc..ecdcac5 100644
--- a/chrome/browser/ui/views/passwords/password_save_update_view.h
+++ b/chrome/browser/ui/views/passwords/password_save_update_view.h
@@ -28,7 +28,8 @@
METADATA_HEADER(PasswordSaveUpdateView, PasswordBubbleViewBase)
public:
- DECLARE_CLASS_ELEMENT_IDENTIFIER_VALUE(kPasswordBubble);
+ DECLARE_CLASS_ELEMENT_IDENTIFIER_VALUE(kPasswordBubbleElementId);
+ DECLARE_CLASS_ELEMENT_IDENTIFIER_VALUE(kExtraButtonElementId);
PasswordSaveUpdateView(content::WebContents* web_contents,
views::View* anchor_view,
diff --git a/components/password_manager/core/browser/password_form_metrics_recorder.cc b/components/password_manager/core/browser/password_form_metrics_recorder.cc
index 585c881..592825d 100644
--- a/components/password_manager/core/browser/password_form_metrics_recorder.cc
+++ b/components/password_manager/core/browser/password_form_metrics_recorder.cc
@@ -51,6 +51,7 @@
// Declined by user.
case metrics_util::CLICKED_CANCEL:
case metrics_util::CLICKED_NEVER:
+ case metrics_util::CLICKED_NOT_NOW:
return BubbleDismissalReason::kDeclined;
// Ignored by user.
diff --git a/components/password_manager/core/browser/password_manager_metrics_util.h b/components/password_manager/core/browser/password_manager_metrics_util.h
index 239d93f9..468a32e 100644
--- a/components/password_manager/core/browser/password_manager_metrics_util.h
+++ b/components/password_manager/core/browser/password_manager_metrics_util.h
@@ -67,6 +67,8 @@
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
// Metrics: "PasswordManager.UIDismissalReason"
+//
+// LINT.IfChange(UIDismissalReason)
enum UIDismissalReason {
// We use this to mean both "Bubble lost focus" and "No interaction with the
// infobar".
@@ -86,8 +88,10 @@
CLICKED_MANAGE_PASSWORD = 13,
CLICKED_GOT_IT = 14,
CLICKED_ABOUT_PASSWORD_CHANGE = 15,
+ CLICKED_NOT_NOW = 16,
NUM_UI_RESPONSES,
};
+// LINT.ThenChange(/tools/metrics/histograms/metadata/password/enums.xml:PasswordManagerUIDismissalReason)
// Enum representing the different leak detection dialogs shown to the user.
// Corresponds to LeakDetectionDialogType suffix in histogram_suffixes_list.xml
diff --git a/tools/metrics/histograms/metadata/password/enums.xml b/tools/metrics/histograms/metadata/password/enums.xml
index 79517f2..b15e0fc 100644
--- a/tools/metrics/histograms/metadata/password/enums.xml
+++ b/tools/metrics/histograms/metadata/password/enums.xml
@@ -1338,6 +1338,8 @@
label="Not Syncing/Sync pasword saved. This value should not happen."/>
</enum>
+<!-- LINT.IfChange(PasswordManagerUIDismissalReason) -->
+
<enum name="PasswordManagerUIDismissalReason">
<int value="0" label="Bubble lost focus / No infobar interaction"/>
<int value="1" label="Clicked 'Save'/'Update'/'Move'/etc"/>
@@ -1360,8 +1362,11 @@
<int value="15"
label="Clicked 'About password change' link in the password change
bubble."/>
+ <int value="16" label="Clicked 'Not now' in the save password bubble."/>
</enum>
+<!-- LINT.ThenChange(/components/password_manager/core/browser/password_manager_metrics_util.h:UIDismissalReason) -->
+
<enum name="PasswordNoteAction">
<int value="0"
label="A new credential is added from settings, with the note field not
