Add support for the ExperimentalSwReporterEngine field trial.

chrome/browser/component_updater/sw_reporter_installer_win.cc

Index: chrome/browser/component_updater/sw_reporter_installer_win.cc
diff --git a/chrome/browser/component_updater/sw_reporter_installer_win.cc b/chrome/browser/component_updater/sw_reporter_installer_win.cc
index cad962c7d29a8f83f4a2a8fb84770271fee87a19..eb16773aca18439d340cc526c78735e9fdcf4b89 100644
--- a/chrome/browser/component_updater/sw_reporter_installer_win.cc
+++ b/chrome/browser/component_updater/sw_reporter_installer_win.cc
@@ -14,6 +14,8 @@
 
 #include "base/base_paths.h"
 #include "base/bind.h"
+#include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
@@ -21,6 +23,7 @@
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_tokenizer.h"
+#include "base/sys_info.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
@@ -35,14 +38,16 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "components/update_client/update_client.h"
 #include "components/update_client/utils.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
+#include "third_party/re2/src/re2/re2.h"

[Sorin Jianu, 2016/08/25 03:04:29]

I suggest checking with brettw@ or some other super Chromista if taking a
dependency on RE2 is something desirable. I've seen examples where people have
pushed back. Code search indicates few usages in the code base. At one point I
recall RE could not even be included due to DEPS. I see that there is a mention
of using RE instead of C++11 <regex> but I suggest asking anyway.

[Joe Mason, 2016/08/25 13:43:28]

Since the use of RE's here is so simple, I'll just replace it.

[Joe Mason, 2016/08/25 15:50:05]

