Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_win.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/srt_fetcher_win.h"
 
 #include <stdint.h>
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/metrics/field_trial.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
@@ skipping 79 matching lines @@
 const wchar_t kFoundUwsValueName[] = L"FoundUws";
 const wchar_t kMemoryUsedValueName[] = L"MemoryUsed";
 
 const char kFoundUwsMetricName[] = "SoftwareReporter.FoundUwS";
 const char kFoundUwsReadErrorMetricName[] =
     "SoftwareReporter.FoundUwSReadError";
 const char kScanTimesMetricName[] = "SoftwareReporter.UwSScanTimes";
 const char kMemoryUsedMetricName[] = "SoftwareReporter.MemoryUsed";
 
 // Reports metrics about the software reporter via UMA (and sometimes Rappor).
+//
+// This will format the names of the histograms at runtime by adding an

[csharp, 2016/08/25 19:47:29]

nit: As discussed, could you move this close to the functions that actually do
this magic?

[Joe Mason, 2016/08/25 22:03:20]

Done.

+// optional suffix, so the UMA_HISTOGRAM macros won't work. Use RecordHistogram
+// and related helper methods instead.
 class UMAHistogramReporter {
  public:
+  explicit UMAHistogramReporter(const std::string& suffix = std::string())
+      : suffix_(suffix),
+        registry_key_(suffix.empty() ? kSoftwareRemovalToolRegistryKey
+                                     : base::StringPrintf(
+                                           L"%ls\\%ls",
+                                           kSoftwareRemovalToolRegistryKey,
+                                           base::UTF8ToUTF16(suffix).c_str())) {
+  }
+
   // Reports the software reporter tool's version via UMA.
   void ReportVersion(const base::Version& version) const {
     DCHECK(!version.components().empty());
     // The minor version is the 2nd last component of the version,
     // or just the first component if there is only 1.
     uint32_t minor_version = 0;
     if (version.components().size() > 1)
       minor_version = version.components()[version.components().size() - 2];
     else
       minor_version = version.components()[0];
-    UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.MinorVersion", minor_version);
+    RecordSparseHistogram("SoftwareReporter.MinorVersion", minor_version);
 
     // The major version for X.Y.Z is X*256^3+Y*256+Z. If there are additional
     // components, only the first three count, and if there are less than 3, the
     // missing values are just replaced by zero. So 1 is equivalent 1.0.0.
     DCHECK_LT(version.components()[0], 0x100U);
     uint32_t major_version = 0x1000000 * version.components()[0];
     if (version.components().size() >= 2) {
       DCHECK_LT(version.components()[1], 0x10000U);
       major_version += 0x100 * version.components()[1];
     }
     if (version.components().size() >= 3) {
       DCHECK_LT(version.components()[2], 0x100U);
       major_version += version.components()[2];
     }
-    UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.MajorVersion", major_version);
+    RecordSparseHistogram("SoftwareReporter.MajorVersion", major_version);
   }
 
   void ReportExitCode(int exit_code) const {
-    UMA_HISTOGRAM_SPARSE_SLOWLY("SoftwareReporter.ExitCode", exit_code);
+    RecordSparseHistogram("SoftwareReporter.ExitCode", exit_code);
   }
 
   // Reports UwS found by the software reporter tool via UMA and RAPPOR.
-  void ReportFoundUwS() const {
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER,
-                                   kSoftwareRemovalToolRegistryKey,
+  void ReportFoundUwS(bool use_rappor) const {
+    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
                                    KEY_QUERY_VALUE | KEY_SET_VALUE);
     std::vector<base::string16> found_uws_strings;
     if (reporter_key.Valid() &&
         reporter_key.ReadValues(kFoundUwsValueName, &found_uws_strings) ==
             ERROR_SUCCESS) {
-      rappor::RapporService* rappor_service =
-          g_browser_process->rappor_service();
+      rappor::RapporService* rappor_service = nullptr;
+      if (use_rappor)
+        rappor_service = g_browser_process->rappor_service();
 
       bool parse_error = false;
       for (const base::string16& uws_string : found_uws_strings) {
         // All UwS ids are expected to be integers.
         uint32_t uws_id = 0;
         if (base::StringToUint(uws_string, &uws_id)) {
-          UMA_HISTOGRAM_SPARSE_SLOWLY(kFoundUwsMetricName, uws_id);
+          RecordSparseHistogram(kFoundUwsMetricName, uws_id);
           if (rappor_service) {
             rappor_service->RecordSample(kFoundUwsMetricName,

[csharp, 2016/08/25 19:47:28]

What about using FullName(kFoundUwSMetricName), then we could still use rappor
with the experiments

[Joe Mason, 2016/08/25 22:03:20]

I think I also need to update some rappor config, similar to histograms.xml, and
I haven't had time to look into it. I can enable it in a followup.

                                          rappor::COARSE_RAPPOR_TYPE,
                                          base::UTF16ToUTF8(uws_string));
           }
         } else {
           parse_error = true;
         }
       }
 
       // Clean up the old value.
       reporter_key.DeleteValue(kFoundUwsValueName);
 
-      UMA_HISTOGRAM_BOOLEAN(kFoundUwsReadErrorMetricName, parse_error);
+      RecordBooleanHistogram(kFoundUwsReadErrorMetricName, parse_error);
     }
   }
 
   // Reports to UMA the memory usage of the software reporter tool as reported
   // by the tool itself in the Windows registry.
   void ReportMemoryUsage() const {
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER,
-                                   kSoftwareRemovalToolRegistryKey,
+    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
                                    KEY_QUERY_VALUE | KEY_SET_VALUE);
     DWORD memory_used = 0;
     if (reporter_key.Valid() &&
         reporter_key.ReadValueDW(kMemoryUsedValueName, &memory_used) ==
             ERROR_SUCCESS) {
-      UMA_HISTOGRAM_MEMORY_KB(kMemoryUsedMetricName, memory_used);
+      RecordMemoryKBHistogram(kMemoryUsedMetricName, memory_used);
       reporter_key.DeleteValue(kMemoryUsedValueName);
     }
   }
 
