[M116][UPMLocalPwd] Don't show the warning more than once a month

Every time a warning is shown the timestamp is stored in a pref. When
attempting to show another warning, the time interval is checked. If
less than 30 days have passed since the last time it was shown, no
prompt is triggered.

This CL also introduces a feature param that allows bypassing the 1
month timeout for manual testing.

(cherry picked from commit f0607cd)

Bug: 1439853
Change-Id: I45577881a2cc95e6335918cb18fce436fd05f886
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4620688
Reviewed-by: Ivana Žužić <izuzic@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1161803}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4645370
Cr-Commit-Position: refs/branch-heads/5845@{#90}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

chrome/android/java/src/org/chromium/chrome/browser/password_manager/settings/PasswordManagerHandler.java

@@ -80,4 +80,10 @@ void serializePasswords(
      */
     void showPasswordEntryEditingView(Context context, SettingsLauncher settingsLauncher, int index,
             boolean isBlockedCredential);
+
+    /**
+     * Checks whether the all the conditions for the migraiton warning to be shown are met.
+     * This includes the flag check, whether there was another warning shown in the past month, etc.
+     */
+    boolean shouldShowMigrationWarning();
 }

chrome/android/java/src/org/chromium/chrome/browser/password_manager/settings/PasswordSettings.java

@@ -32,7 +32,6 @@
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.feedback.FragmentHelpAndFeedbackLauncher;
 import org.chromium.chrome.browser.feedback.HelpAndFeedbackLauncher;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.password_check.PasswordCheck;
 import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
 import org.chromium.chrome.browser.password_manager.ManagePasswordsReferrer;
@@ -405,8 +404,9 @@ public void passwordListAvailable(int count) {
         }
 
         if (!mNoPasswords
-                && ChromeFeatureList.isEnabled(
-                        ChromeFeatureList.UNIFIED_PASSWORD_MANAGER_LOCAL_PWD_MIGRATION_WARNING)) {
+                && PasswordManagerHandlerProvider.getInstance()
+                           .getPasswordManagerHandler()
+                           .shouldShowMigrationWarning()) {
             PasswordMigrationWarningCoordinator passwordMigrationWarningCoordinator =
                     new PasswordMigrationWarningCoordinator(getContext(), mProfile,
                             mBottomSheetController, SyncConsentActivityLauncherImpl.get(),

chrome/android/java/src/org/chromium/chrome/browser/password_manager/settings/PasswordUIView.java

@@ -110,6 +110,11 @@ public void showPasswordEntryEditingView(Context context, SettingsLauncher setti
                 context, settingsLauncher, index, PasswordUIView.this);
     }
 
+    @Override
+    public boolean shouldShowMigrationWarning() {
+        return PasswordUIViewJni.get().shouldShowMigrationWarning();
+    }
+
     /**
      * Returns the URL for the website for managing one's passwords without the need to use Chrome
      * with the user's profile signed in.
@@ -168,5 +173,6 @@ void handleShowPasswordEntryEditingView(long nativePasswordUIViewAndroid, Contex
                 SettingsLauncher launcher, int index, PasswordUIView caller);
         void handleShowBlockedCredentialView(long nativePasswordUIViewAndroid, Context context,
                 SettingsLauncher launcher, int index, PasswordUIView caller);
+        boolean shouldShowMigrationWarning();
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/password_manager/settings/FakePasswordManagerHandler.java

@@ -38,6 +38,8 @@ final class FakePasswordManagerHandler implements PasswordManagerHandler {
     @Nullable
     private String mExportTargetPath;
 
+    private boolean mShouldShowWarning;
+
     void setSavedPasswords(ArrayList<SavedPasswordEntry> savedPasswords) {
         mSavedPasswords = savedPasswords;
     }
@@ -46,6 +48,10 @@ void setSavedPasswordExceptions(ArrayList<String> savedPasswordExceptions) {
         mSavedPasswordExeptions = savedPasswordExceptions;
     }
 
+    void setShouldShowWarning(boolean shouldShowWarning) {
+        mShouldShowWarning = shouldShowWarning;
+    }
+
     IntStringCallback getExportSuccessCallback() {
         return mExportSuccessCallback;
     }
@@ -112,4 +118,9 @@ public void showPasswordEntryEditingView(
             Context context, SettingsLauncher launcher, int index, boolean isBlockedCredential) {
         assert false : "Define this method before starting to use it in tests.";
     }
+
+    @Override
+    public boolean shouldShowMigrationWarning() {
+        return mShouldShowWarning;
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/password_manager/settings/PasswordSettingsTest.java

@@ -20,7 +20,6 @@
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.TestRule;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
@@ -30,7 +29,6 @@
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.CriteriaHelper;
 import org.chromium.base.test.util.Feature;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.password_check.PasswordCheck;
 import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
@@ -41,8 +39,6 @@
 import org.chromium.chrome.browser.sync.SyncServiceFactory;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.R;
-import org.chromium.chrome.test.util.browser.Features;
-import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 import org.chromium.components.browser_ui.settings.ChromeSwitchPreference;
 import org.chromium.components.browser_ui.settings.PlaceholderSettingsForTest;
 import org.chromium.components.prefs.PrefService;
@@ -64,9 +60,6 @@ public class PasswordSettingsTest {
             "PasswordManager.Settings.ToggleOfferToSavePasswords";
     private static final String AUTO_SIGNIN_HISTOGRAM = "PasswordManager.Settings.ToggleAutoSignIn";
 
-    @Rule
-    public TestRule mProcessor = new Features.InstrumentationProcessor();
-
     @Rule
     public SettingsActivityTestRule<PlaceholderSettingsForTest>
             mPlaceholderSettingsActivityTestRule =
@@ -347,11 +340,11 @@ public void testDoesNotDestroyPasswordCheckIfNotFirstInSettingsStack() {
     @Test
     @MediumTest
     @Feature({"Preferences"})
-    @EnableFeatures({ChromeFeatureList.UNIFIED_PASSWORD_MANAGER_LOCAL_PWD_MIGRATION_WARNING})
-    public void testLocalPasswordsMigrationSheetTriggered() {
+    public void testLocalPasswordsMigrationSheetTriggeredWhenShouldShow() {
         mTestHelper.setPasswordSourceWithMultipleEntries(PasswordSettingsTestHelper.GREEK_GODS);
         SettingsActivity activity = mTestHelper.startPasswordSettingsFromMainSettings(
                 mPasswordSettingsActivityTestRule);
+        mTestHelper.getHandler().setShouldShowWarning(true);
         waitForView(
                 withId(org.chromium.chrome.browser.pwd_migration.R.id.pwd_migration_warning_sheet),
                 ViewUtils.VIEW_VISIBLE);

chrome/browser/password_manager/android/all_passwords_bottom_sheet_controller.cc

@@ -64,7 +64,7 @@ AllPasswordsBottomSheetController::AllPasswordsBottomSheetController(
       dismissal_callback_(std::move(dismissal_callback)),
       focused_field_type_(focused_field_type),
       show_migration_warning_callback_(
-          base::BindRepeating(&password_manager::ShowWarning)) {
+          base::BindRepeating(&local_password_migration::ShowWarning)) {
   DCHECK(web_contents_);
   DCHECK(store_);
   DCHECK(dismissal_callback_);

chrome/browser/password_manager/android/local_passwords_migration_warning_util.cc

@@ -3,28 +3,59 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/password_manager/android/local_passwords_migration_warning_util.h"
+#include "base/time/time.h"
 #include "chrome/android/chrome_jni_headers/PasswordMigrationWarningBridge_jni.h"
 #include "chrome/browser/profiles/profile_android.h"
 #include "components/password_manager/core/common/password_manager_features.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "ui/android/window_android.h"
 
 using base::android::AttachCurrentThread;
 
-namespace password_manager {
+namespace local_password_migration {
+
+constexpr base::TimeDelta kMinIntervalBetweenWarnings = base::Days(30);
+
+void SaveWarningShownTimestamp(PrefService* pref_service) {
+  pref_service->SetTime(
+      password_manager::prefs::kLocalPasswordsMigrationWarningShownTimestamp,
+      base::Time::Now());
+}
+
 void ShowWarning(const gfx::NativeWindow window, Profile* profile) {
+  if (!ShouldShowWarning(profile)) {
+    return;
+  }
   Java_PasswordMigrationWarningBridge_showWarning(
       AttachCurrentThread(), window->GetJavaObject(),
       ProfileAndroid::FromProfile(profile)->GetJavaObject());
+  SaveWarningShownTimestamp(profile->GetPrefs());
 }
 
-bool ShouldShowWarning() {
+bool ShouldShowWarning(Profile* profile) {
   if (!base::FeatureList::IsEnabled(
           password_manager::features::
               kUnifiedPasswordManagerLocalPasswordsMigrationWarning)) {
     return false;
   }
-  // TODO(crbug.com/1451827): Implement the actual logic whether to show the
-  // warning here.
+
+  if (password_manager::features::kIgnoreMigrationWarningTimeout.Get()) {
+    return true;
+  }
+
+  PrefService* pref_service = profile->GetPrefs();
+  base::Time last_shown_timestamp = pref_service->GetTime(
+      password_manager::prefs::kLocalPasswordsMigrationWarningShownTimestamp);
+  base::TimeDelta time_since_last_shown =
+      base::Time::Now() - last_shown_timestamp;
+  if (time_since_last_shown < kMinIntervalBetweenWarnings) {
+    return false;
+  }
+
+  // TODO(crbug.com/1451827): Check whether the user is already syncing
+  // passwords.
   return true;
 }
-}  // namespace password_manager
+
+}  // namespace local_password_migration

chrome/browser/password_manager/android/local_passwords_migration_warning_util.h

@@ -8,9 +8,16 @@
 #include "chrome/browser/profiles/profile.h"
 #include "ui/gfx/native_widget_types.h"
 
-namespace password_manager {
+namespace local_password_migration {
+
+// Shows the local password migration warning.
 void ShowWarning(const gfx::NativeWindow window, Profile* profile);
-bool ShouldShowWarning();
-}
+
+// Returns whether the UPM local passwords migration warning should be
+// displayed. `profile` is used to retrieve necessary services for checking
+// the conditions.
+bool ShouldShowWarning(Profile* profile);
+
+}  // namespace local_password_migration
 
 #endif  // CHROME_BROWSER_PASSWORD_MANAGER_ANDROID_LOCAL_PASSWORDS_MIGRATION_WARNING_UTIL_H_

chrome/browser/password_manager/android/local_passwords_migration_warning_util_unittest.cc

@@ -0,0 +1,67 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/password_manager/android/local_passwords_migration_warning_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/password_manager/core/common/password_manager_features.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class LocalPasswordsMigrationWarningUtilTest : public testing::Test {
+ protected:
+  LocalPasswordsMigrationWarningUtilTest() = default;
+  ~LocalPasswordsMigrationWarningUtilTest() override = default;
+
+  sync_preferences::TestingPrefServiceSyncable* pref_service() {
+    return profile_.GetTestingPrefService();
+  }
+
+  Profile* profile() { return &profile_; }
+
+  base::test::TaskEnvironment* task_env() { return &task_env_; }
+
+ private:
+  content::BrowserTaskEnvironment task_env_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
+  TestingProfile profile_;
+};
+
+TEST_F(LocalPasswordsMigrationWarningUtilTest,
+       TestShouldNotShowWhenFeatureDisabled) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      password_manager::features::
+          kUnifiedPasswordManagerLocalPasswordsMigrationWarning);
+  EXPECT_FALSE(local_password_migration::ShouldShowWarning(profile()));
+}
+
+TEST_F(LocalPasswordsMigrationWarningUtilTest,
+       TestShouldShowWhenMoreThanAMonth) {
+  base::test::ScopedFeatureList scoped_feature_list(
+      password_manager::features::
+          kUnifiedPasswordManagerLocalPasswordsMigrationWarning);
+  pref_service()->SetTime(
+      password_manager::prefs::kLocalPasswordsMigrationWarningShownTimestamp,
+      base::Time::Now());
+  task_env()->FastForwardBy(base::Days(31));
+  EXPECT_TRUE(local_password_migration::ShouldShowWarning(profile()));
+}
+
+TEST_F(LocalPasswordsMigrationWarningUtilTest,
+       TestShouldNotShowWhenLessThanAMonth) {
+  base::test::ScopedFeatureList scoped_feature_list(
+      password_manager::features::
+          kUnifiedPasswordManagerLocalPasswordsMigrationWarning);
+  pref_service()->SetTime(
+      password_manager::prefs::kLocalPasswordsMigrationWarningShownTimestamp,
+      base::Time::Now());
+  task_env()->FastForwardBy(base::Days(29));
+  EXPECT_FALSE(local_password_migration::ShouldShowWarning(profile()));
+}

chrome/browser/password_manager/android/password_accessory_controller_impl.cc

@@ -296,7 +296,7 @@ void PasswordAccessoryControllerImpl::CreateForWebContents(
             web_contents, credential_cache, nullptr,
             ChromePasswordManagerClient::FromWebContents(web_contents),
             base::BindRepeating(GetPasswordManagerDriver),
-            base::BindRepeating(&password_manager::ShowWarning))));
+            base::BindRepeating(&local_password_migration::ShowWarning))));
   }
 }
 

chrome/browser/password_manager/android/password_ui_view_android.cc

@@ -27,6 +27,7 @@
 #include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "chrome/android/chrome_jni_headers/PasswordUIView_jni.h"
+#include "chrome/browser/password_manager/android/local_passwords_migration_warning_util.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/password_manager/core/browser/export/password_csv_writer.h"
@@ -301,6 +302,11 @@ jboolean JNI_PasswordUIView_HasAccountForLeakCheckRequest(JNIEnv* env) {
       identity_manager);
 }
 
+jboolean JNI_PasswordUIView_ShouldShowMigrationWarning(JNIEnv* env) {
+  return local_password_migration::ShouldShowWarning(
+      ProfileManager::GetLastUsedProfile());
+}
+
 // static
 static jlong JNI_PasswordUIView_Init(JNIEnv* env,
                                      const JavaParamRef<jobject>& obj) {

chrome/browser/password_manager/android/save_update_password_message_delegate.cc

@@ -112,7 +112,7 @@ void TryToShowPasswordMigrationWarning(
 SaveUpdatePasswordMessageDelegate::SaveUpdatePasswordMessageDelegate()
     : SaveUpdatePasswordMessageDelegate(
           base::BindRepeating(PasswordEditDialogBridge::Create),
-          base::BindRepeating(&password_manager::ShowWarning)) {}
+          base::BindRepeating(&local_password_migration::ShowWarning)) {}
 
 SaveUpdatePasswordMessageDelegate::SaveUpdatePasswordMessageDelegate(
     PasswordEditDialogFactory password_edit_dialog_factory,

chrome/browser/touch_to_fill/touch_to_fill_controller_autofill_delegate.cc

@@ -82,7 +82,7 @@ TouchToFillControllerAutofillDelegate::TouchToFillControllerAutofillDelegate(
       webauthn_delegate_(webauthn_delegate),
       filler_(std::move(filler)),
       show_password_migration_warning_(
-          base::BindRepeating(&password_manager::ShowWarning)),
+          base::BindRepeating(&local_password_migration::ShowWarning)),
       should_show_hybrid_option_(should_show_hybrid_option),
       source_id_(password_client->web_contents()
                      ->GetPrimaryMainFrame()
@@ -254,8 +254,10 @@ void TouchToFillControllerAutofillDelegate::FillCredential(
 
   // Do not trigger autosubmission if the password migration warning is being
   // shown because it interrupts the nomal workflow.
-  filler_->UpdateTriggerSubmission(ShouldTriggerSubmission() &&
-                                   !password_manager::ShouldShowWarning());
+  filler_->UpdateTriggerSubmission(
+      ShouldTriggerSubmission() &&
+      !local_password_migration::ShouldShowWarning(
+          Profile::FromBrowserContext(web_contents_->GetBrowserContext())));
   filler_->FillUsernameAndPassword(credential.username(),
                                    credential.password());
   ShowPasswordMigrationWarningIfNeeded();
@@ -279,7 +281,8 @@ void TouchToFillControllerAutofillDelegate::CleanUpFillerAndReportOutcome(
 
 void TouchToFillControllerAutofillDelegate::
     ShowPasswordMigrationWarningIfNeeded() {
-  if (!password_manager::ShouldShowWarning()) {
+  if (!local_password_migration::ShouldShowWarning(
+          Profile::FromBrowserContext(web_contents_->GetBrowserContext()))) {
     return;
   }
   show_password_migration_warning_.Run(

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

@@ -95,7 +95,7 @@ WeakPtr<AutofillPopupControllerImpl> AutofillPopupControllerImpl::GetOrCreate(
 #if BUILDFLAG(IS_ANDROID)
   AutofillPopupControllerImpl* controller = new AutofillPopupControllerImpl(
       delegate, web_contents, container_view, element_bounds, text_direction,
-      base::BindRepeating(&password_manager::ShowWarning));
+      base::BindRepeating(&local_password_migration::ShowWarning));
 #else
   AutofillPopupControllerImpl* controller = new AutofillPopupControllerImpl(
       delegate, web_contents, container_view, element_bounds, text_direction,

chrome/test/BUILD.gn

@@ -6863,6 +6863,7 @@ test("unit_tests") {
       "../browser/optimization_guide/android/optimization_guide_bridge_unittest.cc",
       "../browser/optimization_guide/android/optimization_guide_tab_url_provider_android_unittest.cc",
       "../browser/page_load_metrics/observers/android_page_load_metrics_observer_unittest.cc",
+      "../browser/password_manager/android/local_passwords_migration_warning_util_unittest.cc",
       "../browser/password_manager/android/password_ui_view_android_unittest.cc",
       "../browser/permissions/notification_blocked_message_delegate_android_unittest.cc",
       "../browser/permissions/permission_prompt_android_unittest.cc",

components/password_manager/core/browser/password_manager.cc

@@ -329,6 +329,8 @@ void PasswordManager::RegisterProfilePrefs(
       prefs::kTimesAttemptedToReenrollToGoogleMobileServices, 0);
   registry->RegisterBooleanPref(
       prefs::kUserAcknowledgedLocalPasswordsMigrationWarning, false);
+  registry->RegisterTimePref(
+      prefs::kLocalPasswordsMigrationWarningShownTimestamp, base::Time());
 #endif
   // Preferences for |PasswordChangeSuccessTracker|.
   registry->RegisterIntegerPref(prefs::kPasswordChangeSuccessTrackerVersion, 0);

components/password_manager/core/common/password_manager_features.h

@@ -149,6 +149,12 @@ inline constexpr base::FeatureParam<UpmExperimentVariation>
                                  &kUpmExperimentVariationOption};
 
 extern const base::FeatureParam<int> kSaveUpdatePromptSyncingStringVersion;
+
+// Whether to ignore the 1 month timeout in between migration warning prompts.
+// Used for manual testing.
+inline constexpr base::FeatureParam<bool> kIgnoreMigrationWarningTimeout = {
+    &kUnifiedPasswordManagerLocalPasswordsMigrationWarning,
+    "ignore_migration_warning_timeout", false};
 #endif
 
 // Field trial and corresponding parameters.