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

chrome/browser/safe_browsing/srt_fetcher_win.cc

Index: chrome/browser/safe_browsing/srt_fetcher_win.cc
diff --git a/chrome/browser/safe_browsing/srt_fetcher_win.cc b/chrome/browser/safe_browsing/srt_fetcher_win.cc
index d94c426cb6e7249db95e0c0f14d9b1b2d811906e..5e4937d95a19efacfd6de30fe392ada1cbd3d0fd 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.cc
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.cc
@@ -6,6 +6,7 @@
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <memory>
 #include <vector>
 
@@ -13,6 +14,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
@@ -29,16 +31,19 @@
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/browser/safe_browsing/srt_client_info_win.h"
 #include "chrome/browser/safe_browsing/srt_global_error_win.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
+#include "chrome/common/pref_names.h"
 #include "components/component_updater/pref_names.h"
 #include "components/prefs/pref_service.h"
 #include "components/rappor/rappor_service.h"
 #include "components/variations/net/variations_http_headers.h"
+#include "components/version_info/version_info.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
@@ -58,6 +63,9 @@ const wchar_t kCleanerSubKey[] = L"Cleaner";
 const wchar_t kEndTimeValueName[] = L"EndTime";
 const wchar_t kStartTimeValueName[] = L"StartTime";
 
