Chrome Cleaner UI: record prompt shown only if prompt is shown

Bug: 763943
Change-Id: Ic16aa5ef180f8e8607e9f161f3f11a5e68ba0b63
Reviewed-on: https://chromium-review.googlesource.com/660399
Commit-Queue: Fabio Tirelo <[email protected]>
Reviewed-by: Chris Sharp <[email protected]>
Reviewed-by: Ali Tofigh <[email protected]>
Cr-Commit-Position: refs/heads/master@{#501024}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_impl_browsertest_win.cc

@@ -11,11 +11,15 @@
 #include "base/run_loop.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/keep_alive_registry/keep_alive_types.h"
 #include "components/keep_alive_registry/scoped_keep_alive.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<const char*, const char*>> {
  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<MockChromeCleanerPromptDelegate> 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<base::FilePath>());
 }
 
-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<ScopedKeepAlive> keep_alive =
       base::MakeUnique<ScopedKeepAlive>(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

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;

chrome/browser/safe_browsing/chrome_cleaner/reporter_runner_browsertest_win.cc

@@ -50,7 +50,7 @@ namespace {
 
 constexpr char kSRTPromptGroup[] = "SRTGroup";
 
-// Parameter for this test:
+// Parameters for this test:
 //  - bool in_browser_cleaner_ui_: indicates if InBrowserCleanerUI feature
 //    is enabled;
 //
@@ -333,8 +333,7 @@ IN_PROC_BROWSER_TEST_P(ReporterRunnerTest, NothingFound) {
 }
 
 IN_PROC_BROWSER_TEST_P(ReporterRunnerTest, CleanupNeeded) {
-  bool expect_prompt =
-      incoming_seed_.empty() ? true : incoming_seed_ != old_seed_;
+  bool expect_prompt = incoming_seed_.empty() || (incoming_seed_ != old_seed_);
 
   RunReporter(chrome_cleaner::kSwReporterCleanupNeeded);
   ExpectReporterLaunches(0, 1, expect_prompt);

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;