Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

The ScopedSubresourceFilterFeatureToggle remains, but is reduced to be
responsible for overriding only the base::FeatureList state.

The ScopedSubresourceFilterConfigurator is introduced as the new way for
overriding the configuration returned by GetActiveConfigurations(), and has the
following advantages:
  -- It does so regardless of the current field trial, feature list, and/or
     variation parameters states, thus the configuration can be changed
     multiple times during a single test.
  -- It can be used without a global base::FieldTrialList singleton, or setting
     up variations parameters.
  -- It will allow setting multiple simultaneous configurations conveniently
     without needing to first encode them as variation parameters.

BUG=708181
Review-Url: https://codereview.chromium.org/2838193002
Cr-Commit-Position: refs/heads/master@{#468939}
Committed: https://chromium.googlesource.com/chromium/src/+/98fda4cef846209efce2b3d5c1e4d6d0ab560a7e

[engedy, 2017-04-25 18:58:29]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:59:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 19:05:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 19:05:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[engedy, 2017-04-25 19:06:07]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 19:06:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-04-25 19:09:35]

Description was changed from

==========
Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

The ScopedSubresourceFilterFeatureToggle remains, but is now only responsible
for setting the base::FeatureList state in a few some unit tests that need it.

The ScopedSubresourceFilterConfigurator is introduced as the new way for
overriding the configuration returned by GetActiveConfigurations(), and has the
following advantages:
  -- It does so regardless of the current field trial, feature list, and/or
     variation parameters states, and therefore can be instantiated multiple
     times to alter the configuration multiple times during a test.
  -- It can be used without a global base::FieldTrialList, or variations.
  -- It will allow setting multiple simultaneous configurations conveniently
     without needing to first encode them as variation parameters.

BUG=708181
==========

to

==========
Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

The ScopedSubresourceFilterFeatureToggle remains, but is reduced to be
responsible for overriding only the base::FeatureList state.

The ScopedSubresourceFilterConfigurator is introduced as the new way for
overriding the configuration returned by GetActiveConfigurations(), and has the
following advantages:
  -- It does so regardless of the current field trial, feature list, and/or
     variation parameters states, thus the configuration can be changed
     multiple times during a single test.
  -- It can be used without a global base::FieldTrialList singleton, or setting
     up variations parameters.
  -- It will allow setting multiple simultaneous configurations conveniently
     without needing to first encode them as variation parameters.

BUG=708181
==========

[engedy, 2017-04-25 19:09:35]

engedy@chromium.org changed reviewers:
+ csharrison@chromium.org

[engedy, 2017-04-25 19:10:02]

@Charlie, could you PTAL?

[Charlie Harrison, 2017-04-25 19:36:22]

Generally LGTM % some nits. Thanks for the great cleanup!

https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:909:
subresource_filter::ActivationLevel::ENABLED,
#include activation_{scope,level,list}

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:113:
ActivationLevel::ENABLED, ActivationScope::ACTIVATION_LIST,
#include activation_list, activation_scope, and activation_level.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:89:
scoped_refptr<ConfigurationList> ReplaceCachedActiveConfigurations(
This is like a get-and-set right? Maybe
GetAndSetActiveConfigurations(scoped_refptr<ConfigurationList> new_configs)?

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_test_support.h
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_test_support.h:31:
scoped_refptr<ConfigurationList> original_config_;
#include ref_counted

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:26:
base::FeatureList::OverrideState feature_state,
#include feature_list

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:27:
std::map<std::string, std::string> variation_params)
#include <map>

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:35:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
#include <memory>

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:35:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
can you use base::MakeUnique?

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:51:
DISALLOW_COPY_AND_ASSIGN(ScopedExperimentalStateToggle);
#include base/macros

[engedy, 2017-04-25 20:12:46]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 20:13:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2017-04-25 20:13:25]

Addressed all comments except one. Sorry for the many nits.

https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:909:
subresource_filter::ActivationLevel::ENABLED,
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> #include activation_{scope,level,list}

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:113:
ActivationLevel::ENABLED, ActivationScope::ACTIVATION_LIST,
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> #include activation_list, activation_scope, and activation_level.

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:89:
scoped_refptr<ConfigurationList> ReplaceCachedActiveConfigurations(
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> This is like a get-and-set right? Maybe
> GetAndSetActiveConfigurations(scoped_refptr<ConfigurationList> new_configs)?

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_test_support.h
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_test_support.h:31:
scoped_refptr<ConfigurationList> original_config_;
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> #include ref_counted

Currently, both the RefCounted base classes, as well as scoped_refptr is defined
in this header, so a scoped_refptr-compatible cannot be defined without
including this header. WDYT? :)

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:26:
base::FeatureList::OverrideState feature_state,
On 2017/04/25 19:36:22, Charlie Harrison wrote:
> #include feature_list

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:27:
std::map<std::string, std::string> variation_params)
On 2017/04/25 19:36:22, Charlie Harrison wrote:
> #include <map>

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:35:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> can you use base::MakeUnique?

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:35:
std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
On 2017/04/25 19:36:21, Charlie Harrison wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:51:
DISALLOW_COPY_AND_ASSIGN(ScopedExperimentalStateToggle);
On 2017/04/25 19:36:22, Charlie Harrison wrote:
> #include base/macros

