Add support for multiple simultaneous subresource_filter::Configurations.

components/subresource_filter/core/browser/subresource_filter_features.h

Index: components/subresource_filter/core/browser/subresource_filter_features.h
diff --git a/components/subresource_filter/core/browser/subresource_filter_features.h b/components/subresource_filter/core/browser/subresource_filter_features.h
index c6106319cac4c7ab993d0b3b08c1a29f86f65257..162c6d759f4d6debc3d8f54f62f939a3d5d868c5 100644
--- a/components/subresource_filter/core/browser/subresource_filter_features.h
+++ b/components/subresource_filter/core/browser/subresource_filter_features.h
@@ -5,6 +5,9 @@
 #ifndef COMPONENTS_SUBRESOURCE_FILTER_CORE_BROWSER_SUBRESOURCE_FILTER_FEATURES_H_
 #define COMPONENTS_SUBRESOURCE_FILTER_CORE_BROWSER_SUBRESOURCE_FILTER_FEATURES_H_
 
+#include <iosfwd>
+#include <vector>
+
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -14,63 +17,121 @@
 
 namespace subresource_filter {
 
-// Encapsulates all parameters that define how the subresource filter feature
-// should operate.
+// Encapsulates a set of parameters that define how the subresource filter
+// feature should operate. Each configuration consists of three parts as
+// described in detail below.
+//
+// There can be multiple configuration enabled at the same time. For each
+// navigation, however, subresource filtering will be activated according to
+// exactly one of these enabled configuration, if any. Namely, the configuration
+// with the highest priority among those whose |activation_conditions| are
+// satisfied for the navigation.

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

A note about the specific case of the ruleset flavor would be good here.

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

Done.

+//
+// The priorities for configurations are defined as follows. The configuration
+// with the higher priority is:
+//  -- the one with the higher |activation_level|,
+//  -- in case of a tie, the one with the more specific |activation_scope|,
+//  -- in case of a tie, the one with the more recently added |activation_list|,
+//  -- then at this point there should not be a tie anymore. If there is one, a
+//     configuration will be selected deterministically, but arbitrarily.
+// See details in the .cc file and the example ordering in the unit test.
 struct Configuration {
+  // The conditions that determine whether subresource filtering should be
+  // activated for a given main frame navigation using this configuration.
+  struct ActivationConditions {
+    // The activation scope. That is, the subset of page loads where subresource
+    // filtering should be activated.
+    ActivationScope activation_scope = ActivationScope::NO_SITES;
+
+    // The activation list to use when the |activation_scope| is
+    // ACTIVATION_LIST, ignored otherwise.
+    ActivationList activation_list = ActivationList::NONE;
+  };
+
+  // The details of how subresource filtering should operate for a given main
+  // frame navigation if the conditions above are met (and this is the highest
+  // priority configuration among those whose conditions are met).
+  struct ActivationOptions {
+    // The maximum degree to which subresource filtering should be activated on
+    // any RenderFrame.
+    ActivationLevel activation_level = ActivationLevel::DISABLED;
+
+    // A number in the range [0, 1], indicating the fraction of page loads that
+    // should have extended performance measurements enabled.
+    double performance_measurement_rate = 0.0;
+
+    // Whether notifications indicating that a subresource was disallowed should
+    // be suppressed in the UI.
+    bool should_suppress_notifications = false;
+
+    // Whether to whitelist a site when a page loaded from that site is
+    // reloaded.
+    bool should_whitelist_site_on_reload = false;
+  };
+
+  // General settings the apply outside of the scope a navigation.

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

nit: ... that apply ... of the scope of a ...

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

Done.

+  struct GeneralSettings {
+    // The ruleset flavor to download through the component updater. The empty
+    // string indicates that the default ruleset should be used.
+    std::string ruleset_flavor;
+  };
+

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

Please add a comment somewhere around here to update operator== and any other
necessary methods if any new members are added.

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

Done. Do you know of any cool ways to static_assert things like this?

[Charlie Harrison, 2017/05/05 13:12:52]

Not in a clean way, though I wouldn't call myself a C++ guru.

[pkalinnikov, 2017/05/05 13:29:58]

One weird way would be smth like:
static_assert(sizeof(Configuration) == ###, "Have you updated operator==() and
this assert?");

Not sure this would be portable.

[Charlie Harrison, 2017/05/05 14:00:08]

I don't think this is very portable, but there are some places in Blink that do
something like this to ensure classes don't get too big (StringImpl is one
example).

[engedy, 2017/05/05 19:24:10]

Yeah, that's very not portable. Even within the same platform, the size is
different based on DCHECKs being enabled.

   Configuration();
   Configuration(ActivationLevel activation_level,
                 ActivationScope activation_scope,
                 ActivationList activation_list = ActivationList::NONE);
+  Configuration(const Configuration&);
   Configuration(Configuration&&);
   ~Configuration();
+  Configuration& operator=(const Configuration&);
   Configuration& operator=(Configuration&&);
 
-  // The maximum degree to which subresource filtering should be activated on
-  // any RenderFrame. This will be ActivationLevel::DISABLED unless the feature
-  // is enabled and variation parameters prescribe a higher activation level.
-  ActivationLevel activation_level = ActivationLevel::DISABLED;
-
-  // The activation scope. That is, the subset of page loads where subresource
-  // filtering should be activated. This will be ActivationScope::NO_SITES
-  // unless the feature is =enabled and variation parameters prescribe a wider
-  // activation scope.
-  ActivationScope activation_scope = ActivationScope::NO_SITES;
-
-  // The activation list to use when the |activation_scope| is ACTIVATION_LIST.
-  // This will be ActivationList::NONE unless variation parameters prescribe a
-  // recognized list.
-  ActivationList activation_list = ActivationList::NONE;
-
-  // A number in the range [0, 1], indicating the fraction of page loads that
-  // should have extended performance measurements enabled. The rate will
-  // be 0 unless a greater frequency is specified by variation parameters.
-  double performance_measurement_rate = 0.0;
-
-  // Whether notifications indicating that a subresource was disallowed should
-  // be suppressed in the UI.
-  bool should_suppress_notifications = false;
-
-  // The ruleset flavor to download through the component updater. This or the
-  // empty string if the default ruleset should be used.
-  std::string ruleset_flavor;
-
-  // Whether to whitelist a site when a page loaded from that site is reloaded.
-  bool should_whitelist_site_on_reload = false;
+  bool operator==(const Configuration& rhs) const;
+  bool operator!=(const Configuration& rhs) const;
+
+  // Factory methods for preset configurations.
+  //
+  // To add a new preset:
+  //  1.) Define a named factory method here.
+  //  2.) Define a name for the configuration to be used in variation params.
+  //  3.) Register it into |kAvailablePresetConfigurations| in the .cc file.
+  //  4.) Update unittests to cover the new preset.
+  static Configuration MakePresetForLiveRunOnPhishingSites();
+  static Configuration MakePresetForPerformanceTestingDryRunOnAllSites();
+
+  ActivationConditions activation_conditions;
+  ActivationOptions activation_options;
+  GeneralSettings general_settings;
 };
 
-// TODO(engedy): Make this an actual list once all call sites are prepared to
-// handle multiple simultaneous configurations.
+// For logging in tests.
+std::ostream& operator<<(std::ostream& os, const Configuration& config);
+
+// Thread-safe, ref-counted wrapper around an immutable list of configurations.
 class ConfigurationList : public base::RefCountedThreadSafe<ConfigurationList> {
  public:
+  explicit ConfigurationList(std::vector<Configuration> configs);
   explicit ConfigurationList(Configuration config);
 
-  const Configuration& the_one_and_only() const { return config_; }
+  // Retrieves the configurations pre-sorted in decreasing order of priority
+  // (see the comment above the definition of Configuration for details).
+  const std::vector<Configuration>& configs_by_decreasing_priority() const {
+    return configs_by_decreasing_priority_;
+  }
+
+  // Returns the most specific ruleset flavor string that is prescribed by any
+  // of the configurations.
+  //
+  // The `most specific` flavor is defined as the longest flavor string, and, in

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

As discussed offline, can we get rid of length-first comparison, and just
compare the flavors lexicographically? For current flavors ("" and a-d) it works
the same way, but is a bit shorter to explain/code. Also, it could potentially
allow some sort of versioning on flavors, e.g., 2.1 would be more specific than
1 and 1.1.

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

Done.

+  // case of a tie, the lexicographically greatest among those. The ruleset
+  // flavor is derived independently of activation `priorities`.

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

A comment why this is the case would be nice.

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

No longer applicable.

+  std::string GetMostSpecificRulesetFlavor() const;
 
  private:
   friend class base::RefCountedThreadSafe<ConfigurationList>;
   ~ConfigurationList();
 
-  const Configuration config_;
+  const std::vector<Configuration> configs_by_decreasing_priority_;
 
   DISALLOW_COPY_AND_ASSIGN(ConfigurationList);
 };
@@ -78,13 +139,13 @@ class ConfigurationList : public base::RefCountedThreadSafe<ConfigurationList> {
 // Retrieves all currently enabled subresource filtering configurations. The
 // configurations are parsed on first access and then the result is cached.
 //
-// In tests, however, the config may be altered in-between navigations, so
+// In tests, however, the config may be changed in-between navigations, so
 // callers should not hold on to the result for long.
-scoped_refptr<ConfigurationList> GetActiveConfigurations();
+scoped_refptr<ConfigurationList> GetEnabledConfigurations();
 
 namespace testing {
 
-// Returns the currently cached active ConfigurationList, if any, and replaces
+// Returns the currently cached enabled ConfigurationList, if any, and replaces
 // it with |new_configs|, which may be nullptr to clear the cache.
 scoped_refptr<ConfigurationList> GetAndSetActivateConfigurations(
     scoped_refptr<ConfigurationList> new_configs);
@@ -115,14 +176,18 @@ extern const char kActivationListSocialEngineeringAdsInterstitial[];
 extern const char kActivationListPhishingInterstitial[];
 extern const char kActivationListSubresourceFilter[];
 
-extern const char kRulesetFlavorParameterName[];
-
 extern const char kPerformanceMeasurementRateParameterName[];
 
 extern const char kSuppressNotificationsParameterName[];
 
 extern const char kWhitelistSiteOnReloadParameterName[];
 
+extern const char kRulesetFlavorParameterName[];
+
+extern const char kEnablePresetsParameterName[];
+extern const char kPresetLiveRunOnPhishingSites[];
+extern const char kPresetPerformanceTestingDryRunOnAllSites[];
+
 }  // namespace subresource_filter
 
 #endif  // COMPONENTS_SUBRESOURCE_FILTER_CORE_BROWSER_SUBRESOURCE_FILTER_FEATURES_H_