Done.

 
 namespace component_updater {
 
 namespace {
 
-// These two sets of values are used to send UMA information and are replicated
-// in the histograms.xml file, so the order MUST NOT CHANGE.
+// These values are used to send UMA information and are replicated in the
+// histograms.xml file, so the order MUST NOT CHANGE.
 enum SRTCompleted {
   SRT_COMPLETED_NOT_YET = 0,
   SRT_COMPLETED_YES = 1,
@@ -117,11 +122,114 @@ void RunSwReporterAfterStartup(
                  base::WorkerPool::GetTaskRunner(true)));
 }
 
+constexpr base::Feature kExperimentalEngineFeature{

[grt (UTC plus 2), 2016/08/25 15:42:13]

i think it's good practice to put things like enums and constants up near the
top of the namespace to kinda-sorta follow the guidance for the order of
declarations in a class.

+    "ExperimentalSwReporterEngine", base::FEATURE_DISABLED_BY_DEFAULT};
+
+// These MUST match the values for SwReporterExperimentError in histograms.xml.
+enum ExperimentError {
+  EXPERIMENT_ERROR_BAD_TAG = 0,
+  EXPERIMENT_ERROR_BAD_PARAMS = 1,
+  EXPERIMENT_ERROR_MAX,
+};
+
+void ReportExperimentError(ExperimentError error) {

[grt (UTC plus 2), 2016/08/25 15:42:12]

you could move this up closer to the top to keep it closer to the enum, too

+  UMA_HISTOGRAM_ENUMERATION("SoftwareReporter.ExperimentErrors", error,
+                            EXPERIMENT_ERROR_MAX);
+}
+
+// Read the command-line params and an UMA histogram suffix from the manifest,

[grt (UTC plus 2), 2016/08/25 15:42:12]

// Reads the... and launches the...
as per https://google.github.io/styleguide/cppguide.html#Function_Comments

[Joe Mason, 2016/08/25 17:52:58]

Done.

+// and launch the SwReporter with those parameters.  If anything goes wrong the

[Sorin Jianu, 2016/08/25 03:04:29]

Why two spaces after period?

[Joe Mason, 2016/08/25 15:50:05]

Done.

+// SwReporter should not be run at all - do not fall back to the default.
+void RunExperimentalSwReporter(const base::FilePath& exe_path,
+                               const base::Version& version,
+                               std::unique_ptr<base::DictionaryValue> manifest,
+                               const SwReporterRunner& reporter_runner) {
+  // Don't log an error if the launch_params section is entirely missing.  This
+  // can happen when the user already has an older version of the software
+  // reporter component, so |ComponentReady| is called on startup before the
+  // experimental version is downloaded.
+  //
+  // Also don't run the old version, though. We want this user to run the
+  // experimental version once it's been downloaded.
+  if (!manifest->HasKey("launch_params"))
+    return;
+
+  const base::ListValue* parameter_list = nullptr;
+  if (!manifest->GetList("launch_params", &parameter_list)) {
+    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);

[grt (UTC plus 2), 2016/08/25 15:42:12]

rather than doing two lookups in the dict, how about:

  base::Value* launch_params = nullptr;
  if (!manifest->Get("launch_params", &launch_params))
    return;

  const base::ListValue* parameter_list = nullptr;
  if (!launch_param->GetAsList(&parameter_list)) {
    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
    return;
  }

[Joe Mason, 2016/08/25 17:52:58]

Done.

+    return;
+  }
+
+  // If launch_params exists but is empty, run the experimental reporter once
+  // with no suffix or command-line parameters, to mimic the regular version.
+  // (The control group would be set up this way.)
+  DCHECK(parameter_list);

[grt (UTC plus 2), 2016/08/25 15:42:13]

remove this. it's impossible thanks to the two early returns above

[Joe Mason, 2016/08/25 17:52:58]

Done.

+  if (parameter_list->empty()) {

[Joe Mason, 2016/08/25 13:43:28]

With an evening's distance from the code, I can see this is too complicated. I'm
just going to make an empty launch_params an error, an allow an empty suffix.
That way for the "control group" that uses no suffix and no arguments, we
explictly use empty strings for "suffix" and "arguments" in the manifest.
Working on that patch now; hold tight.

[Joe Mason, 2016/08/25 15:50:05]

Done.

+    auto invocation =
+        safe_browsing::SwReporterInvocation::FromFilePath(exe_path);
+    invocation.is_experimental = true;
+    reporter_runner.Run(invocation, version);
+    return;
+  }
+
+  // For now we only support running the reporter once, so only look at the
+  // first set of arguments in the list.
+  const base::DictionaryValue* invocation_params = nullptr;
+  if (!parameter_list->GetDictionary(0, &invocation_params)) {
+    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // The suffix is required, and must be a short alphanumeric string.
+  static const re2::RE2 kValidSuffixPattern("[a-zA-Z0-9]{1,256}");
+  std::string suffix;
+  DCHECK(invocation_params);
+  invocation_params->GetString("suffix", &suffix);
+  if (suffix.empty() || !re2::RE2::FullMatch(suffix, kValidSuffixPattern)) {
+    ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+    return;
+  }
+
+  // Build a command line for the reporter out of the executable path and the
+  // (optional) arguments from the manifest.
+  std::vector<base::string16> argv{exe_path.value()};

[grt (UTC plus 2), 2016/08/25 15:42:12]

...argv = {exe_path.value()};
as per
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

[Joe Mason, 2016/08/25 17:52:58]

Done.

+  if (invocation_params->HasKey("arguments")) {
+    const base::ListValue* arguments = nullptr;
+    if (!invocation_params->GetList("arguments", &arguments)) {
+      ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+      return;
+    }
+    for (const std::unique_ptr<base::Value>& value : *arguments) {

[Sorin Jianu, 2016/08/25 03:04:29]

using const auto& could be reasonable imho, assuming the code compiles.

[Joe Mason, 2016/08/25 13:43:28]

I think it's better to make the unique_ptr here explicit, since otherwise
there's no way to know the exact type that arguments->begin / end returns
without looking at the Value docs. It's easy to get cryptic compile errors when
changing this line because the unique_ptr interacts with iterators in unexpected
ways.

[grt (UTC plus 2), 2016/08/25 15:42:12]

I agree. If not, base::ListValue::iterator is more appropriate.

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

After working with this some more I've come to agree with you. Done.

+      base::string16 argument;
+      if (!value->GetAsString(&argument)) {
+        ReportExperimentError(EXPERIMENT_ERROR_BAD_PARAMS);
+        return;
+      }
+      argv.push_back(argument);
+    }
+  }
+  base::CommandLine command_line(argv);
+
+  // Add the histogram suffix to the command-line as well, so that the
+  // reporter will add the same suffix to registry keys where it writes
+  // metrics.
+  command_line.AppendSwitchASCII("registry-suffix", suffix);
+
+  auto invocation =
+      safe_browsing::SwReporterInvocation::FromCommandLine(command_line);
+  invocation.suffix = suffix;
+  invocation.is_experimental = true;
+
+  reporter_runner.Run(invocation, version);
+}
+
 }  // namespace
 
 SwReporterInstallerTraits::SwReporterInstallerTraits(
-    const SwReporterRunner& reporter_runner)
-    : reporter_runner_(reporter_runner) {}
+    const SwReporterRunner& reporter_runner,
+    bool is_experimental_engine_supported)
+    : reporter_runner_(reporter_runner),
+      is_experimental_engine_supported_(is_experimental_engine_supported) {}
 
 SwReporterInstallerTraits::~SwReporterInstallerTraits() {}
 
