diff --git a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_browsertest_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_browsertest_win.cc index 9aab983b5e79f..fa879fee558e5 100644 --- a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_browsertest_win.cc +++ b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_browsertest_win.cc @@ -13,9 +13,13 @@ #include "chrome/browser/lifetime/scoped_keep_alive.h" #include "chrome/browser/profiles/profile_manager.h" #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h" +#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h" #include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/browser_window.h" #include "chrome/test/base/in_process_browser_test.h" +#include "components/component_updater/pref_names.h" +#include "components/variations/variations_params_manager.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -27,6 +31,8 @@ using ::testing::InvokeWithoutArgs; using ::testing::StrictMock; using ::testing::Return; +constexpr char kSRTPromptGroup[] = "SRTGroup"; + class MockChromeCleanerPromptDelegate : public ChromeCleanerPromptDelegate { public: MOCK_METHOD3(ShowChromeCleanerPrompt, @@ -52,8 +58,24 @@ class MockChromeCleanerController MOCK_METHOD0(Reboot, void()); }; -class ChromeCleanerPromptUserTest : public InProcessBrowserTest { +// Parameters for this test: +// - const char* old_seed_: The old "Seed" Finch parameter saved in prefs. +// - const char* incoming_seed_: The new "Seed" Finch parameter. +class ChromeCleanerPromptUserTest + : public InProcessBrowserTest, + public ::testing::WithParamInterface< + testing::tuple> { public: + ChromeCleanerPromptUserTest() { + std::tie(old_seed_, incoming_seed_) = GetParam(); + } + + void SetUpCommandLine(base::CommandLine* command_line) override { + variations::testing::VariationParamsManager::AppendVariationParams( + kSRTPromptTrial, kSRTPromptGroup, {{"Seed", incoming_seed_}}, + command_line); + } + void SetUpInProcessBrowserTestFixture() override { // dialog_controller_ expects that the cleaner controller would be on // scanning state. @@ -67,19 +89,35 @@ class ChromeCleanerPromptUserTest : public InProcessBrowserTest { dialog_controller_->SetPromptDelegateForTests(&mock_delegate_); } + void SetUpOnMainThread() override { + chrome::FindLastActive()->profile()->GetPrefs()->SetString( + prefs::kSwReporterPromptSeed, old_seed_); + } + + void TearDownOnMainThread() override { + bool expect_seed_changed = + !incoming_seed_.empty() && (incoming_seed_ != old_seed_); + EXPECT_EQ(expect_seed_changed ? incoming_seed_ : old_seed_, + chrome::FindLastActive()->profile()->GetPrefs()->GetString( + prefs::kSwReporterPromptSeed)); + } + protected: MockChromeCleanerController mock_cleaner_controller_; ChromeCleanerDialogControllerImpl* dialog_controller_; StrictMock mock_delegate_; + + std::string old_seed_; + std::string incoming_seed_; }; -IN_PROC_BROWSER_TEST_F(ChromeCleanerPromptUserTest, +IN_PROC_BROWSER_TEST_P(ChromeCleanerPromptUserTest, OnInfectedBrowserAvailable) { EXPECT_CALL(mock_delegate_, ShowChromeCleanerPrompt(_, _, _)).Times(1); dialog_controller_->OnInfected(std::set()); } -IN_PROC_BROWSER_TEST_F(ChromeCleanerPromptUserTest, +IN_PROC_BROWSER_TEST_P(ChromeCleanerPromptUserTest, DISABLED_OnInfectedBrowserNotAvailable) { browser()->window()->Minimize(); base::RunLoop().RunUntilIdle(); @@ -95,7 +133,7 @@ IN_PROC_BROWSER_TEST_F(ChromeCleanerPromptUserTest, run_loop.Run(); } -IN_PROC_BROWSER_TEST_F(ChromeCleanerPromptUserTest, AllBrowsersClosed) { +IN_PROC_BROWSER_TEST_P(ChromeCleanerPromptUserTest, AllBrowsersClosed) { std::unique_ptr keep_alive = base::MakeUnique(KeepAliveOrigin::BROWSER, KeepAliveRestartOption::DISABLED); @@ -114,5 +152,12 @@ IN_PROC_BROWSER_TEST_F(ChromeCleanerPromptUserTest, AllBrowsersClosed) { run_loop.Run(); } +INSTANTIATE_TEST_CASE_P( + WithVaryingSeeds, + ChromeCleanerPromptUserTest, + ::testing::Combine( + ::testing::Values("", "Seed1"), // old_seed_ + ::testing::Values("", "Seed1", "Seed2"))); // incoming_seed + } // namespace } // namespace safe_browsing diff --git a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc index 52dbed65ddc29..d0fb3c9f9a3b8 100644 --- a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc +++ b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_win.cc @@ -7,11 +7,14 @@ #include "base/metrics/histogram_macros.h" #include "base/metrics/user_metrics.h" #include "base/metrics/user_metrics_action.h" +#include "chrome/browser/profiles/profile.h" #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_navigation_util_win.h" #include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_dialogs.h" #include "chrome/browser/ui/browser_list.h" +#include "components/component_updater/pref_names.h" +#include "components/prefs/pref_service.h" #include "ui/base/window_open_disposition.h" namespace safe_browsing { @@ -232,6 +235,19 @@ void ChromeCleanerDialogControllerImpl::SetPromptDelegateForTests( } void ChromeCleanerDialogControllerImpl::ShowChromeCleanerPrompt() { + DCHECK(browser_); + Profile* profile = browser_->profile(); + DCHECK(profile); + PrefService* prefs = profile->GetPrefs(); + DCHECK(prefs); + + // Don't show the prompt again if it's been shown before for this profile and + // for the current variations seed. + const std::string incoming_seed = GetIncomingSRTSeed(); + const std::string old_seed = prefs->GetString(prefs::kSwReporterPromptSeed); + if (!incoming_seed.empty() && incoming_seed != old_seed) + prefs->SetString(prefs::kSwReporterPromptSeed, incoming_seed); + prompt_delegate_->ShowChromeCleanerPrompt(browser_, this, cleaner_controller_); dialog_shown_ = true; diff --git a/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc index b0a6bafe111ee..acce667e66a8c 100644 --- a/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc +++ b/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc @@ -45,8 +45,8 @@ namespace safe_browsing { namespace { -// Parameter for this test: -// - bool in_browser_cleaner_ui: indicates if InBrowserCleanerUI feature +// Parameters for this test: +// - bool in_browser_cleaner_ui_: indicates if InBrowserCleanerUI feature // is enabled; // // We expect that the reporter's logic should remain unchanged even when the @@ -312,6 +312,8 @@ IN_PROC_BROWSER_TEST_P(ReporterRunnerTest, NothingFound) { } IN_PROC_BROWSER_TEST_P(ReporterRunnerTest, CleanupNeeded) { + bool expect_prompt = incoming_seed_.empty() || (incoming_seed_ != old_seed_); + RunReporter(chrome_cleaner::kSwReporterCleanupNeeded); ExpectReporterLaunches(0, 1, true); ExpectToRunAgain(kDaysBetweenSuccessfulSwReporterRuns); diff --git a/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.cc index 04b9e56dcedd1..8b89377732838 100644 --- a/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.cc +++ b/chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_win.cc @@ -607,7 +607,8 @@ void MaybeScanAndPrompt(const SwReporterInvocation& reporter_invocation) { DCHECK(prefs); // Don't show the prompt again if it's been shown before for this profile and - // for the current variations seed. + // for the current variations seed. The seed preference will be updated once + // the prompt is shown. const std::string incoming_seed = GetIncomingSRTSeed(); const std::string old_seed = prefs->GetString(prefs::kSwReporterPromptSeed); if (!incoming_seed.empty() && incoming_seed == old_seed) { @@ -616,9 +617,6 @@ void MaybeScanAndPrompt(const SwReporterInvocation& reporter_invocation) { return; } - if (!incoming_seed.empty() && incoming_seed != old_seed) - prefs->SetString(prefs::kSwReporterPromptSeed, incoming_seed); - if (g_testing_delegate_) { g_testing_delegate_->TriggerPrompt(); return;