Sends switches to the Software Reporter to enable matching data collection.

chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
index e77870481184d9455df698b491d7641195865259..0cead7216bfc17d30389286202162eb0ebd785e2 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc
@@ -10,11 +10,15 @@
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
@@ -56,13 +60,19 @@ class SRTFetcherTest : public InProcessBrowserTest,
 
   void NotifyReporterDone() override { reporter_done_notified_ = true; }
 
-  void SetDaysSinceLastReport(int days) {
+  // Sets |path| in the local state to a date corresponding to |days| days ago.
+  void SetDateInLocalState(const std::string& path, int days) {
     PrefService* local_state = g_browser_process->local_state();
-    local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
+    DCHECK_NE(local_state, nullptr);
+    local_state->SetInt64(path,
                           (base::Time::Now() - base::TimeDelta::FromDays(days))
                               .ToInternalValue());
   }
 
+  void SetDaysSinceLastReport(int days) {
+    SetDateInLocalState(prefs::kSwReporterLastTimeTriggered, days);
+  }
+
   void ExpectToRunAgain(int days) {
     ASSERT_TRUE(task_runner_->HasPendingTask());
     EXPECT_LE(task_runner_->NextPendingTaskDelay(),
@@ -71,6 +81,39 @@ class SRTFetcherTest : public InProcessBrowserTest,
               base::TimeDelta::FromDays(days) - base::TimeDelta::FromHours(1));
   }
 
+  // Sets local state for last time the software reporter sent logs to |days|
+  // days ago.
+  void SetLastTimeSentReport(int days) {
+    SetDateInLocalState(prefs::kSwReporterLastTimeSentReport, days);
+  }
+
+  int64_t GetLastTimeSentReport() {
+    const PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    DCHECK(local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport));
+    return local_state->GetInt64(prefs::kSwReporterLastTimeSentReport);
+  }
+
+  void ExpectLastTimeSentReportNotSet() {
+    PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    EXPECT_FALSE(
+        local_state->HasPrefPath(prefs::kSwReporterLastTimeSentReport));
+  }
+
+  void ExpectLastReportSentAsToday() {
+    const PrefService* local_state = g_browser_process->local_state();
+    DCHECK_NE(local_state, nullptr);
+    const base::Time now = base::Time::Now();
+    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
+        local_state->GetInt64(prefs::kSwReporterLastTimeSentReport));
+
+    // Checks if the last time set logs is set as no more than one hour ago,
+    // which should be enough time if the execution does not fail.
+    EXPECT_LT(now - base::TimeDelta::FromHours(1), last_time_sent_logs);

[csharp, 2016/09/02 21:16:25]

This doesn't seem to quite line up with the function name. Could you change one
to keep them in sync.

This also makes me wonder if we should make now() mockable for this test, but
I'm not sure it is worth opening that can of worms

[ftirelo, 2016/09/06 16:37:43]

Done.

+    EXPECT_LT(last_time_sent_logs, now);
+  }
+
   void TestReporterLaunchCycle(int expected_launch_count,
                                const base::FilePath& expected_launch_path) {
     // This test has an unfortunate amount of knowledge of the internals of
@@ -150,6 +193,32 @@ class SRTFetcherTest : public InProcessBrowserTest,
     reporter_done_notified_ = false;
   }
 
+  // Expects |reporter_launch_parameters_| contain exactly the command line

[csharp, 2016/09/02 21:16:25]

nit: contain -> to contain(?)

[ftirelo, 2016/09/06 16:37:43]

Done.