+const char kExtendedSafeBrowsingEnabledSwitch[] =
+    "extended-safebrowsing-enabled";
+
 namespace {
 
 // Used to send UMA information about missing start and end time registry
@@ -115,6 +123,9 @@ const char kFoundUwsReadErrorMetricName[] =
 const char kScanTimesMetricName[] = "SoftwareReporter.UwSScanTimes";
 const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
 
+const base::Feature kSwReporterExtendedSafeBrowsingFeature{
+    "SwReporterExtendedSafeBrowsingFeature", base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
 class UMAHistogramReporter {
  public:
@@ -354,6 +365,20 @@ void DisplaySRTPrompt(const base::FilePath& download_path) {
     global_error->ShowBubbleView(browser);
 }
 
+// Returns true if there is a profile that is not in incognito mode and the user
+// has opted into Safe Browsing extended reporting.
+bool SafeBrowsingExtendedReportingEnabled() {
+  BrowserList* browser_list = BrowserList::GetInstance();
+  return std::any_of(browser_list->begin_last_active(),
+                     browser_list->end_last_active(),
+                     [](const Browser* browser) {
+                       const Profile* profile = browser->profile();

[csharp, 2016/08/30 13:39:08]

What about converting this lambda to a helper
function,BrowserIsSafeBrowsingExtended? (or something)

(It seems just a little big for a lambda)

[ftirelo, 2016/08/30 21:21:48]

Done.

+                       return profile && !profile->IsOffTheRecord() &&
+                              profile->GetPrefs()->GetBoolean(
+                                  prefs::kSafeBrowsingExtendedReportingEnabled);
+                     });
+}
+
 // This function is called from a worker thread to launch the SwReporter and
 // wait for termination to collect its exit code. This task could be
 // interrupted by a shutdown at any time, so it shouldn't depend on anything
@@ -549,6 +574,7 @@ class ReporterRunner : public chrome::BrowserListObserver {
   // be resilient to unexpected shutdown.
   void ReporterDone(const base::Time& reporter_start_time,
                     const base::Version& version,
+                    bool logging_enabled,
                     int exit_code) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -576,6 +602,10 @@ class ReporterRunner : public chrome::BrowserListObserver {
       local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
+      if (logging_enabled) {
+        local_state->SetInt64(prefs::kSwReporterLastTimeSentReport,

[csharp, 2016/08/30 13:39:08]

I wonder if this should be set right before the reporter is called. Setting it
here could result in a user uploading logs but Chrome stopping before this value
is set, so they'd run it again too quickly.

Although if we set it before the reporter runs some times the reporter would
stop before it uploads, so we'd miss some cases. But missing some cases seems
like it might be better than getting too many. WDYT?

[ftirelo, 2016/08/30 21:21:49]

Done.

+                              base::Time::Now().ToInternalValue());
+      }
     }
     uma.ReportRuntime(reporter_running_time);
     uma.ReportScanTimes();
@@ -639,15 +669,27 @@ class ReporterRunner : public chrome::BrowserListObserver {
       if (g_testing_delegate_)
         g_testing_delegate_->NotifyLaunchReady();
 
+      // Creating a new invocation to add switches for users who opted into

[csharp, 2016/08/30 13:39:08]

Could you add some tests for this?

[ftirelo, 2016/08/30 21:21:49]

Done.

+      // extended Safe Browsing reporting. The invocation object is changed
+      // locally right before the actual process is launched because user status
+      // can between this and the next run for this ReporterRunner object. For
+      // example, the ReporterDone() callback schedules the next run for a few
+      // days later, and the user might have changed settings in the meantime.
+      SwReporterInvocation actual_invocation =

[csharp, 2016/08/30 13:39:08]

I think you want to create a copy constructor and use it here, since this will
drop all the fields but the command line.

[ftirelo, 2016/08/30 21:21:49]

Done.

+          SwReporterInvocation::FromCommandLine(invocation_.command_line);
+      bool logging_enabled = ShouldSendReporterLogs(*local_state);
+      if (logging_enabled)
+        AddSwitchesForExtendedReporterUser(&actual_invocation);
+
       // It's OK to simply |PostTaskAndReplyWithResult| so that
       // |LaunchAndWaitForExit| doesn't need to access
       // |main_thread_task_runner_| since the callback is not delayed and the
       // test task runner won't need to force it.
       base::PostTaskAndReplyWithResult(
           blocking_task_runner_.get(), FROM_HERE,
-          base::Bind(&LaunchAndWaitForExit, invocation_),
+          base::Bind(&LaunchAndWaitForExit, actual_invocation),
           base::Bind(&ReporterRunner::ReporterDone, base::Unretained(this),
-                     base::Time::Now(), version_));
+                     base::Time::Now(), version_, logging_enabled));
     } else {
       main_thread_task_runner_->PostDelayedTask(
           FROM_HERE,
@@ -656,6 +698,35 @@ class ReporterRunner : public chrome::BrowserListObserver {
     }
   }
 
+  // Returns true if the experiment to send reporter logs is enabled, the user
+  // opted into Safe Browsing extended reporting, and logs have been sent at
+  // least |kSwReporterLastTimeSentReport| days ago.
+  bool ShouldSendReporterLogs(const PrefService& local_state) {
+    if (!base::FeatureList::IsEnabled(kSwReporterExtendedSafeBrowsingFeature) ||
+        !SafeBrowsingExtendedReportingEnabled()) {
+      return false;
+    }
+
+    const base::Time now = base::Time::Now();
+    const base::Time last_time_sent_logs = base::Time::FromInternalValue(
+        local_state.GetInt64(prefs::kSwReporterLastTimeSentReport));
+    // Send the logs if the last send was in the future.

[csharp, 2016/08/30 13:39:08]

nit: Could you explain why we care about this a bit?

[ftirelo, 2016/08/30 21:21:49]

Done.

+    if (last_time_sent_logs > now)
+      return true;
+    // Otherwise, send them if the interval has passed.
+    return last_time_sent_logs +
+               base::TimeDelta::FromDays(kDaysBetweenReporterLogsSent) <=
+           now;
+  }
+
+  void AddSwitchesForExtendedReporterUser(SwReporterInvocation* invocation) {
+    invocation->command_line.AppendSwitch(kExtendedSafeBrowsingEnabledSwitch);
+    invocation->command_line.AppendSwitchASCII(
+        kChromeVersionSwitch, version_info::GetVersionNumber());
+    invocation->command_line.AppendSwitchNative(
+        kChromeChannelSwitch, base::IntToString16(ChannelAsInt()));
+  }
+
   bool first_run_ = true;
   SwReporterInvocation invocation_;
   base::Version version_;