Publish histogram when password saving is blocked by trusted vault error [chromium/src : main]
0 views
Skip to first unread message
Yurii Lahodiuk (Gerrit)
9:09 AM (4 hours ago) 9:09 AM
to Mohamed Amir Yosef, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Mohamed Amir Yosef
Yurii Lahodiuk added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Mohamed Amir Yosef
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
9:34 AM (4 hours ago) 9:34 AM
to Yurii Lahodiuk, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Yurii Lahodiuk
Mohamed Amir Yosef added 1 comment![]( "Open in Gerrit")
File components/password_manager/core/browser/password_manager.cc
Line 1667, Patchset 2 (Latest):
UMA_HISTOGRAM_BOOLEAN(Mohamed Amir Yosef . unresolved
Did you intentionally use the macro instead of the function here?
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#coding-emitting-to-histograms
Related details
Attention is currently required from:
- Yurii Lahodiuk
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
9:36 AM (4 hours ago) 9:36 AM
to Yurii Lahodiuk, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Yurii Lahodiuk
Mohamed Amir Yosef added 1 comment![]( "Open in Gerrit")
File components/password_manager/core/browser/password_manager.cc
Line 1669, Patchset 2 (Latest):
able_to_save_passwords_after_fixing_trusted_vault_error);Mohamed Amir Yosef . unresolved
Maybe add a test like
```
TEST_P(PasswordManagerTest, SavingBlockedByTrustedVaultErrorMetric) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(
password_manager::features::kPasswordSaveInContextErrorResolution);
// Setup error on the appropriate store.
if (ShouldEnableAccountStorage()) {
syncer::TestSyncService sync_service;
sync_service.GetUserSettings()->SetSelectedTypes(
/*sync_everything=*/false, {syncer::UserSelectableType::kPasswords});
ON_CALL(client_, GetSyncService()).WillByDefault(Return(&sync_service));
account_store_->SetError(ActionableError::kTrustedVaultKeyNeeded);
} else {
store_->SetError(ActionableError::kTrustedVaultKeyNeeded);
}
PasswordForm form(MakeSimpleForm());
store_->AddLogin(password_manager::FromPasswordForm(form));
FormData observed_form = form.form_data;
manager()->OnPasswordFormsParsed(&driver_, {observed_form});
manager()->OnPasswordFormsRendered(&driver_, {observed_form});
task_environment_.RunUntilIdle();
manager()->OnPasswordFormSubmitted(&driver_, observed_form);
base::HistogramTester histogram_tester;
EXPECT_CALL(client_, IsSavingAndFillingEnabled).WillRepeatedly(Return(true));
manager()->OnPasswordFormsRendered(&driver_, {});
task_environment_.RunUntilIdle();
histogram_tester.ExpectUniqueSample(
"PasswordManager.AbleToSavePasswordsOnSuccessfulLogin", false, 1);
histogram_tester.ExpectUniqueSample(
"PasswordManager.SavingBlockedByTrustedVaultErrorOnSuccessfulLogin", true,
1);
}
```
Yurii Lahodiuk (Gerrit)
12:06 PM (1 hour ago) 12:06 PM
to Mohamed Amir Yosef, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Mohamed Amir Yosef
Yurii Lahodiuk added 2 comments![]( "Open in Gerrit")
File components/password_manager/core/browser/password_manager.cc
Did you intentionally use the macro instead of the function here?
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#coding-emitting-to-histograms
Yurii Lahodiuk
Thanks for the link!
Originally, for the consistency purpose I decided to use `UMA_HISTOGRAM_BOOLEAN`, but after reading the documentation I changed the code to use `base::UmaHistogramBoolean`.
Line 1669, Patchset 2:
able_to_save_passwords_after_fixing_trusted_vault_error);Mohamed Amir Yosef . resolved
Attention is currently required from:
- Mohamed Amir Yosef
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
12:15 PM (1 hour ago) 12:15 PM
to Yurii Lahodiuk, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Attention needed from Yurii Lahodiuk
Mohamed Amir Yosef voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mohamed Amir Yosef
| Code-Review | +1 |
Mohamed Amir Yosef . resolved
Attention is currently required from:
- Yurii Lahodiuk
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yurii Lahodiuk (Gerrit)
1:11 PM (22 minutes ago) 1:11 PM
to Mohamed Amir Yosef, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Yurii Lahodiuk voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yurii Lahodiuk (Gerrit)
1:12 PM (22 minutes ago) 1:12 PM
to Mohamed Amir Yosef, Ivana Žužić, Rafał Godlewski, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Yurii Lahodiuk added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Yurii Lahodiuk . resolved
Thank you for the review!
Chromium LUCI CQ (Gerrit)
1:18 PM (16 minutes ago) 1:18 PM
to Yurii Lahodiuk, Mohamed Amir Yosef, Ivana Žužić, Rafał Godlewski, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, gcasto+w...@chromium.org, vasilii+watchlis...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Publish histogram when password saving is blocked by trusted vault error
Bug: b:484367376
Change-Id: I065aa96369780e369984d25540a79f3ab93e3db2
Reviewed-by: Mohamed Amir Yosef <ma...@chromium.org>
Commit-Queue: Yurii Lahodiuk <ylah...@google.com>
Cr-Commit-Position: refs/heads/main@{#1659666}
Files:
- M components/password_manager/core/browser/password_manager.cc
- M components/password_manager/core/browser/password_manager_unittest.cc
- M tools/metrics/histograms/metadata/password/histograms.xml
Change size: M
Delta: 3 files changed, 68 insertions(+), 5 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Mohamed Amir Yosef
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages