Add support for multiple simultaneous subresource_filter::Configurations.

components/subresource_filter/core/browser/subresource_filter_features.cc

Index: components/subresource_filter/core/browser/subresource_filter_features.cc
diff --git a/components/subresource_filter/core/browser/subresource_filter_features.cc b/components/subresource_filter/core/browser/subresource_filter_features.cc
index 06b06f1b3a6f2be15c0e3ddf5aa1321fe1380e7e..140256adf39f0fbd85887381bf1cb5f6245e23bd 100644
--- a/components/subresource_filter/core/browser/subresource_filter_features.cc
+++ b/components/subresource_filter/core/browser/subresource_filter_features.cc
@@ -4,9 +4,14 @@
 
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
 
+#include <map>
+#include <ostream>
+#include <sstream>
 #include <string>
+#include <tuple>
 #include <utility>
 
+#include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/strings/string_number_conversions.h"
@@ -14,12 +19,15 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
+#include "base/values.h"
 #include "components/variations/variations_associated_data.h"
 
 namespace subresource_filter {
 
 namespace {
 
+// Helpers for parsing the experimental configuration -------------------------
+
 std::string TakeVariationParamOrReturnEmpty(
     std::map<std::string, std::string>* params,
     const std::string& key) {
@@ -69,9 +77,9 @@ ActivationList ParseActivationList(const base::StringPiece activation_lists) {
 }
 
 double ParsePerformanceMeasurementRate(const std::string& rate) {
-  double value = 0;
+  double value = 0.0;
   if (!base::StringToDouble(rate, &value) || value < 0)
-    return 0;
+    return 0.0;
   return value < 1 ? value : 1;
 }
 
@@ -79,39 +87,145 @@ bool ParseBool(const base::StringPiece value) {
   return base::LowerCaseEqualsASCII(value, "true");
 }
 
-Configuration ParseFieldTrialConfiguration() {
-  Configuration configuration;
+std::vector<Configuration> FillEnabledPresetConfigurations(
+    std::map<std::string, std::string>* params) {
+  const struct {
+    const char* name;
+    Configuration (*factory_method)();
+  } kAvailablePresetConfigurations[] = {
+      {kPresetLiveRunOnPhishingSites,
+       &Configuration::MakePresetForLiveRunOnPhishingSites},
+      {kPresetPerformanceTestingDryRunOnAllSites,
+       &Configuration::MakePresetForPerformanceTestingDryRunOnAllSites}};
+
+  std::vector<Configuration> enabled_configurations;
+  const std::string enabled_presets_list =
+      TakeVariationParamOrReturnEmpty(params, kEnablePresetsParameterName);
+  for (const base::StringPiece enabled_preset :
+       base::SplitStringPiece(enabled_presets_list, ",", base::TRIM_WHITESPACE,
+                              base::SPLIT_WANT_NONEMPTY)) {
+    for (const auto& available_preset : kAvailablePresetConfigurations) {
+      if (base::LowerCaseEqualsASCII(enabled_preset, available_preset.name)) {
+        enabled_configurations.push_back(available_preset.factory_method());
+        break;
+      }
+    }
+  }
+  return enabled_configurations;
+}
 
-  std::map<std::string, std::string> params;
-  base::GetFieldTrialParamsByFeature(kSafeBrowsingSubresourceFilter, &params);
+Configuration ParseExperimentalConfiguration(
+    std::map<std::string, std::string>* params) {
+  Configuration configuration;
 
-  configuration.activation_level = ParseActivationLevel(
-      TakeVariationParamOrReturnEmpty(&params, kActivationLevelParameterName));
+  // ActivationConditions:
+  configuration.activation_conditions.activation_scope = ParseActivationScope(
+      TakeVariationParamOrReturnEmpty(params, kActivationScopeParameterName));
 
-  configuration.activation_scope = ParseActivationScope(
-      TakeVariationParamOrReturnEmpty(&params, kActivationScopeParameterName));
+  configuration.activation_conditions.activation_list = ParseActivationList(
+      TakeVariationParamOrReturnEmpty(params, kActivationListsParameterName));
 
-  configuration.activation_list = ParseActivationList(
-      TakeVariationParamOrReturnEmpty(&params, kActivationListsParameterName));
+  // ActivationOptions:
+  configuration.activation_options.activation_level = ParseActivationLevel(
+      TakeVariationParamOrReturnEmpty(params, kActivationLevelParameterName));
 
-  configuration.performance_measurement_rate =
+  configuration.activation_options.performance_measurement_rate =
       ParsePerformanceMeasurementRate(TakeVariationParamOrReturnEmpty(
-          &params, kPerformanceMeasurementRateParameterName));
+          params, kPerformanceMeasurementRateParameterName));
 
-  configuration.should_suppress_notifications =
+  configuration.activation_options.should_suppress_notifications =
       ParseBool(TakeVariationParamOrReturnEmpty(
-          &params, kSuppressNotificationsParameterName));
-
-  configuration.ruleset_flavor =
-      TakeVariationParamOrReturnEmpty(&params, kRulesetFlavorParameterName);
+          params, kSuppressNotificationsParameterName));
 
-  configuration.should_whitelist_site_on_reload =
+  configuration.activation_options.should_whitelist_site_on_reload =
       ParseBool(TakeVariationParamOrReturnEmpty(
-          &params, kWhitelistSiteOnReloadParameterName));
+          params, kWhitelistSiteOnReloadParameterName));
+
+  // GeneralSettings:
+  configuration.general_settings.ruleset_flavor =
+      TakeVariationParamOrReturnEmpty(params, kRulesetFlavorParameterName);
 
   return configuration;
 }
 
