Commit 5c03167f authored by Victor Hugo Vianna Silva's avatar Victor Hugo Vianna Silva Committed by Commit Bot

Rename *MoveToAccountRefusedCount to *MoveOfferedToNonOptedInUserCount

Turns out it's easier to record when the move flow is offered rather
than when it's refused/ignored. For non-opted-in users these things are
basically the same, since accepting the move bubble prompts an opt-in.
No behavior is changed, the counter still isn't incremented.

Bug: 1082152
Change-Id: I379e137d8c24e02625d569d9b65ddd715050ae58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352229Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#797227}
parent a000dc42
...@@ -32,8 +32,8 @@ class MockPasswordFeatureManager : public PasswordFeatureManager { ...@@ -32,8 +32,8 @@ class MockPasswordFeatureManager : public PasswordFeatureManager {
MOCK_CONST_METHOD0(ComputePasswordAccountStorageUsageLevel, MOCK_CONST_METHOD0(ComputePasswordAccountStorageUsageLevel,
metrics_util::PasswordAccountStorageUsageLevel()); metrics_util::PasswordAccountStorageUsageLevel());
MOCK_METHOD0(IncrementMoveToAccountRefusedCount, void()); MOCK_METHOD0(RecordMoveOfferedToNonOptedInUser, void());
MOCK_CONST_METHOD0(GetMoveToAccountRefusedCount, int()); MOCK_CONST_METHOD0(GetMoveOfferedToNonOptedInUserCount, int());
}; };
} // namespace password_manager } // namespace password_manager
......
...@@ -71,17 +71,17 @@ class PasswordFeatureManager { ...@@ -71,17 +71,17 @@ class PasswordFeatureManager {
virtual metrics_util::PasswordAccountStorageUsageLevel virtual metrics_util::PasswordAccountStorageUsageLevel
ComputePasswordAccountStorageUsageLevel() const = 0; ComputePasswordAccountStorageUsageLevel() const = 0;
// Increases the count of how many times Chrome automatically offered to move // Increases the count of how many times Chrome automatically offered a user
// a password to the account and the user refused this, e.g. by ignoring the // not opted-in to the account-scoped passwords storage to move a password to
// bubble that's shown after successful sign-in with a device password. Should // their account. Should only be called if the user is signed-in and not
// only be called if the user is signed-in and not opted-in. // opted-in.
virtual void IncrementMoveToAccountRefusedCount() = 0; virtual void RecordMoveOfferedToNonOptedInUser() = 0;
// Gets the count of how many times Chrome automatically offered to move a // Gets the count of how many times Chrome automatically offered a user
// password to the account and the user refused this, e.g. by ignoring the // not opted-in to the account-scoped passwords storage to move a password to
// bubble that's shown after successful sign-in with a device password. Should // their account. Should only be called if the user is signed-in and not
// only be called if the user is signed-in and not opted-in. // opted-in.
virtual int GetMoveToAccountRefusedCount() const = 0; virtual int GetMoveOfferedToNonOptedInUserCount() const = 0;
private: private:
DISALLOW_COPY_AND_ASSIGN(PasswordFeatureManager); DISALLOW_COPY_AND_ASSIGN(PasswordFeatureManager);
......
...@@ -77,14 +77,14 @@ PasswordFeatureManagerImpl::ComputePasswordAccountStorageUsageLevel() const { ...@@ -77,14 +77,14 @@ PasswordFeatureManagerImpl::ComputePasswordAccountStorageUsageLevel() const {
sync_service_); sync_service_);
} }
void PasswordFeatureManagerImpl::IncrementMoveToAccountRefusedCount() { void PasswordFeatureManagerImpl::RecordMoveOfferedToNonOptedInUser() {
features_util::IncrementMoveToAccountRefusedCount(pref_service_, features_util::RecordMoveOfferedToNonOptedInUser(pref_service_,
sync_service_); sync_service_);
} }
int PasswordFeatureManagerImpl::GetMoveToAccountRefusedCount() const { int PasswordFeatureManagerImpl::GetMoveOfferedToNonOptedInUserCount() const {
return features_util::GetMoveToAccountRefusedCount(pref_service_, return features_util::GetMoveOfferedToNonOptedInUserCount(pref_service_,
sync_service_); sync_service_);
} }
} // namespace password_manager } // namespace password_manager
...@@ -41,8 +41,8 @@ class PasswordFeatureManagerImpl : public PasswordFeatureManager { ...@@ -41,8 +41,8 @@ class PasswordFeatureManagerImpl : public PasswordFeatureManager {
metrics_util::PasswordAccountStorageUsageLevel metrics_util::PasswordAccountStorageUsageLevel
ComputePasswordAccountStorageUsageLevel() const override; ComputePasswordAccountStorageUsageLevel() const override;
void IncrementMoveToAccountRefusedCount() override; void RecordMoveOfferedToNonOptedInUser() override;
int GetMoveToAccountRefusedCount() const override; int GetMoveOfferedToNonOptedInUserCount() const override;
private: private:
PrefService* const pref_service_; PrefService* const pref_service_;
......
...@@ -132,7 +132,7 @@ bool PasswordManagerClientHelper::ShouldPromptToMovePasswordToAccount( ...@@ -132,7 +132,7 @@ bool PasswordManagerClientHelper::ShouldPromptToMovePasswordToAccount(
features::kMaxMoveToAccountOffersForNonOptedInUser, features::kMaxMoveToAccountOffersForNonOptedInUser,
features::kMaxMoveToAccountOffersForNonOptedInUserDefaultValue); features::kMaxMoveToAccountOffersForNonOptedInUserDefaultValue);
return feature_manager->IsOptedInForAccountStorage() || return feature_manager->IsOptedInForAccountStorage() ||
feature_manager->GetMoveToAccountRefusedCount() < feature_manager->GetMoveOfferedToNonOptedInUserCount() <
max_move_to_account_offers_for_non_opted_in_user; max_move_to_account_offers_for_non_opted_in_user;
} }
......
...@@ -229,7 +229,7 @@ TEST_F(PasswordManagerClientHelperTest, ...@@ -229,7 +229,7 @@ TEST_F(PasswordManagerClientHelperTest,
// Simulate that no refusals happened so far. Moving should be offered. // Simulate that no refusals happened so far. Moving should be offered.
EXPECT_CALL(*client()->GetPasswordFeatureManager(), EXPECT_CALL(*client()->GetPasswordFeatureManager(),
GetMoveToAccountRefusedCount) GetMoveOfferedToNonOptedInUserCount)
.WillOnce(Return(0)); .WillOnce(Return(0));
EXPECT_CALL(*client(), PromptUserToMovePasswordToAccount); EXPECT_CALL(*client(), PromptUserToMovePasswordToAccount);
const PasswordForm form = const PasswordForm form =
...@@ -239,7 +239,7 @@ TEST_F(PasswordManagerClientHelperTest, ...@@ -239,7 +239,7 @@ TEST_F(PasswordManagerClientHelperTest,
// If the previous move was refused and the max is 1, shouldn't offer anymore. // If the previous move was refused and the max is 1, shouldn't offer anymore.
EXPECT_CALL(*client()->GetPasswordFeatureManager(), EXPECT_CALL(*client()->GetPasswordFeatureManager(),
GetMoveToAccountRefusedCount) GetMoveOfferedToNonOptedInUserCount)
.WillOnce(Return(1)); .WillOnce(Return(1));
EXPECT_CALL(*client(), PromptUserToMovePasswordToAccount).Times(0); EXPECT_CALL(*client(), PromptUserToMovePasswordToAccount).Times(0);
helper()->NotifySuccessfulLoginWithExistingPassword( helper()->NotifySuccessfulLoginWithExistingPassword(
......
...@@ -72,7 +72,7 @@ PasswordForm::Store PasswordStoreFromInt(int value) { ...@@ -72,7 +72,7 @@ PasswordForm::Store PasswordStoreFromInt(int value) {
const char kAccountStorageOptedInKey[] = "opted_in"; const char kAccountStorageOptedInKey[] = "opted_in";
const char kAccountStorageDefaultStoreKey[] = "default_store"; const char kAccountStorageDefaultStoreKey[] = "default_store";
const char kMoveToAccountStoreRefusedCountKey[] = const char kMoveToAccountStoreOfferedCountKey[] =
"move_to_account_store_refused_count"; "move_to_account_store_refused_count";
// Returns the total number of accounts for which an opt-in to the account // Returns the total number of accounts for which an opt-in to the account
...@@ -117,10 +117,10 @@ class AccountStorageSettingsReader { ...@@ -117,10 +117,10 @@ class AccountStorageSettingsReader {
return PasswordStoreFromInt(*value); return PasswordStoreFromInt(*value);
} }
int GetMoveToAccountRefusedCount() const { int GetMoveOfferedToNonOptedInUserCount() const {
if (!account_settings_) if (!account_settings_)
return 0; return 0;
return account_settings_->FindIntKey(kMoveToAccountStoreRefusedCountKey) return account_settings_->FindIntKey(kMoveToAccountStoreOfferedCountKey)
.value_or(0); .value_or(0);
} }
...@@ -152,7 +152,7 @@ class ScopedAccountStorageSettingsUpdate { ...@@ -152,7 +152,7 @@ class ScopedAccountStorageSettingsUpdate {
void SetOptedIn() { void SetOptedIn() {
base::Value* account_settings = GetOrCreateAccountSettings(); base::Value* account_settings = GetOrCreateAccountSettings();
// The count of refusals is only tracked when the user is not opted-in. // The count of refusals is only tracked when the user is not opted-in.
account_settings->RemoveKey(kMoveToAccountStoreRefusedCountKey); account_settings->RemoveKey(kMoveToAccountStoreOfferedCountKey);
account_settings->SetBoolKey(kAccountStorageOptedInKey, true); account_settings->SetBoolKey(kAccountStorageOptedInKey, true);
} }
...@@ -162,11 +162,11 @@ class ScopedAccountStorageSettingsUpdate { ...@@ -162,11 +162,11 @@ class ScopedAccountStorageSettingsUpdate {
static_cast<int>(default_store)); static_cast<int>(default_store));
} }
void IncrementMovePasswordToAccountBubble() { void RecordMoveOfferedToNonOptedInUser() {
base::Value* account_settings = GetOrCreateAccountSettings(); base::Value* account_settings = GetOrCreateAccountSettings();
int count = account_settings->FindIntKey(kMoveToAccountStoreRefusedCountKey) int count = account_settings->FindIntKey(kMoveToAccountStoreOfferedCountKey)
.value_or(0); .value_or(0);
account_settings->SetIntKey(kMoveToAccountStoreRefusedCountKey, ++count); account_settings->SetIntKey(kMoveToAccountStoreOfferedCountKey, ++count);
} }
void ClearAllSettings() { update_->RemoveKey(account_hash_); } void ClearAllSettings() { update_->RemoveKey(account_hash_); }
...@@ -450,7 +450,7 @@ PasswordAccountStorageUsageLevel ComputePasswordAccountStorageUsageLevel( ...@@ -450,7 +450,7 @@ PasswordAccountStorageUsageLevel ComputePasswordAccountStorageUsageLevel(
} }
} }
void IncrementMoveToAccountRefusedCount( void RecordMoveOfferedToNonOptedInUser(
PrefService* pref_service, PrefService* pref_service,
const syncer::SyncService* sync_service) { const syncer::SyncService* sync_service) {
DCHECK(pref_service); DCHECK(pref_service);
...@@ -462,11 +462,12 @@ void IncrementMoveToAccountRefusedCount( ...@@ -462,11 +462,12 @@ void IncrementMoveToAccountRefusedCount(
.IsOptedIn()); .IsOptedIn());
ScopedAccountStorageSettingsUpdate(pref_service, ScopedAccountStorageSettingsUpdate(pref_service,
GaiaIdHash::FromGaiaId(gaia_id)) GaiaIdHash::FromGaiaId(gaia_id))
.IncrementMovePasswordToAccountBubble(); .RecordMoveOfferedToNonOptedInUser();
} }
int GetMoveToAccountRefusedCount(const PrefService* pref_service, int GetMoveOfferedToNonOptedInUserCount(
const syncer::SyncService* sync_service) { const PrefService* pref_service,
const syncer::SyncService* sync_service) {
DCHECK(pref_service); DCHECK(pref_service);
DCHECK(sync_service); DCHECK(sync_service);
std::string gaia_id = sync_service->GetAuthenticatedAccountInfo().gaia; std::string gaia_id = sync_service->GetAuthenticatedAccountInfo().gaia;
...@@ -474,7 +475,7 @@ int GetMoveToAccountRefusedCount(const PrefService* pref_service, ...@@ -474,7 +475,7 @@ int GetMoveToAccountRefusedCount(const PrefService* pref_service,
AccountStorageSettingsReader reader(pref_service, AccountStorageSettingsReader reader(pref_service,
GaiaIdHash::FromGaiaId(gaia_id)); GaiaIdHash::FromGaiaId(gaia_id));
DCHECK(!reader.IsOptedIn()); DCHECK(!reader.IsOptedIn());
return reader.GetMoveToAccountRefusedCount(); return reader.GetMoveOfferedToNonOptedInUserCount();
} }
} // namespace features_util } // namespace features_util
......
...@@ -128,24 +128,22 @@ ComputePasswordAccountStorageUsageLevel( ...@@ -128,24 +128,22 @@ ComputePasswordAccountStorageUsageLevel(
const PrefService* pref_service, const PrefService* pref_service,
const syncer::SyncService* sync_service); const syncer::SyncService* sync_service);
// Increases the count of how many times Chrome automatically offered to move a // Increases the count of how many times Chrome automatically offered a user
// password to the account and the user refused this, e.g. by ignoring the // not opted-in to the account-scoped passwords storage to move a password to
// bubble that's shown after successful sign-in with a device password. Should // their account. Should only be called if the user is signed-in and not
// only be called if the user is signed-in and not opted-in. |pref_service| and // opted-in. |pref_service| and |sync_service| must be non-null.
// |sync_service| must be non-null. // See PasswordFeatureManager::RecordMoveOfferedToNonOptedInUser().
// See PasswordFeatureManager::IncrementMoveToAccountRefusedCount(). void RecordMoveOfferedToNonOptedInUser(PrefService* pref_service,
void IncrementMoveToAccountRefusedCount( const syncer::SyncService* sync_service);
PrefService* pref_service,
const syncer::SyncService* sync_service);
// Gets the count of how many times Chrome automatically offered to move a // Gets the count of how many times Chrome automatically offered a user
// password to the account and the user refused this, e.g. by ignoring the // not opted-in to the account-scoped passwords storage to move a password to
// bubble that's shown after successful sign-in with a device password. Should // their account. Should only be called if the user is signed-in and not
// only be called if the user is signed-in and not opted-in. |pref_service| and // opted-in. |pref_service| and |sync_service| must be non-null.
// |sync_service| must be non-null. // See PasswordFeatureManager::GetMoveOfferedToNonOptedInUserCount().
// See PasswordFeatureManager::GetMoveToAccountRefusedCount(). int GetMoveOfferedToNonOptedInUserCount(
int GetMoveToAccountRefusedCount(const PrefService* pref_service, const PrefService* pref_service,
const syncer::SyncService* sync_service); const syncer::SyncService* sync_service);
} // namespace features_util } // namespace features_util
......
...@@ -506,7 +506,7 @@ TEST(PasswordFeatureManagerUtil, OptInOutHistograms) { ...@@ -506,7 +506,7 @@ TEST(PasswordFeatureManagerUtil, OptInOutHistograms) {
"PasswordManager.AccountStorage.ClearedOptInForAllAccounts", 1, 1); "PasswordManager.AccountStorage.ClearedOptInForAllAccounts", 1, 1);
} }
TEST(PasswordFeatureManagerUtil, MovePasswordToAccountStoreRefusedCount) { TEST(PasswordFeatureManagerUtil, MovePasswordToAccountStoreOfferedCount) {
// Set up a user signed-in, not syncing and not opted-in. // Set up a user signed-in, not syncing and not opted-in.
base::test::ScopedFeatureList features; base::test::ScopedFeatureList features;
features.InitAndEnableFeature(features::kEnablePasswordsAccountStorage); features.InitAndEnableFeature(features::kEnablePasswordsAccountStorage);
...@@ -522,11 +522,14 @@ TEST(PasswordFeatureManagerUtil, MovePasswordToAccountStoreRefusedCount) { ...@@ -522,11 +522,14 @@ TEST(PasswordFeatureManagerUtil, MovePasswordToAccountStoreRefusedCount) {
sync_service.SetIsAuthenticatedAccountPrimary(false); sync_service.SetIsAuthenticatedAccountPrimary(false);
ASSERT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service)); ASSERT_FALSE(IsOptedInForAccountStorage(&pref_service, &sync_service));
EXPECT_EQ(0, GetMoveToAccountRefusedCount(&pref_service, &sync_service)); EXPECT_EQ(0,
IncrementMoveToAccountRefusedCount(&pref_service, &sync_service); GetMoveOfferedToNonOptedInUserCount(&pref_service, &sync_service));
EXPECT_EQ(1, GetMoveToAccountRefusedCount(&pref_service, &sync_service)); RecordMoveOfferedToNonOptedInUser(&pref_service, &sync_service);
IncrementMoveToAccountRefusedCount(&pref_service, &sync_service); EXPECT_EQ(1,
EXPECT_EQ(2, GetMoveToAccountRefusedCount(&pref_service, &sync_service)); GetMoveOfferedToNonOptedInUserCount(&pref_service, &sync_service));
RecordMoveOfferedToNonOptedInUser(&pref_service, &sync_service);
EXPECT_EQ(2,
GetMoveOfferedToNonOptedInUserCount(&pref_service, &sync_service));
} }
} // namespace features_util } // namespace features_util
......
...@@ -41,8 +41,8 @@ class WebViewPasswordFeatureManager ...@@ -41,8 +41,8 @@ class WebViewPasswordFeatureManager
password_manager::metrics_util::PasswordAccountStorageUsageLevel password_manager::metrics_util::PasswordAccountStorageUsageLevel
ComputePasswordAccountStorageUsageLevel() const override; ComputePasswordAccountStorageUsageLevel() const override;
void IncrementMoveToAccountRefusedCount() override; void RecordMoveOfferedToNonOptedInUser() override;
int GetMoveToAccountRefusedCount() const override; int GetMoveOfferedToNonOptedInUserCount() const override;
private: private:
PrefService* const pref_service_; PrefService* const pref_service_;
......
...@@ -71,11 +71,11 @@ WebViewPasswordFeatureManager::ComputePasswordAccountStorageUsageLevel() const { ...@@ -71,11 +71,11 @@ WebViewPasswordFeatureManager::ComputePasswordAccountStorageUsageLevel() const {
kUsingAccountStorage; kUsingAccountStorage;
} }
void WebViewPasswordFeatureManager::IncrementMoveToAccountRefusedCount() { void WebViewPasswordFeatureManager::RecordMoveOfferedToNonOptedInUser() {
NOTREACHED(); NOTREACHED();
} }
int WebViewPasswordFeatureManager::GetMoveToAccountRefusedCount() const { int WebViewPasswordFeatureManager::GetMoveOfferedToNonOptedInUserCount() const {
NOTREACHED(); NOTREACHED();
return 0; return 0;
} }
......
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