[PWM Onboarding]Add new password onboarding pref to fix feature check

This change adds a pref that is set to true when the first
eligibility check passes and is not reset on subsequent checks. This
helps ensure that the feature is checked for all past and present
eligible cases.

Bug: 1021033
Change-Id: Ie0f48e07ed5e28b8ee6fec0400d7d5fac41f1a3a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893204
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Natalie Chouinard <chouinard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714879}

components/password_manager/core/browser/password_manager.cc

@@ -173,6 +173,8 @@ void PasswordManager::RegisterProfilePrefs(
   registry->RegisterIntegerPref(
       prefs::kPasswordManagerOnboardingState,
       static_cast<int>(metrics_util::OnboardingState::kDoNotShow));
+  registry->RegisterBooleanPref(prefs::kWasOnboardingFeatureCheckedBefore,
+                                false);
 
 #if defined(OS_MACOSX)
   registry->RegisterIntegerPref(prefs::kKeychainMigrationStatus,

components/password_manager/core/browser/password_manager_onboarding.cc

@@ -110,38 +110,45 @@ bool ShouldShowOnboarding(PrefService* prefs,
                           PasswordUpdateBool is_password_update,
                           BlacklistedBool is_blacklisted,
                           SyncState sync_state) {
-  if (sync_state == NOT_SYNCING) {
-    return false;
-  }
   if (is_blacklisted) {
     return false;
   }
+
   if (is_password_update) {
     return false;
   }
 
-  int pref_value = prefs->GetInteger(
-      password_manager::prefs::kPasswordManagerOnboardingState);
+  bool was_feature_checked_before = prefs->GetBoolean(
+      password_manager::prefs::kWasOnboardingFeatureCheckedBefore);
 
-  bool should_show =
-      (pref_value == static_cast<int>(OnboardingState::kShouldShow));
-  bool already_shown =
-      (pref_value == static_cast<int>(OnboardingState::kShown));
-  bool is_eligible = should_show || already_shown;
+  if (was_feature_checked_before) {
+    // This is a signal that the user was at some point eligible for onboarding.
+    // The feature needs to be checked again, irrespective of onboarding status,
+    // in order to ensure data completeness.
+    ignore_result(base::FeatureList::IsEnabled(
+        password_manager::features::kPasswordManagerOnboardingAndroid));
+  }
 
-  if (!is_eligible) {
+  if (sync_state == NOT_SYNCING) {
     return false;
   }
 
-  // It is very important that the feature is checked only for eligible users.
-  // otherwise the data will be diluted. It's also important that the feature is
-  // checked in all eligible cases, even if the onboarding prompt was shown,
-  // otherwise the data will be incomplete.
-  if (!base::FeatureList::IsEnabled(
-          password_manager::features::kPasswordManagerOnboardingAndroid)) {
+  int pref_value = prefs->GetInteger(
+      password_manager::prefs::kPasswordManagerOnboardingState);
+  bool should_show =
+      (pref_value == static_cast<int>(OnboardingState::kShouldShow));
+  if (!should_show)
     return false;
-  }
-  return should_show;
+
+  // It is very important that the feature is checked only for users who
+  // are or were eligible for onboarding, otherwise the data will be diluted.
+  // It's also important that the feature is checked in all eligible cases,
+  // including past eligibiliy and users having already seen the onboarding
+  // prompt, otherwise the data will be incomplete.
+  prefs->SetBoolean(password_manager::prefs::kWasOnboardingFeatureCheckedBefore,
+                    true);
+  return base::FeatureList::IsEnabled(
+      password_manager::features::kPasswordManagerOnboardingAndroid);
 }
 
 SavingFlowMetricsRecorder::SavingFlowMetricsRecorder() = default;

components/password_manager/core/browser/password_manager_onboarding_unittest.cc

@@ -48,6 +48,8 @@ class PasswordManagerOnboardingTest : public testing::Test {
     prefs_->registry()->RegisterIntegerPref(
         prefs::kPasswordManagerOnboardingState,
         static_cast<int>(OnboardingState::kDoNotShow));
+    prefs_->registry()->RegisterBooleanPref(
+        prefs::kWasOnboardingFeatureCheckedBefore, false);
   }
 
   void TearDown() override {
@@ -123,7 +125,7 @@ TEST_F(PasswordManagerOnboardingTest,
 TEST_F(PasswordManagerOnboardingTest,
        CredentialsCountThresholdHitAfterShouldShow) {
   // Check that the threshold is handled correctly
-  // in case the current state is |kShouldShow|.
+  // in case the current state was |kShouldShow|.
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(
       features::kPasswordManagerOnboardingAndroid);

components/password_manager/core/browser/password_manager_unittest.cc

@@ -330,6 +330,8 @@ class PasswordManagerTest : public testing::Test {
     prefs_->registry()->RegisterIntegerPref(
         prefs::kPasswordManagerOnboardingState,
         static_cast<int>(metrics_util::OnboardingState::kDoNotShow));
+    prefs_->registry()->RegisterBooleanPref(
+        prefs::kWasOnboardingFeatureCheckedBefore, false);
     prefs_->registry()->RegisterBooleanPref(
         prefs::kPasswordLeakDetectionEnabled, true);
     ON_CALL(client_, GetPrefs()).WillByDefault(Return(prefs_.get()));

components/password_manager/core/common/password_manager_pref_names.cc

@@ -57,5 +57,8 @@ const char kPasswordManagerOnboardingState[] =
 const char kPasswordLeakDetectionEnabled[] =
     "profile.password_manager_leak_detection";
 
+const char kWasOnboardingFeatureCheckedBefore[] =
+    "profile.was_pwm_onboarding_feature_checked_before";
+
 }  // namespace prefs
 }  // namespace password_manager

components/password_manager/core/common/password_manager_pref_names.h

@@ -89,6 +89,12 @@ extern const char kPasswordManagerOnboardingState[];
 // submitted by the user were part of a leak.
 extern const char kPasswordLeakDetectionEnabled[];
 
+// Boolean indicating whether this profile was ever eligible for password
+// manager onboarding. If the profile was eligible, then the feature flag
+// will be checked and this will be set to true. This is then used for
+// subsequent feature checks to ensure data completeness.
+extern const char kWasOnboardingFeatureCheckedBefore[];
+
 }  // namespace prefs
 }  // namespace password_manager