+std::vector<Configuration> ParseEnabledConfigurations() {
+  std::map<std::string, std::string> params;
+  base::GetFieldTrialParamsByFeature(kSafeBrowsingSubresourceFilter, &params);
+
+  std::vector<Configuration> configs = FillEnabledPresetConfigurations(&params);
+
+  Configuration experimental_config = ParseExperimentalConfiguration(&params);
+  configs.push_back(std::move(experimental_config));

[pkalinnikov, 2017/05/04 12:04:22]

So, we add an experimental config regardless of whether the flags have actually
been used. Do I understand correctly, that the default config (in case no flags
are specified) has the lowest possible priority, and won't override any of the
above?

Is it also a safe-guard, so that we have at least 1 config in case there are no
enabled presets?

[engedy, 2017/05/05 12:25:42]

Yes, but it is mostly only needed because otherwise the tests would be really
horrible. Let me know if you feel strongly about this :)

+
+  return configs;
+}
+
+std::vector<Configuration> MakeConfigVector(Configuration config) {
+  std::vector<Configuration> configs;
+  configs.push_back(std::move(config));
+  return configs;
+}
+
+template <class T>
+std::string StreamToString(const T& value) {
+  std::ostringstream oss;
+  oss << value;
+  return oss.str();
+}
+
+// Helpers for calculating configuration priorities ---------------------------
+

[Charlie Harrison, 2017/05/03 14:52:17]

A comment here about what the priority levels mean (is 3 higher priority than
1?) would be helpful.

[engedy, 2017/05/05 12:25:42]

Done.

+size_t GetActivationLevelPriority(ActivationLevel activation_level) {
+  switch (activation_level) {
+    case ActivationLevel::DISABLED:
+      return 1;
+    case ActivationLevel::DRYRUN:
+      return 2;
+    case ActivationLevel::ENABLED:
+      return 3;
+  }
+  NOTREACHED();
+  return 0;
+}
+
+size_t GetActivationScopePriority(ActivationScope activation_scope) {
+  // The more specific ACTIVATION_LIST trumps the blanket scopes.
+  switch (activation_scope) {
+    case ActivationScope::NO_SITES:

[pkalinnikov, 2017/05/04 12:04:22]

Hm, I am confused by the term "more specific". My initial guess was that if A is
a subset of B, then the A set is more specific. According to this, NO_SITES
would be the most specific and come after ACTIVATION_LIST.

I think it worth explicitly mentioning the priorities somewhere in the header
file.

[engedy, 2017/05/05 12:25:42]

Removed this, N/A anymore.

+      return 1;
+    case ActivationScope::ALL_SITES:
+      return 2;
+    case ActivationScope::ACTIVATION_LIST:
+      return 3;
+  }
+  NOTREACHED();
+  return 0;
+}
+
+// Returns an opaque value that represents the priority of the given |config|,
+// where a greater value indicates higher priority. If it falls onto the
+// |activation_list| to break a tie, the more recently implemented list has a
+// higher priority.
+std::tuple<size_t, size_t, size_t> GetConfigurationPriority(
+    const Configuration& config) {
+  // std::tuple implements lexicographical compare.
+  return std::make_tuple(
+      GetActivationLevelPriority(config.activation_options.activation_level),
+      GetActivationScopePriority(config.activation_conditions.activation_scope),
+      static_cast<size_t>(config.activation_conditions.activation_list));
+}
+
+std::vector<Configuration> SortConfigsByDecreasingPriority(
+    std::vector<Configuration> configs) {
+  auto comp = [](const Configuration& a, const Configuration& b) {
+    return GetConfigurationPriority(a) > GetConfigurationPriority(b);
+  };
+  std::sort(configs.begin(), configs.end(), comp);
+  return configs;
+}
+
+// Globals --------------------------------------------------------------------
+
 base::LazyInstance<base::Lock>::Leaky g_active_configurations_lock =
     LAZY_INSTANCE_INITIALIZER;
 