+  // switches specified in |expected_switches|.
+  void ExpectLoggingSwitches(const std::set<std::string>& expected_switches) {
+    const base::CommandLine::SwitchMap& invocation_switches =

[csharp, 2016/09/02 21:16:25]

What about using std::set_symmetric_difference

[ftirelo, 2016/09/06 16:37:43]

std::set_symmetric_different relies on the order of the sets. Since we have no
control on how CommandLine::SwitchesMap is implemented (today it's a map, but
there is no guarantee it will not be changed to an unordered_map in the future),
I'd rather not create that dependency now.

Another difficulty in making the change, is that we are not really comparing
collections of the same type (one is a map the other is a set). The function
above will also require either a comparator function or an adapter for the map
iterator (none is available in base/), and I'm not sure if the final outcome
will be worth the added complexity.

I simplified the code though, by checking first if both have the same size.

+        reporter_launch_parameters_.command_line.GetSwitches();
+    // Checks if all expected switches are in the invocation switches.
+    for (const std::string& expected_switch : expected_switches) {
+      EXPECT_NE(invocation_switches.find(expected_switch),
+                invocation_switches.end());
+    }
+    // Checks if all invocation switches are in the expected switches.
+    for (const auto& invocation_switch : invocation_switches) {
+      EXPECT_NE(expected_switches.find(invocation_switch.first),
+                expected_switches.end());
+    }
+  }
+
+  void EnableSBExtendedReporting() {
+    Browser* browser = chrome::FindLastActive();
+    ASSERT_NE(browser, nullptr);
+    Profile* profile = browser->profile();
+    ASSERT_NE(profile, nullptr);
+    profile->GetPrefs()->SetBoolean(
+        prefs::kSafeBrowsingExtendedReportingEnabled, true);
+  }
+
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   bool prompt_trigger_called_ = false;
   int reporter_launch_count_ = 0;
@@ -306,4 +375,87 @@ IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ParameterChange) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_FeatureDisabled) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches({/*expect no switches*/});
+  ExpectLastTimeSentReportNotSet();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches({/*expect no switches*/});
+  ExpectLastTimeSentReportNotSet();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledFirstRun) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  EnableSBExtendedReporting();
+  // Note: don't set last time sent logs in the local state.
+  // SBER is enabled and there is no record in the local state of the last time
+  // logs have been sent, so we should send logs in this run.
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(
+      std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch,

[csharp, 2016/09/02 21:16:25]

Could you make this set a const?

[ftirelo, 2016/09/06 16:37:43]

Done.

+                            kChromeVersionSwitch, kChromeChannelSwitch});
+  ExpectLastReportSentAsToday();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledNoRecentLogging) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  // SBER is enabled and last time logs were sent was more than
+  // |kDaysBetweenReporterLogsSent| day ago, so we should send logs in this run.
+  EnableSBExtendedReporting();
+  SetLastTimeSentReport(kDaysBetweenReporterLogsSent + 3);
+  RunReporter();
+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(
+      std::set<std::string>{kExtendedSafeBrowsingEnabledSwitch,
+                            kChromeVersionSwitch, kChromeChannelSwitch});
+  ExpectLastReportSentAsToday();
+}
+
+IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_EnabledRecentlyLogged) {
+  exit_code_to_report_ = kSwReporterNothingFound;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      kSwReporterExtendedSafeBrowsingFeature);
+  // SBER is enabled, but logs have been sent less than
+  // |kDaysBetweenReporterLogsSent| day ago, so we shouldn't send any logs in
+  // this run.
+  EnableSBExtendedReporting();
+  SetLastTimeSentReport(kDaysBetweenReporterLogsSent - 1);
+  int64_t last_time_sent_logs = GetLastTimeSentReport();
+  RunReporter();

[csharp, 2016/09/02 21:16:25]

All of your tests have this same four middle lines, make a helper function?

Or maybe you could just use TestReporterLaunchCycle

[ftirelo, 2016/09/06 16:37:42]

Done.

+  task_runner_->RunPendingTasks();
+  EXPECT_EQ(1, reporter_launch_count_);
+  base::RunLoop().RunUntilIdle();
+  ExpectLoggingSwitches(std::set<std::string>{/*expect no switches*/});
+  EXPECT_EQ(last_time_sent_logs, GetLastTimeSentReport());
+}
+
 }  // namespace safe_browsing