-  // Report the SwReporter run time with UMA both as reported by the tool via
+  // Reports the SwReporter run time with UMA both as reported by the tool via
   // the registry and as measured by |ReporterRunner|.
   void ReportRuntime(const base::TimeDelta& reporter_running_time) const {
-    UMA_HISTOGRAM_LONG_TIMES("SoftwareReporter.RunningTimeAccordingToChrome",
+    RecordLongTimesHistogram("SoftwareReporter.RunningTimeAccordingToChrome",
                              reporter_running_time);
 
-    base::win::RegKey reporter_key(
-        HKEY_CURRENT_USER, kSoftwareRemovalToolRegistryKey, KEY_ALL_ACCESS);
+    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
+                                   KEY_ALL_ACCESS);
     if (!reporter_key.Valid()) {
-      UMA_HISTOGRAM_ENUMERATION(
+      RecordEnumerationHistogram(
           kRunningTimeErrorMetricName,
           REPORTER_RUNNING_TIME_ERROR_REGISTRY_KEY_INVALID,
           REPORTER_RUNNING_TIME_ERROR_MAX);
       return;
     }
 
     bool has_start_time = false;
     int64_t start_time_value = 0;
     if (reporter_key.HasValue(kStartTimeValueName) &&
         reporter_key.ReadInt64(kStartTimeValueName, &start_time_value) ==
             ERROR_SUCCESS) {
       has_start_time = true;
       reporter_key.DeleteValue(kStartTimeValueName);
     }
 
     bool has_end_time = false;
     int64_t end_time_value = 0;
     if (reporter_key.HasValue(kEndTimeValueName) &&
         reporter_key.ReadInt64(kEndTimeValueName, &end_time_value) ==
             ERROR_SUCCESS) {
       has_end_time = true;
       reporter_key.DeleteValue(kEndTimeValueName);
     }
 
     if (has_start_time && has_end_time) {
       base::TimeDelta registry_run_time =
           base::Time::FromInternalValue(end_time_value) -
           base::Time::FromInternalValue(start_time_value);
-      UMA_HISTOGRAM_LONG_TIMES("SoftwareReporter.RunningTime",
+      RecordLongTimesHistogram("SoftwareReporter.RunningTime",
                                registry_run_time);
-      UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
-                                REPORTER_RUNNING_TIME_ERROR_NO_ERROR,
-                                REPORTER_RUNNING_TIME_ERROR_MAX);
+      RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+                                 REPORTER_RUNNING_TIME_ERROR_NO_ERROR,
+                                 REPORTER_RUNNING_TIME_ERROR_MAX);
     } else if (!has_start_time && !has_end_time) {
-      UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
-                                REPORTER_RUNNING_TIME_ERROR_MISSING_BOTH_TIMES,
-                                REPORTER_RUNNING_TIME_ERROR_MAX);
+      RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+                                 REPORTER_RUNNING_TIME_ERROR_MISSING_BOTH_TIMES,
+                                 REPORTER_RUNNING_TIME_ERROR_MAX);
     } else if (!has_start_time) {
-      UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
-                                REPORTER_RUNNING_TIME_ERROR_MISSING_START_TIME,
-                                REPORTER_RUNNING_TIME_ERROR_MAX);
+      RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+                                 REPORTER_RUNNING_TIME_ERROR_MISSING_START_TIME,
+                                 REPORTER_RUNNING_TIME_ERROR_MAX);
     } else {
       DCHECK(!has_end_time);
-      UMA_HISTOGRAM_ENUMERATION(kRunningTimeErrorMetricName,
-                                REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME,
-                                REPORTER_RUNNING_TIME_ERROR_MAX);
+      RecordEnumerationHistogram(kRunningTimeErrorMetricName,
+                                 REPORTER_RUNNING_TIME_ERROR_MISSING_END_TIME,
+                                 REPORTER_RUNNING_TIME_ERROR_MAX);
     }
   }
 