@@ -120,6 +234,8 @@ base::LazyInstance<scoped_refptr<ConfigurationList>>::Leaky
 
 }  // namespace
 
+// Constant definitions -------------------------------------------------------
+
 const base::Feature kSafeBrowsingSubresourceFilter{
     "SubresourceFilter", base::FEATURE_DISABLED_BY_DEFAULT};
 
@@ -143,35 +259,114 @@ const char kActivationListSocialEngineeringAdsInterstitial[] =
 const char kActivationListPhishingInterstitial[] = "phishing_interstitial";
 const char kActivationListSubresourceFilter[] = "subresource_filter";
 
-const char kRulesetFlavorParameterName[] = "ruleset_flavor";
-
 const char kPerformanceMeasurementRateParameterName[] =
     "performance_measurement_rate";
-
 const char kSuppressNotificationsParameterName[] = "suppress_notifications";
-
 const char kWhitelistSiteOnReloadParameterName[] = "whitelist_site_on_reload";
 
+const char kRulesetFlavorParameterName[] = "ruleset_flavor";
+
+const char kEnablePresetsParameterName[] = "enable_presets";
+const char kPresetLiveRunOnPhishingSites[] = "liverun_on_phishing_sites";
+const char kPresetPerformanceTestingDryRunOnAllSites[] =
+    "performance_testing_dryrun_on_all_sites";
+
+// Configuration --------------------------------------------------------------
+
+// static
+Configuration Configuration::MakePresetForLiveRunOnPhishingSites() {
+  return Configuration(ActivationLevel::ENABLED,
+                       ActivationScope::ACTIVATION_LIST,
+                       ActivationList::PHISHING_INTERSTITIAL);
+}
+
+// static
+Configuration Configuration::MakePresetForPerformanceTestingDryRunOnAllSites() {
+  Configuration config(ActivationLevel::DRYRUN, ActivationScope::ALL_SITES);
+  config.activation_options.performance_measurement_rate = 1.0;
+  return config;
+}
+
 Configuration::Configuration() = default;
 Configuration::Configuration(ActivationLevel activation_level,
                              ActivationScope activation_scope,
-                             ActivationList activation_list)
-    : activation_level(activation_level),
-      activation_scope(activation_scope),
-      activation_list(activation_list) {}
+                             ActivationList activation_list) {
+  activation_options.activation_level = activation_level;
+  activation_conditions.activation_scope = activation_scope;
+  activation_conditions.activation_list = activation_list;
+}
+Configuration::Configuration(const Configuration&) = default;
 Configuration::Configuration(Configuration&&) = default;
 Configuration::~Configuration() = default;
+Configuration& Configuration::operator=(const Configuration&) = default;
 Configuration& Configuration::operator=(Configuration&&) = default;
 