@@ -152,6 +260,11 @@ void SwReporterInstallerTraits::ComponentReady(
     std::unique_ptr<base::DictionaryValue> manifest) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   const base::FilePath exe_path(install_dir.Append(kSwReporterExeName));
+  if (IsExperimentalEngineEnabled()) {
+    RunExperimentalSwReporter(exe_path, version, std::move(manifest),
+                              reporter_runner_);
+    return;
+  }
   reporter_runner_.Run(
       safe_browsing::SwReporterInvocation::FromFilePath(exe_path), version);
 }
@@ -171,13 +284,38 @@ std::string SwReporterInstallerTraits::GetName() const {
 
 update_client::InstallerAttributes
 SwReporterInstallerTraits::GetInstallerAttributes() const {
-  return update_client::InstallerAttributes();
+  update_client::InstallerAttributes attributes;
+  if (IsExperimentalEngineEnabled()) {
+    // Pass the "tag" parameter to the installer; it will be used to choose
+    // which binary is downloaded.
+    constexpr char kTagParam[] = "tag";
+    std::string tag = variations::GetVariationParamValueByFeature(

[Sorin Jianu, 2016/08/25 03:04:29]

const?

[Joe Mason, 2016/08/25 15:50:05]

Done.

+        kExperimentalEngineFeature, kTagParam);
+
+    // Validate that the tag is valid (regexp taken from
+    // ComponentInstallerTraits::InstallerAttributes). If not, set it to a
+    // valid but unrecognized value so that nothing will be downloaded.
+    if (tag.empty() ||
+        !re2::RE2::FullMatch(tag, "^[-.,;+_=a-zA-Z0-9]{0,256}$")) {
+      ReportExperimentError(EXPERIMENT_ERROR_BAD_TAG);
+      attributes[kTagParam] = "missing_tag";
+    } else {
+      attributes[kTagParam] = tag;
+    }
+  }
+  return attributes;
 }
 
 std::vector<std::string> SwReporterInstallerTraits::GetMimeTypes() const {
   return std::vector<std::string>();
 }
 
+bool SwReporterInstallerTraits::IsExperimentalEngineEnabled() const {
+  if (!is_experimental_engine_supported_)

[Sorin Jianu, 2016/08/25 03:04:29]

could use ?:

[Joe Mason, 2016/08/25 15:50:05]

Or just &&. Done.

+    return false;
+  return base::FeatureList::IsEnabled(kExperimentalEngineFeature);
+}
+
 void RegisterSwReporterComponent(ComponentUpdateService* cus) {
   if (!safe_browsing::IsSwReporterEnabled())
     return;
@@ -254,9 +392,15 @@ void RegisterSwReporterComponent(ComponentUpdateService* cus) {
     }
   }
 
+  // The experiment is only enabled on x86. There's no way to check this in the
+  // variations config so we'll hard-code it.
+  bool is_experimental_engine_enabled =

[Sorin Jianu, 2016/08/25 03:04:29]

const?

[Joe Mason, 2016/08/25 15:50:05]

Done.

+      base::SysInfo::OperatingSystemArchitecture() == "x86";

[grt (UTC plus 2), 2016/08/25 15:42:13]

use this:
base::win::OSInfo::GetInstance()->architecture() ==
base::win::OSInfo::X86_ARCHITECTURE;

[Joe Mason, 2016/08/25 17:52:58]

Done.

+
   // Install the component.
   std::unique_ptr<ComponentInstallerTraits> traits(
-      new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup)));
+      new SwReporterInstallerTraits(base::Bind(&RunSwReporterAfterStartup),
+                                    is_experimental_engine_enabled));
   // |cus| will take ownership of |installer| during installer->Register(cus).
   DefaultComponentInstaller* installer =
       new DefaultComponentInstaller(std::move(traits));