Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_win.h

 // 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.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SRT_FETCHER_WIN_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SRT_FETCHER_WIN_H_
 
 #include <limits.h>
 
 #include <string>
+#include <vector>
 
 #include "base/callback_forward.h"
+#include "base/command_line.h"
 #include "base/memory/ref_counted.h"
 #include "base/version.h"
 
 namespace base {
-class FilePath;
 class TaskRunner;
 }
 
 class Browser;
 
 namespace safe_browsing {
 
 // SRT registry keys and value names.
 extern const wchar_t kSoftwareRemovalToolRegistryKey[];
 extern const wchar_t kEndTimeValueName[];
 extern const wchar_t kStartTimeValueName[];
 
 // Reporter exit codes.
 const int kSwReporterCleanupNeeded = 0;
 const int kSwReporterNothingFound = 2;
 const int kSwReporterPostRebootCleanupNeeded = 4;
 const int kSwReporterDelayedPostRebootCleanupNeeded = 15;
 
 // A special exit code identifying a failure to run the reporter.
 const int kReporterFailureExitCode = INT_MAX;
 
 // The number of days to wait before triggering another reporter run.
 const int kDaysBetweenSuccessfulSwReporterRuns = 7;
 const int kDaysBetweenSwReporterRunsForPendingPrompt = 1;
 
 // To test SRT Fetches.
 const int kSRTFetcherID = 47;
 
+// Parameters used to invoke the sw_reporter component.
+struct SwReporterInvocation {
+  base::CommandLine command_line;
+
+  // Experimental versions of the reporter will write metrics to registry keys
+  // ending in |suffix|. Those metrics should be copid to UMA histograms also

[csharp, 2016/08/11 18:58:02]

nit: copid -> copied

[Joe Mason, 2016/08/15 17:24:40]

Done.

+  // ending in |suffix|. For the canonical version, |suffix| will be empty.
+  std::string suffix;
+

[csharp, 2016/08/11 18:58:02]

I think this should always contain a bool to determine if we want to prompt a
user or not, instead of relying on suffix being empty

[Joe Mason, 2016/08/15 17:24:40]

Done.

+  SwReporterInvocation();
+  explicit SwReporterInvocation(const base::FilePath& exe_path);
+  SwReporterInvocation(const base::CommandLine& command_line,
+                       const std::string& suffix);
+
+  bool operator==(const SwReporterInvocation& other) const;

[csharp, 2016/08/11 18:58:02]

Why do you need to compare SwReporterInvocations?

[Joe Mason, 2016/08/15 17:24:40]

ReporterRunner::Run starts with a check to see if the way the reporter was
invoked has changed. Before this patch it's just comparing the exe path, but I
think with this patch it makes sense to check if any of the exe path, command
line args, or place the metrics should be recorded has changed. (Or number of
invocations! It actually now compares two queues of SwReporterInvocations.)

At a higher level, what we'll have is that if ComponentReady is called again and
the experiment params haven't changed, it'll construct exactly the same chain of
invocations out of those params and then pass them to ReporterRunner::Run. Since
nothing has changed and that chain of invocations is already scheduled to run,
ReporterRunner should just skip them. It's the same logic that was used when the
only "param" that could change was the path to the reporter exe and version, so
that was all that ReporterRunner checked.

+};
+
 // Tries to run the sw_reporter component, and then schedule the next try. If
 // called multiple times, then multiple sequences of trying to run will happen,
 // yet only one reporter will run per specified period (either
 // |kDaysBetweenSuccessfulSwReporterRuns| or
 // |kDaysBetweenSwReporterRunsForPendingPrompt|) will actually happen.
-// |exe_path| is the full path to the SwReporter to execute and |version| is its
-// version. The task runners are provided to allow tests to provide their own.
+// |command_line| is the command line to execute, including the full path to
+// the SwReporter to execute, and |version| is its version. The task runners are
+// provided to allow tests to provide their own.
 void RunSwReporter(

[csharp, 2016/08/11 18:58:02]

Why not merge this and the next function and make callers responsible for
determining how many reporters to run (then this code doesn't have to worry
about experiments)

[Joe Mason, 2016/08/15 17:24:40]

I removed Experimental from the function name but I kept it as two functions,
since it seems useful to be able to pass a single invocation without the caller
having to construct a queue. (After all, that's the more common case - the
entire logic of having a chain of invocations is specific to this experiment.)

After the experiment is over, I would replace the queue<SwReporterInvocation> in
ReporterRunner with just a single SwReporterInvocation, since that seems like a
useful abstraction if we ever want to make more changes, but I wouldn't want to
keep the rest of this complexity around, so I would remove the second function.

To back up my point: can you imagine trying to merge the comments above these
two functions to explain the difference between "Tries to run the sw_reporter
component, and then schedule the next try," and, "Here'e a queue of
invocations," in one step? I find it much easier to follow with the simpler
function, and then the more complicated one expanding on it.

     const base::FilePath& exe_path,
     const base::Version& version,
     const scoped_refptr<base::TaskRunner>& main_thread_task_runner,
     const scoped_refptr<base::TaskRunner>& blocking_task_runner);
 
+// Try to run multiple invocations of the experimental sw_reporter component,
+// with different combinations of command line args. Each set of invocations is
+// treated as a normal scheduled launch of the sw_reporter for the purposes of
+// retries, as described in |RunSwReporter|.
+//
+// |invocation_queue| is a list of sw_reporter configurations which will be
+// launched in order. Once one has been executed, the reporter is considered to
+// have "run" - the remainder will also be executed as part of this run, but
+// then the reporter will not be run again for the specified period. If none of
+// the invocations in the queue execute, another queue can be tried by calling
+// this function again.
+//
+// See |RunSwReporter| for the meaning of the remaining parameters.
+//
+// The experimental sw_reporter never triggers the prompt, just reports results.
+void RunExperimentalSwReporters(
+    const std::vector<SwReporterInvocation>& invocation_queue,
+    const base::Version& version,
+    const scoped_refptr<base::TaskRunner>& main_thread_task_runner,

[grt (UTC plus 2), 2016/08/10 06:47:02]

since this function holds a reference to these two objects beyond the lifetime
of its call, it should take the refptrs by value rather than by constref; see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md....

[Joe Mason, 2016/08/15 17:24:40]

Done.

+    const scoped_refptr<base::TaskRunner>& blocking_task_runner);
+
 // Returns true iff Local State is successfully accessed and indicates the most
 // recent Reporter run terminated with an exit code indicating the presence of
 // UwS.
 bool ReporterFoundUws();
 
 // Returns true iff a valid registry key for the SRT Cleaner exists, and that
 // key is nonempty.
 // TODO(tmartino): Consider changing to check whether the user has recently
 // run the cleaner, rather than checking if they've run it at all.
 bool UserHasRunCleaner();
 
 // Test mocks for launching the reporter and showing the prompt
 typedef base::Callback<int(const base::FilePath& exe_path)> ReporterLauncher;
 typedef base::Callback<void(Browser*, const std::string&)> PromptTrigger;
 void SetReporterLauncherForTesting(const ReporterLauncher& reporter_launcher);
 void SetPromptTriggerForTesting(const PromptTrigger& prompt_trigger);
 
 }  // namespace safe_browsing
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SRT_FETCHER_WIN_H_