+bool Configuration::operator==(const Configuration& rhs) const {
+  const auto tie = [](const Configuration& config) {
+    return std::tie(config.activation_conditions.activation_scope,
+                    config.activation_conditions.activation_list,
+                    config.activation_options.activation_level,
+                    config.activation_options.performance_measurement_rate,
+                    config.activation_options.should_whitelist_site_on_reload,
+                    config.activation_options.should_suppress_notifications,
+                    config.general_settings.ruleset_flavor);
+  };
+  return tie(*this) == tie(rhs);
+}
+
+bool Configuration::operator!=(const Configuration& rhs) const {
+  return !(*this == rhs);
+}
+
+std::ostream& operator<<(std::ostream& os, const Configuration& config) {

[Charlie Harrison, 2017/05/03 14:52:18]

This reminds me, with all these configurations I think it would be extremely
useful to include a TRACE macro at the start of navigation / navigation commit
with what the active configuration is.

We should probably add TRACE macros around a bunch of our code but this one
seems especially important to me as a way to see the active config for that
navigation.

Can do it in a follow up though.

[pkalinnikov, 2017/05/04 12:04:23]

+1

Does trace work well with multi-line strings?

[engedy, 2017/05/05 12:25:42]

As clarified in our offline discussion, we are talking about TRACE_EVENTs here.

[engedy, 2017/05/05 12:25:42]

To clarify, are we talking about SCOPED_TRACE's here?

+  base::DictionaryValue dict;
+  dict.SetString("activation_scope",
+                 StreamToString(config.activation_conditions.activation_scope));
+  dict.SetString("activation_list",
+                 StreamToString(config.activation_conditions.activation_list));
+  dict.SetString("activation_level",
+                 StreamToString(config.activation_options.activation_level));
+  dict.SetDouble("performance_measurement_rate",
+                 config.activation_options.performance_measurement_rate);
+  dict.SetBoolean("should_suppress_notifications",
+                  config.activation_options.should_suppress_notifications);
+  dict.SetBoolean("should_whitelist_site_on_reload",
+                  config.activation_options.should_whitelist_site_on_reload);
+  dict.SetString("ruleset_flavor",
+                 StreamToString(config.general_settings.ruleset_flavor));
+  std::string json;
+  base::JSONWriter::WriteWithOptions(
+      dict, base::JSONWriter::OPTIONS_PRETTY_PRINT, &json);
+  return os << json;
+}
+
+// ConfigurationList ----------------------------------------------------------
+
+ConfigurationList::ConfigurationList(std::vector<Configuration> configs)
+    : configs_by_decreasing_priority_(
+          SortConfigsByDecreasingPriority(std::move(configs))) {}
 ConfigurationList::ConfigurationList(Configuration config)
-    : config_(std::move(config)) {}
+    : ConfigurationList(MakeConfigVector(std::move(config))) {}

[pkalinnikov, 2017/05/04 12:04:22]

nit1: Can this be just {std::move(config)} instead of MakeConfigVector?
nit to nit1: Can we directly initialize |configs_by_decreasing_priority_| with
this syntax, to skip a 1-element vector sorting?

[engedy, 2017/05/05 12:25:42]

Initializer lists don't take rvalues, so there will be a copy here, but maybe
that's worth it given that this is called once during the process lifetime, and
can allow us to remove that entire helper method.

I just added some more functionality to the ctor, so it's no longer futile to
call it.

 ConfigurationList::~ConfigurationList() = default;
 
-scoped_refptr<ConfigurationList> GetActiveConfigurations() {
+std::string ConfigurationList::GetMostSpecificRulesetFlavor() const {

[pkalinnikov, 2017/05/04 12:04:22]

Since the ConfigurationList is immutable, could we precompute this in ctor?
Alternatively, cache to a member variable on the first call?

Although, seems like this function is called only once in
SubresourceFilterComponentInstallerTraits. If it's not called frequently, then
just ignore this comment.

[engedy, 2017/05/05 12:25:42]

Done. It's more consistent either way.

+  std::string most_specific_flavor;

[Charlie Harrison, 2017/05/03 14:52:17]

Can it be const ref so we don't copy the underlying string in the loop?

[pkalinnikov, 2017/05/04 12:04:22]

+1, const ptr.

[engedy, 2017/05/05 12:25:42]

Done.

[engedy, 2017/05/05 12:25:42]

Well we cannot re-assign a const&, but StringPiece saves the day.

+  for (const auto& config : configs_by_decreasing_priority_) {
+    const std::string& flavor = config.general_settings.ruleset_flavor;
+    if (flavor.size() > most_specific_flavor.size() ||
+        (flavor.size() == most_specific_flavor.size() &&
+         flavor > most_specific_flavor)) {
+      most_specific_flavor = flavor;
+    }
+  }
+  return most_specific_flavor;
+}
+
+scoped_refptr<ConfigurationList> GetEnabledConfigurations() {
   base::AutoLock lock(g_active_configurations_lock.Get());
   if (!g_active_configurations.Get()) {
     g_active_configurations.Get() =
-        base::MakeShared<ConfigurationList>(ParseFieldTrialConfiguration());
+        base::MakeShared<ConfigurationList>(ParseEnabledConfigurations());
   }
   return g_active_configurations.Get();
 }