-  // Report the UwS scan times of the software reporter tool via UMA.
+  // Reports the UwS scan times of the software reporter tool via UMA.
   void ReportScanTimes() const {
     base::string16 scan_times_key_path = base::StringPrintf(
-        L"%ls\\%ls", kSoftwareRemovalToolRegistryKey, kScanTimesSubKey);
+        L"%ls\\%ls", registry_key_.c_str(), kScanTimesSubKey);
     base::win::RegKey scan_times_key(
         HKEY_CURRENT_USER, scan_times_key_path.c_str(), KEY_ALL_ACCESS);
     if (!scan_times_key.Valid())
       return;
 
     base::string16 value_name;
     int uws_id = 0;
     int64_t raw_scan_time = 0;
     int num_scan_times = scan_times_key.GetValueCount();
     for (int i = 0; i < num_scan_times; ++i) {
       if (scan_times_key.GetValueNameAt(i, &value_name) == ERROR_SUCCESS &&
           base::StringToInt(value_name, &uws_id) &&
           scan_times_key.ReadInt64(value_name.c_str(), &raw_scan_time) ==
               ERROR_SUCCESS) {
         base::TimeDelta scan_time =
             base::TimeDelta::FromInternalValue(raw_scan_time);
-        base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
-            kScanTimesMetricName,
-            base::HistogramBase::kUmaTargetedHistogramFlag);
-        if (histogram) {
-          // We report the number of seconds plus one because it can take less
-          // than one second to scan some UwS and the count passed to |AddCount|
-          // must be at least one.
-          histogram->AddCount(uws_id, scan_time.InSeconds() + 1);
-        }
+        // We report the number of seconds plus one because it can take less
+        // than one second to scan some UwS and the count passed to |AddCount|
+        // must be at least one.
+        RecordSparseHistogramCount(kScanTimesMetricName, uws_id,
+                                   scan_time.InSeconds() + 1);
       }
     }
     // Clean up by deleting the scan times key, which is a subkey of the main
     // reporter key.
     scan_times_key.Close();
