Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/safe_browsing/srt_fetcher_win.h

Index: chrome/browser/safe_browsing/srt_fetcher_win.h
diff --git a/chrome/browser/safe_browsing/srt_fetcher_win.h b/chrome/browser/safe_browsing/srt_fetcher_win.h
index 7d310194d93001db65c2c1556608c6649b85319a..66ba4cb89eb063ba1586916250ed1782bd6c4478 100644
--- a/chrome/browser/safe_browsing/srt_fetcher_win.h
+++ b/chrome/browser/safe_browsing/srt_fetcher_win.h
@@ -8,13 +8,14 @@
 #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;
 }
 
@@ -43,19 +44,58 @@ 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.