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

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

Created:
6 months, 3 weeks ago by engedy
Modified:
6 months, 2 weeks 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
Trybot results:  win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   mac_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   ios-simulator-xcode-clang   ios-simulator   ios-device-xcode-clang   ios-device   linux_chromium_tsan_rel_ng   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromeos_daisy_chromium_compile_only_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   cast_shell_android   linux_android_rel_ng   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   chromium_presubmit   chromium_presubmit   android_n5x_swarming_rel   linux_android_rel_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-device-xcode-clang   ios-simulator-xcode-clang   ios-simulator   ios-device   linux_chromium_compile_dbg_ng   linux_chromium_tsan_rel_ng   linux_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_asan_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromium_presubmit   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   linux_android_rel_ng   cast_shell_linux   cast_shell_android   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_arm64_dbg_recipe   android_clang_dbg_recipe 

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 37 (25 generated)
engedy
@Charlie, could you PTAL?
6 months, 3 weeks 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 ...
6 months, 3 weeks 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 ...
6 months, 3 weeks 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
6 months, 3 weeks 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 ...
6 months, 3 weeks ago (2017-04-25 20:29:13 UTC) #18
Sorin Jianu
lgtm component updater lgtm Thank you!
6 months, 3 weeks 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
6 months, 3 weeks 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)
6 months, 3 weeks ago (2017-04-26 17:15:21 UTC) #30
engedy
@Nathan, PTAL.
6 months, 3 weeks ago (2017-04-26 17:54:17 UTC) #31
Nathan Parker
Sorry for the delay -- safe_browsing/* LGTM
6 months, 2 weeks 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
6 months, 2 weeks ago (2017-05-03 10:27:34 UTC) #34
commit-bot: I haz the power
6 months, 2 weeks 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 efc10ee0f