-    base::win::RegKey reporter_key(HKEY_CURRENT_USER,
-                                   kSoftwareRemovalToolRegistryKey,
+    base::win::RegKey reporter_key(HKEY_CURRENT_USER, registry_key_.c_str(),
                                    KEY_ENUMERATE_SUB_KEYS);
     if (reporter_key.Valid())
       reporter_key.DeleteKey(kScanTimesSubKey);
   }
+
+  void RecordReporterStep(SwReporterUmaValue value) {
+    RecordEnumerationHistogram("SoftwareReporter.Step", value, SW_REPORTER_MAX);
+  }
+
+ private:
+  std::string FullName(const std::string& name) const {
+    if (suffix_.empty())
+      return name;
+    return base::StringPrintf("%s_%s", name.c_str(), suffix_.c_str());
+  }
+
+  template <typename HistogramType, typename SampleType>
+  void RecordHistogram(const std::string& name, SampleType sample) const {
+    base::HistogramBase* histogram = HistogramType::FactoryGet(
+        FullName(name), base::HistogramBase::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->Add(sample);
+  }
+
+  template <typename SampleType>
+  void RecordBooleanHistogram(const std::string& name,
+                              SampleType sample) const {
+    RecordHistogram<base::BooleanHistogram, SampleType>(name, sample);
+  }
+
+  template <typename SampleType>
+  void RecordEnumerationHistogram(const std::string& name,
+                                  SampleType sample,
+                                  SampleType boundary) const {
+    // See HISTOGRAM_ENUMERATION_WITH_FLAG for the parameters to |FactoryGet|.
+    base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+        FullName(name), 1, boundary, boundary + 1,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->Add(sample);
+  }
+
+  template <typename SampleType>
+  void RecordLongTimesHistogram(const std::string& name,
+                                SampleType sample) const {
+    // See UMA_HISTOGRAM_LONG_TIMES for the parameters to |FactoryTimeGet|.
+    base::HistogramBase* histogram = base::Histogram::FactoryTimeGet(
+        FullName(name), base::TimeDelta::FromMilliseconds(1),
+        base::TimeDelta::FromHours(1), 100,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->AddTime(sample);
+  }
+
+  template <typename SampleType>
+  void RecordMemoryKBHistogram(const std::string& name,
+                               SampleType sample) const {
+    // See UMA_HISTOGRAM_MEMORY_KB for the parameters to |FactoryGet|.
+    base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
+        FullName(name), 1000, 500000, 50,
+        base::HistogramBase::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->Add(sample);
+  }
+
+  template <typename SampleType>
+  void RecordSparseHistogram(const std::string& name, SampleType sample) const {
+    RecordHistogram<base::SparseHistogram, SampleType>(name, sample);
+  }
+
+  template <typename SampleType>
+  void RecordSparseHistogramCount(const std::string& name,
+                                  SampleType sample,
+                                  int count) const {
+    base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
+        FullName(name), base::HistogramBase::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->AddCount(sample, count);
+  }
+
+  std::string suffix_;

[csharp, 2016/08/25 19:47:28]

What about making these both consts?

[Joe Mason, 2016/08/25 22:03:20]

Done.

+  std::wstring registry_key_;
 };
 
+// Records the reporter step without a suffix. (For steps that are never run by
+// the experimental reporter.)
 void RecordReporterStepHistogram(SwReporterUmaValue value) {
-  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.Step", value, SW_REPORTER_MAX);
+  UMAHistogramReporter uma;
+  uma.RecordReporterStep(value);
 }
 
 void DisplaySRTPrompt(const base::FilePath& download_path) {
   // Find the last active browser, which may be NULL, in which case we won't
   // show the prompt this time and will wait until the next run of the
   // reporter. We can't use other ways of finding a browser because we don't
   // have a profile.
   Browser* browser = chrome::FindLastActive();
   if (!browser)
     return;
@@ skipping 43 matching lines @@
 // interrupted by a shutdown at any time, so it shouldn't depend on anything
 // external that could be shut down beforehand.
 int LaunchAndWaitForExit(const SwReporterInvocation& invocation) {
   if (g_testing_delegate_)
     return g_testing_delegate_->LaunchReporter(invocation);
   base::Process reporter_process =
       base::LaunchProcess(invocation.command_line, base::LaunchOptions());
   // This exit code is used to identify that a reporter run didn't happen, so
   // the result should be ignored and a rerun scheduled for the usual delay.
   int exit_code = kReporterFailureExitCode;
+  UMAHistogramReporter uma(invocation.suffix);
   if (reporter_process.IsValid()) {
-    RecordReporterStepHistogram(SW_REPORTER_START_EXECUTION);
+    uma.RecordReporterStep(SW_REPORTER_START_EXECUTION);
     bool success = reporter_process.WaitForExit(&exit_code);
     DCHECK(success);
   } else {
-    RecordReporterStepHistogram(SW_REPORTER_FAILED_TO_START);
+    uma.RecordReporterStep(SW_REPORTER_FAILED_TO_START);
   }
   return exit_code;
 }
 
 }  // namespace
 
 // Class that will attempt to download the SRT, showing the SRT notification
 // bubble when the download operation is complete. Instances of SRTFetcher own
 // themselves, they will self-delete on completion of the network request when
 // OnURLFetchComplete is called.
@@ skipping 129 matching lines @@
       instance_ = new ReporterRunner;
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     // There's nothing to do if the path and version of the reporter has not
     // changed, we just keep running the tasks that are running now.
     if (instance_->invocation_ == invocation && instance_->version_.IsValid() &&
         instance_->version_ == version)
       return;
 
     instance_->invocation_ = invocation;
     instance_->version_ = version;
-    instance_->main_thread_task_runner_ = main_thread_task_runner;
-    instance_->blocking_task_runner_ = blocking_task_runner;
+    instance_->main_thread_task_runner_ = std::move(main_thread_task_runner);
+    instance_->blocking_task_runner_ = std::move(blocking_task_runner);
 
     if (instance_->first_run_) {
       instance_->first_run_ = false;
       instance_->TryToRun();
     }
   }
 
  private:
   ReporterRunner() {}
   ~ReporterRunner() override {}
 
   // BrowserListObserver.
   void OnBrowserSetLastActive(Browser* browser) override {}
   void OnBrowserRemoved(Browser* browser) override {}
   void OnBrowserAdded(Browser* browser) override {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     DCHECK(browser);
     MaybeFetchSRT(browser, version_);
     BrowserList::RemoveObserver(this);
   }
 
   // This method is called on the UI thread when the reporter run has completed.
   // This is run as a task posted from an interruptible worker thread so should
   // be resilient to unexpected shutdown.
   void ReporterDone(const base::Time& reporter_start_time,
                     const base::Version& version,
+                    const SwReporterInvocation& finished_invocation,
                     int exit_code) {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     if (g_testing_delegate_)
       g_testing_delegate_->NotifyReporterDone();
 
     base::TimeDelta reporter_running_time =
         base::Time::Now() - reporter_start_time;
     // Don't continue when the reporter process failed to launch, but still try
     // again after the regular delay. It's not worth retrying earlier, risking
     // running too often if it always fails, since not many users fail here.
     main_thread_task_runner_->PostDelayedTask(
         FROM_HERE,
         base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
         base::TimeDelta::FromDays(days_between_reporter_runs_));
     if (exit_code == kReporterFailureExitCode)
       return;
 
-    UMAHistogramReporter uma;
+    UMAHistogramReporter uma(finished_invocation.suffix);
     uma.ReportVersion(version);
     uma.ReportExitCode(exit_code);
-    uma.ReportFoundUwS();
+    uma.ReportFoundUwS(!finished_invocation.is_experimental /*use_rappor*/);
+
+    //  Only save results from the canonical version of the software.
     PrefService* local_state = g_browser_process->local_state();
-    if (local_state) {
+    if (local_state && !finished_invocation.is_experimental) {
       local_state->SetInteger(prefs::kSwReporterLastExitCode, exit_code);
       local_state->SetInt64(prefs::kSwReporterLastTimeTriggered,
                             base::Time::Now().ToInternalValue());
     }
     uma.ReportRuntime(reporter_running_time);
     uma.ReportScanTimes();
     uma.ReportMemoryUsage();
 
+    // Only continue to launch the prompt for the canonical version.
+    if (finished_invocation.is_experimental)
+      return;
+
     if (!IsInSRTPromptFieldTrialGroups()) {
       // Knowing about disabled field trial is more important than reporter not
       // finding anything to remove, so check this case first.
       RecordReporterStepHistogram(SW_REPORTER_NO_PROMPT_FIELD_TRIAL);
       return;
     }
 
     if (exit_code != kSwReporterPostRebootCleanupNeeded &&
         exit_code != kSwReporterCleanupNeeded) {
       RecordReporterStepHistogram(SW_REPORTER_NO_PROMPT_NEEDED);
@@ skipping 46 matching lines @@
         g_testing_delegate_->NotifyLaunchReady();
 
       // 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(&ReporterRunner::ReporterDone, base::Unretained(this),
-                     base::Time::Now(), version_));
+                     base::Time::Now(), version_, invocation_));
     } else {
       main_thread_task_runner_->PostDelayedTask(
           FROM_HERE,
           base::Bind(&ReporterRunner::TryToRun, base::Unretained(this)),
           next_trigger_delay);
     }
   }
 
   bool first_run_ = true;
   SwReporterInvocation invocation_;