Done.

[engedy, 2017-04-25 20:15:32]

engedy@chromium.org changed reviewers:
+ nparker@chromium.org, sorin@chromium.org

[engedy, 2017-04-25 20:15:33]

@Sorin please review as OWNERs of:
  c/b/component_updater

@Nathan please review as OWNERs of:
  chrome/browser/safe_browsing
  components/safe_browsing_db

[engedy, 2017-04-25 20:19:06]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 20:19:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-04-25 20:29:13]

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
File
components/subresource_filter/core/browser/subresource_filter_features_test_support.h
(right):

https://codereview.chromium.org/2838193002/diff/60001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features_test_support.h:31:
scoped_refptr<ConfigurationList> original_config_;
On 2017/04/25 20:13:24, engedy wrote:
> On 2017/04/25 19:36:21, Charlie Harrison wrote:
> > #include ref_counted
> 
> Currently, both the RefCounted base classes, as well as scoped_refptr is
defined
> in this header, so a scoped_refptr-compatible cannot be defined without
> including this header. WDYT? :)

Gotcha, still lgtm

[commit-bot: I haz the power, 2017-04-25 21:48:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 21:48:48]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-04-26 09:59:30]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 09:59:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 11:31:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 11:31:52]

Dry run: This issue passed the CQ dry run.

[Sorin Jianu, 2017-04-26 16:59:14]

lgtm

component updater lgtm

Thank you!

[engedy, 2017-04-26 17:06:41]

The CQ bit was checked by engedy@chromium.org

[engedy, 2017-04-26 17:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2838193002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-26 17:07:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 17:15:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 17:15:21]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[engedy, 2017-04-26 17:54:17]

@Nathan, PTAL.

[Nathan Parker, 2017-05-03 00:11:46]

Sorry for the delay -- safe_browsing/* LGTM

[engedy, 2017-05-03 10:27:24]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2017-05-03 10:27:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 11:22:14]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493807244174040,
"parent_rev": "76928f07e2ff7aa3a91b99ca422f6aff1676eaab", "commit_rev":
"98fda4cef846209efce2b3d5c1e4d6d0ab560a7e"}

[commit-bot: I haz the power, 2017-05-03 11:22:25]

Description was changed from

==========
Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

The ScopedSubresourceFilterFeatureToggle remains, but is reduced to be
responsible for overriding only the base::FeatureList state.

The ScopedSubresourceFilterConfigurator is introduced as the new way for
overriding the configuration returned by GetActiveConfigurations(), and has the
following advantages:
  -- It does so regardless of the current field trial, feature list, and/or
     variation parameters states, thus the configuration can be changed
     multiple times during a single test.
  -- It can be used without a global base::FieldTrialList singleton, or setting
     up variations parameters.
  -- It will allow setting multiple simultaneous configurations conveniently
     without needing to first encode them as variation parameters.

BUG=708181
==========

to

==========
Split the ScopedSubresourceFilterFeatureToggle into two helper classes.

The ScopedSubresourceFilterFeatureToggle remains, but is reduced to be
responsible for overriding only the base::FeatureList state.

The ScopedSubresourceFilterConfigurator is introduced as the new way for
overriding the configuration returned by GetActiveConfigurations(), and has the
following advantages:
  -- It does so regardless of the current field trial, feature list, and/or
     variation parameters states, thus the configuration can be changed
     multiple times during a single test.
  -- It can be used without a global base::FieldTrialList singleton, or setting
     up variations parameters.
  -- It will allow setting multiple simultaneous configurations conveniently
     without needing to first encode them as variation parameters.

BUG=708181

Review-Url: https://codereview.chromium.org/2838193002
Cr-Commit-Position: refs/heads/master@{#468939}
Committed:
https://chromium.googlesource.com/chromium/src/+/98fda4cef846209efce2b3d5c1e4...
==========

[commit-bot: I haz the power, 2017-05-03 11:22:27]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/98fda4cef846209efce2b3d5c1e4...