Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 2838193002: Split the ScopedSubresourceFilterFeatureToggle into two helper classes. (Closed)

Created:
1 year, 2 months ago by engedy
Modified:
1 year, 2 months ago
CC:
chromium-reviews, subresource-filter-reviews_chromium.org, vakh+watch_chromium.org, grt+watch_chromium.org, jam, timvolodine, darin-cc_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Optimize more things away. #

Patch Set 3 : Polish. #

Patch Set 4 : More polish. #

Total comments: 19

Patch Set 5 : Addressed comments. #

Patch Set 6 : Phrasing. #

Patch Set 7 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+355 lines, -372 lines) Patch
M chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc View 1 2 3 4 5 6 chunks +15 lines, -34 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc View 1 2 3 4 3 chunks +13 lines, -12 lines 0 comments Download
M chrome/browser/subresource_filter/subresource_filter_browsertest.cc View 1 2 14 chunks +52 lines, -93 lines 0 comments Download
M chrome/browser/subresource_filter/subresource_filter_unittest.cc View 1 2 4 chunks +12 lines, -13 lines 0 comments Download
M components/safe_browsing_db/v4_database.h View 1 2 chunks +2 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 17 chunks +79 lines, -102 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc View 1 2 3 4 3 chunks +7 lines, -6 lines 0 comments Download
M components/subresource_filter/core/browser/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features.h View 1 2 3 4 3 chunks +12 lines, -8 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features.cc View 1 2 3 4 2 chunks +12 lines, -3 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features_test_support.h View 1 2 1 chunk +33 lines, -19 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features_test_support.cc View 1 2 3 4 1 chunk +28 lines, -61 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features_unittest.cc View 1 2 3 4 9 chunks +89 lines, -19 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 37 (25 generated)
engedy
@Charlie, could you PTAL?
1 year, 2 months ago (2017-04-25 19:10:02 UTC) #9
Charlie Harrison
Generally LGTM % some nits. Thanks for the great cleanup! https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#newcode909 ...
1 year, 2 months ago (2017-04-25 19:36:22 UTC) #10
engedy
Addressed all comments except one. Sorry for the many nits. https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2838193002/diff/60001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#newcode909 ...
1 year, 2 months ago (2017-04-25 20:13:25 UTC) #13
engedy
@Sorin please review as OWNERs of: c/b/component_updater @Nathan please review as OWNERs of: chrome/browser/safe_browsing components/safe_browsing_db
1 year, 2 months ago (2017-04-25 20:15:33 UTC) #15
Charlie Harrison
https://codereview.chromium.org/2838193002/diff/60001/components/subresource_filter/core/browser/subresource_filter_features_test_support.h File components/subresource_filter/core/browser/subresource_filter_features_test_support.h (right): https://codereview.chromium.org/2838193002/diff/60001/components/subresource_filter/core/browser/subresource_filter_features_test_support.h#newcode31 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 ...
1 year, 2 months ago (2017-04-25 20:29:13 UTC) #18
Sorin Jianu
lgtm component updater lgtm Thank you!
1 year, 2 months ago (2017-04-26 16:59:14 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2838193002/120001
1 year, 2 months ago (2017-04-26 17:07:01 UTC) #28
commit-bot: I haz the power
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_presubmit/builds/420982)
1 year, 2 months ago (2017-04-26 17:15:21 UTC) #30
engedy
@Nathan, PTAL.
1 year, 2 months ago (2017-04-26 17:54:17 UTC) #31
Nathan Parker
Sorry for the delay -- safe_browsing/* LGTM
1 year, 2 months ago (2017-05-03 00:11:46 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2838193002/120001
1 year, 2 months ago (2017-05-03 10:27:34 UTC) #34
commit-bot: I haz the power
1 year, 2 months ago (2017-05-03 11:22:27 UTC) #37
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/98fda4cef846209efce2b3d5c1e4...

Powered by Google App Engine
This is Rietveld 408576698