@@ skipping 28 matching lines @@
 }
 
 SwReporterInvocation SwReporterInvocation::FromCommandLine(
     const base::CommandLine& command_line) {
   SwReporterInvocation invocation;
   invocation.command_line = command_line;
   return invocation;
 }
 
 bool SwReporterInvocation::operator==(const SwReporterInvocation& other) const {
-  return command_line.argv() == other.command_line.argv();
+  return command_line.argv() == other.command_line.argv() &&
+         suffix == other.suffix && is_experimental == other.is_experimental;
 }
 
 void RunSwReporter(const SwReporterInvocation& invocation,
                    const base::Version& version,
                    scoped_refptr<base::TaskRunner> main_thread_task_runner,
                    scoped_refptr<base::TaskRunner> blocking_task_runner) {
-  ReporterRunner::Run(invocation, version, main_thread_task_runner,
-                      blocking_task_runner);
+  ReporterRunner::Run(invocation, version, std::move(main_thread_task_runner),
+                      std::move(blocking_task_runner));
 }
 
 bool ReporterFoundUws() {
   PrefService* local_state = g_browser_process->local_state();
   if (!local_state)
     return false;
   int exit_code = local_state->GetInteger(prefs::kSwReporterLastExitCode);
   return exit_code == kSwReporterCleanupNeeded;
 }
 
 bool UserHasRunCleaner() {
   base::string16 cleaner_key_path(kSoftwareRemovalToolRegistryKey);
   cleaner_key_path.append(L"\\").append(kCleanerSubKey);
 
   base::win::RegKey srt_cleaner_key(HKEY_CURRENT_USER, cleaner_key_path.c_str(),
                                     KEY_QUERY_VALUE);
 
   return srt_cleaner_key.Valid() && srt_cleaner_key.GetValueCount() > 0;
 }
 
 void SetSwReporterTestingDelegate(SwReporterTestingDelegate* delegate) {
   g_testing_delegate_ = delegate;
 }
 
 }  // namespace safe_browsing