Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844063002:
Add support for multiple simultaneous subresource_filter::Configurations. (Closed)
|
Created:
3 years, 7 months ago by engedy Modified:
3 years, 7 months ago Reviewers:
pkalinnikov, Charlie Harrison, Sorin Jianu, Alexei Svitkine (slow), Bryan McQuade, Nathan Parker CC:
chromium-reviews, darin-cc_chromium.org, grt+watch_chromium.org, jam, subresource-filter-reviews_chromium.org, timvolodine, vakh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for multiple simultaneous subresource_filter::Configurations.
Also introduce the concept of presets, which will be used in a follow-up CL
to enable the feature on phishing sites by default.
BUG=708181
Review-Url: https://codereview.chromium.org/2844063002
Cr-Commit-Position: refs/heads/master@{#470559}
Committed: https://chromium.googlesource.com/chromium/src/+/610e540fa3179c3a5ed8eb91b7d48e935c67d86d
Patch Set 1 : Correct base. #Patch Set 2 : Minimal polish, still missing unittests for multiple simultaneous configs. #
Total comments: 6
Patch Set 3 : Further tests. #Patch Set 4 : Polish + rebase. #
Total comments: 61
Patch Set 5 : Addressed all comments except traces and refactoring. #Patch Set 6 : Rebase. #
Total comments: 3
Patch Set 7 : Fix compile error. #
Total comments: 2
Patch Set 8 : Tests + polish. #Patch Set 9 : Comment typo. #Patch Set 10 : Comment nit. #
Total comments: 16
Patch Set 11 : Fix unit_tests. #Patch Set 12 : Add tests for disabled presets. #Patch Set 13 : Polish factory_unittests. #Patch Set 14 : Addressed comments from csharrison@. #
Total comments: 10
Patch Set 15 : Address all comments. #
Total comments: 2
Patch Set 16 : Addressed nit from sorin@. #Patch Set 17 : Rebase. #
Total comments: 2
Messages
Total messages: 108 (70 generated)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
engedy@chromium.org changed reviewers: + csharrison@chromium.org
Charlie, this is severely lacking unittests, but could you please take a high-level look at the configuration selection logic?
On 2017/04/26 20:58:57, engedy wrote: > Charlie, this is severely lacking unittests, but could you please take a > high-level look at the configuration selection logic? It also needs some polish, so don't bother with nits just yet. :)
Oh man does this code make me nervous :P I think this deserves some major comments for how it works, because figuring out what exactly is running will start being really tricky. https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:124: constexpr ActivationLevel kValuesInIncreasingOrderOfPriority[] = { I see the appeal here but maybe it would just be easier to implement these as switch/case statements? You get the compile time checking for free without static asserts, you don't need to use std::find, and I don't think updating them will be too big a hassle. https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:164: return GetConfigurationPriority(a) > GetConfigurationPriority(b); How does this operator work (can you include a comment)? I assume it compares by the first elt and goes on to the next elt in case of a tie? https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:226: std::string ConfigurationList::GetMostSpecificRulesetFlavor() const { Please comment what "most specific" means and how you've implemented it. I don't readily see how the comparator yields the most specific.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed comments, added a whole bunch of comments and tests, and made things more readable. The one thing missing are the DriverFactory unittests for multiple enabled configs, which are coming very shortly. Please take another look at the rest in the meantime. https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:124: constexpr ActivationLevel kValuesInIncreasingOrderOfPriority[] = { On 2017/04/26 21:28:11, Charlie (ooo-ish until may 2) wrote: > I see the appeal here but maybe it would just be easier to implement these as > switch/case statements? > > You get the compile time checking for free without static asserts, you don't > need to use std::find, and I don't think updating them will be too big a hassle. Done. https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:164: return GetConfigurationPriority(a) > GetConfigurationPriority(b); On 2017/04/26 21:28:11, Charlie (ooo-ish until may 2) wrote: > How does this operator work (can you include a comment)? I assume it compares by > the first elt and goes on to the next elt in case of a tie? Yep, added comment. https://codereview.chromium.org/2844063002/diff/40001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:226: std::string ConfigurationList::GetMostSpecificRulesetFlavor() const { On 2017/04/26 21:28:11, Charlie (ooo-ish until may 2) wrote: > Please comment what "most specific" means and how you've implemented it. I don't > readily see how the comparator yields the most specific. Documented in the header, and cleaned up the implementation.
Description was changed from ========== Add support for multiple simultaneous subresource_filter::Configurations. BUG=708181 ========== to ========== Add support for multiple simultaneous subresource_filter::Configurations. BUG=708181 ==========
engedy@chromium.org changed reviewers: + pkalinnikov@chromium.org
+Pavel, PTAL as well.
Since driver factory is going away do you think we could implement the tests in another layer (subresource_filter_unittests)? Up to you.
On 2017/05/03 12:15:29, Charlie Harrison wrote: > Since driver factory is going away do you think we could implement the tests in > another layer (subresource_filter_unittests)? Up to you. Good point. I'll take a look, not sure if it's technically possible yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Flushing some initial comments. https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/subresource_filter_component_installer.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/subresource_filter_component_installer.cc:122: // In addition to "", we allow 4 ruleset flavor identifiers: a, b, c, d. Move this comment above the empty() check. https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:276: {"invalid", {"foo", "a", "b"}}}; Can we include valid configs with lengths > 1 to ensure we use the most specific one? Using 0 and 1 length strings seems very edge-case-y https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:912: MakePresetForLiveRunOnPhishingSites()); Ahh, much better :D https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:160: // The activation decision corresponding to the most recently _started_ Optional: Since driver factory is going away this is unfortunate. I wonder if we can reduce the churn here by encapsulating this logic in a helper class (something like ScopedPageActivationDecision). If we can plug something like that into the SB throttle (and maybe move it into the throttle manager / filter client) it would be pretty cool. I think the only state necessary in this helper would be a pointer to the client? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:177: A comment here about what the priority levels mean (is 3 higher priority than 1?) would be helpful. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:321: std::ostream& operator<<(std::ostream& os, const Configuration& config) { 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. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:353: std::string most_specific_flavor; Can it be const ref so we don't copy the underlying string in the loop? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:28: // satisfied for the navigation. A note about the specific case of the ruleset flavor would be good here. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: Please add a comment somewhere around here to update operator== and any other necessary methods if any new members are added. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:127: // flavor is derived independently of activation `priorities`. A comment why this is the case would be nice.
Added some comments. On a high level, do we really need that complicated priority system? Despite it is quite uneasy to absorb, it looks like there are only 9 meaningful priorities: (3 activation levels) x (3 activation scopes). The ActivationList tie-breaker seems only necessary for making it deterministic. Besides. Suppose LiveRunOnPhishingSites is included by default. Do I understand correctly that there is no way to override it if one wants to DISABLE on ALL_SITES for testing or experimental purposes? Could we think of a simpler (and more flexible) priority system? For instance, we could explicitly assign a priority to each config. Or just base the priorities on their order in the trials list. Balazs, Charlie, WDYT? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. g/that/than/ https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. If we have 2 configs, and both return different non-disabled |decision|s, do we not care which one returned what? Looks hacky, but makes sense to me because this class seems only caring about UNKNOWN, ACTIVATED and ACTIVATION_DISABLED. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:251: navigation_chain_.clear(); optional: navigation_chain_.assign(1, navigation_handle->GetURL()); https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:275: if (config_list->configs_by_decreasing_priority().empty()) Given that ParseEnabledConfigurations always pushes_back a default Configuration, this shouldn't happen, right? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:158: configs.push_back(std::move(experimental_config)); 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? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:194: case ActivationScope::NO_SITES: 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. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:321: std::ostream& operator<<(std::ostream& os, const Configuration& config) { On 2017/05/03 14:52:18, Charlie Harrison wrote: > 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. +1 Does trace work well with multi-line strings? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:349: : ConfigurationList(MakeConfigVector(std::move(config))) {} 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? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:352: std::string ConfigurationList::GetMostSpecificRulesetFlavor() const { 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. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:353: std::string most_specific_flavor; On 2017/05/03 14:52:17, Charlie Harrison wrote: > Can it be const ref so we don't copy the underlying string in the loop? +1, const ptr. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:72: // General settings the apply outside of the scope a navigation. nit: ... that apply ... of the scope of a ... https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:125: // The `most specific` flavor is defined as the longest flavor string, and, in 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. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:422: const Configuration kConfigurationsByDecreasingPriority[] = { nit: Can you make this const std::vector<Configuration> to get rid of AsVector? And assign it directly to |expected_order_by_decreasing_priority|? And rename it to |configurations_by_decreasing_priority|? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:437: Configuration() /* default constructor */ nit: comma? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:446: scoped_configuration(std::move(reverse_order)); #include <utility> for std::move.
I tend to agree with Pavel here, the priority system should be as simple as possible to explain. I don't see a huge problem with just going by (order of finch configs, order of default configs). However, we still wouldn't have a way to turn off the default configs using this (the core logic for activation would need to change too).
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All done except the refactoring and TRACE_EVENTs. Please take another look. https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/subresource_filter_component_installer.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/subresource_filter_component_installer.cc:122: // In addition to "", we allow 4 ruleset flavor identifiers: a, b, c, d. On 2017/05/03 14:52:17, Charlie Harrison wrote: > Move this comment above the empty() check. Done. https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... File chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/componen... chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:276: {"invalid", {"foo", "a", "b"}}}; On 2017/05/03 14:52:17, Charlie Harrison wrote: > Can we include valid configs with lengths > 1 to ensure we use the most specific > one? Using 0 and 1 length strings seems very edge-case-y Because of the checks in the production code, we don't have much slack here, but I added some. There is another test that does that, though: SubresourceFilterFeaturesTest.MostSpecificRulesetFlavor. https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:912: MakePresetForLiveRunOnPhishingSites()); On 2017/05/03 14:52:17, Charlie Harrison wrote: > Ahh, much better :D :-) https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. On 2017/05/04 12:04:22, pkalinnikov wrote: > g/that/than/ Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. On 2017/05/04 12:04:22, pkalinnikov wrote: > If we have 2 configs, and both return different non-disabled |decision|s, do we > not care which one returned what? > > Looks hacky, but makes sense to me because this class seems only caring about > UNKNOWN, ACTIVATED and ACTIVATION_DISABLED. Yes and yes, but agreed that it's a bit hacky. Glad to improve this if you have any ideas. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:251: navigation_chain_.clear(); On 2017/05/04 12:04:22, pkalinnikov wrote: > optional: navigation_chain_.assign(1, navigation_handle->GetURL()); How about this? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:160: // The activation decision corresponding to the most recently _started_ On 2017/05/03 14:52:17, Charlie Harrison wrote: > Optional: > > Since driver factory is going away this is unfortunate. I wonder if we can > reduce the churn here by encapsulating this logic in a helper class (something > like ScopedPageActivationDecision). If we can plug something like that into the > SB throttle (and maybe move it into the throttle manager / filter client) it > would be pretty cool. > > I think the only state necessary in this helper would be a pointer to the > client? We discussed offline that due to subtle timing constraints, this is complicated. But what did we agree on in the end? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:275: if (config_list->configs_by_decreasing_priority().empty()) On 2017/05/04 12:04:22, pkalinnikov wrote: > Given that ParseEnabledConfigurations always pushes_back a default > Configuration, this shouldn't happen, right? Yes, I made this cleaner. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:158: configs.push_back(std::move(experimental_config)); On 2017/05/04 12:04:22, pkalinnikov wrote: > 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? Yes, but it is mostly only needed because otherwise the tests would be really horrible. Let me know if you feel strongly about this :) https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:177: On 2017/05/03 14:52:17, Charlie Harrison wrote: > A comment here about what the priority levels mean (is 3 higher priority than > 1?) would be helpful. Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:194: case ActivationScope::NO_SITES: On 2017/05/04 12:04:22, pkalinnikov wrote: > 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. Removed this, N/A anymore. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:321: std::ostream& operator<<(std::ostream& os, const Configuration& config) { On 2017/05/03 14:52:18, Charlie Harrison wrote: > 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. To clarify, are we talking about SCOPED_TRACE's here? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:321: std::ostream& operator<<(std::ostream& os, const Configuration& config) { On 2017/05/04 12:04:23, pkalinnikov wrote: > On 2017/05/03 14:52:18, Charlie Harrison wrote: > > 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. > > +1 > > Does trace work well with multi-line strings? As clarified in our offline discussion, we are talking about TRACE_EVENTs here. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:349: : ConfigurationList(MakeConfigVector(std::move(config))) {} On 2017/05/04 12:04:22, pkalinnikov wrote: > 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? 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. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:352: std::string ConfigurationList::GetMostSpecificRulesetFlavor() const { On 2017/05/04 12:04:22, pkalinnikov wrote: > 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. Done. It's more consistent either way. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:353: std::string most_specific_flavor; On 2017/05/03 14:52:17, Charlie Harrison wrote: > Can it be const ref so we don't copy the underlying string in the loop? Well we cannot re-assign a const&, but StringPiece saves the day. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.cc:353: std::string most_specific_flavor; On 2017/05/04 12:04:22, pkalinnikov wrote: > On 2017/05/03 14:52:17, Charlie Harrison wrote: > > Can it be const ref so we don't copy the underlying string in the loop? > > +1, const ptr. Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:28: // satisfied for the navigation. On 2017/05/03 14:52:18, Charlie Harrison wrote: > A note about the specific case of the ruleset flavor would be good here. Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:72: // General settings the apply outside of the scope a navigation. On 2017/05/04 12:04:23, pkalinnikov wrote: > nit: ... that apply ... of the scope of a ... Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: On 2017/05/03 14:52:18, Charlie Harrison wrote: > Please add a comment somewhere around here to update operator== and any other > necessary methods if any new members are added. Done. Do you know of any cool ways to static_assert things like this? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:125: // The `most specific` flavor is defined as the longest flavor string, and, in On 2017/05/04 12:04:23, pkalinnikov wrote: > 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. Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:127: // flavor is derived independently of activation `priorities`. On 2017/05/03 14:52:18, Charlie Harrison wrote: > A comment why this is the case would be nice. No longer applicable. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:422: const Configuration kConfigurationsByDecreasingPriority[] = { On 2017/05/04 12:04:23, pkalinnikov wrote: > nit: Can you make this const std::vector<Configuration> to get rid of AsVector? > And assign it directly to |expected_order_by_decreasing_priority|? > And rename it to |configurations_by_decreasing_priority|? Done. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:437: Configuration() /* default constructor */ On 2017/05/04 12:04:23, pkalinnikov wrote: > nit: comma? I am not sure I understand. We don't have trailing commas anywhere in this test? https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:446: scoped_configuration(std::move(reverse_order)); On 2017/05/04 12:04:23, pkalinnikov wrote: > #include <utility> for std::move. Done.
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:160: // The activation decision corresponding to the most recently _started_ On 2017/05/05 12:25:41, engedy wrote: > On 2017/05/03 14:52:17, Charlie Harrison wrote: > > Optional: > > > > Since driver factory is going away this is unfortunate. I wonder if we can > > reduce the churn here by encapsulating this logic in a helper class (something > > like ScopedPageActivationDecision). If we can plug something like that into > the > > SB throttle (and maybe move it into the throttle manager / filter client) it > > would be pretty cool. > > > > I think the only state necessary in this helper would be a pointer to the > > client? > > We discussed offline that due to subtle timing constraints, this is complicated. > But what did we agree on in the end? I think keeping as-is is ok, and we'll rethink once the PageActivation is all in one place (the throttle). https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: On 2017/05/05 12:25:42, engedy wrote: > On 2017/05/03 14:52:18, Charlie Harrison wrote: > > Please add a comment somewhere around here to update operator== and any other > > necessary methods if any new members are added. > > Done. Do you know of any cool ways to static_assert things like this? Not in a clean way, though I wouldn't call myself a C++ guru. https://codereview.chromium.org/2844063002/diff/140001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/140001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:112: if (decision == ActivationDecision::ACTIVATED) { As Pavel pointed out I think the main problem here is that higher priority configs cannot stop activation. We have a preset to do DryRun on all sites. How do we turn that off?
https://codereview.chromium.org/2844063002/diff/140001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/140001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:112: if (decision == ActivationDecision::ACTIVATED) { On 2017/05/05 13:12:52, Charlie Harrison wrote: > As Pavel pointed out I think the main problem here is that higher priority > configs cannot stop activation. > > We have a preset to do DryRun on all sites. How do we turn that off? Ah, I slightly misunderstood the concern previously. I have added `disable_presets` already, but let me rework this a bit more so that ActivationOptions don't play a role in picking if a config applies or not, and the decision that gets recorded is cleaner.
Quick responses. Will do another round later. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. On 2017/05/05 12:25:41, engedy wrote: > On 2017/05/04 12:04:22, pkalinnikov wrote: > > If we have 2 configs, and both return different non-disabled |decision|s, do > we > > not care which one returned what? > > > > Looks hacky, but makes sense to me because this class seems only caring about > > UNKNOWN, ACTIVATED and ACTIVATION_DISABLED. > > Yes and yes, but agreed that it's a bit hacky. Glad to improve this if you have > any ideas. Maybe you could add a short explicit comment that this is deliberate? E.g.: // It doesn't matter which one of multiple possible decisions is recorded. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:251: navigation_chain_.clear(); On 2017/05/05 12:25:41, engedy wrote: > On 2017/05/04 12:04:22, pkalinnikov wrote: > > optional: navigation_chain_.assign(1, navigation_handle->GetURL()); > > How about this? Even better :) https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:275: if (config_list->configs_by_decreasing_priority().empty()) On 2017/05/05 12:25:42, engedy wrote: > On 2017/05/04 12:04:22, pkalinnikov wrote: > > Given that ParseEnabledConfigurations always pushes_back a default > > Configuration, this shouldn't happen, right? > > Yes, I made this cleaner. Yup, looks better. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: On 2017/05/05 12:25:42, engedy wrote: > On 2017/05/03 14:52:18, Charlie Harrison wrote: > > Please add a comment somewhere around here to update operator== and any other > > necessary methods if any new members are added. > > Done. Do you know of any cool ways to static_assert things like this? One weird way would be smth like: static_assert(sizeof(Configuration) == ###, "Have you updated operator==() and this assert?"); Not sure this would be portable. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:437: Configuration() /* default constructor */ On 2017/05/05 12:25:42, engedy wrote: > On 2017/05/04 12:04:23, pkalinnikov wrote: > > nit: comma? > > I am not sure I understand. We don't have trailing commas anywhere in this test? I suggested to put comma after /* default_constructor */, but I leave it up to you. https://codereview.chromium.org/2844063002/diff/120001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/120001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:315: return tie(*this) == tie(rhs); Belated "nice" :) https://codereview.chromium.org/2844063002/diff/120001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/120001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:85: // Do not forget updatin operator==, operator<<, and any other necessary nit: updating
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: On 2017/05/05 13:29:58, pkalinnikov wrote: > On 2017/05/05 12:25:42, engedy wrote: > > On 2017/05/03 14:52:18, Charlie Harrison wrote: > > > Please add a comment somewhere around here to update operator== and any > other > > > necessary methods if any new members are added. > > > > Done. Do you know of any cool ways to static_assert things like this? > > One weird way would be smth like: > static_assert(sizeof(Configuration) == ###, "Have you updated operator==() and > this assert?"); > > Not sure this would be portable. 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).
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Charlie, please take another look. Presets can now be disabled, and it is also possible to have a matching configuration that DISABLEs activation for a navigation. Hopefully this is cleaner now. This resulted in a slight change in the definition of the buckets in the histogram. I'd argue that it was slightly wrong before, so I wouldn't want to deprecate things. The changes are: -- ACTIVATION_LIST_NOT_MATCHED became ACTIVATION_CONDITIONS_NOT_MET, and is now recorded for all scope-failures, the new case being when the config does not apply because it's scope is NO_SITES -- ACTIVATION_DISABLED: this used to be recorded for the above case, but no longer. Now this is strictly recorded when the highest priority matching config prescribes ActivationLevel::DISABLED. -- URL_WHITELISTED: this is now only recorded when the activation conditions would otherwise be satisfied, and no longer for whitelisted URLs that do not match any configs. -- UNSUPPORTED_SCHEME: this is now the value reported for unsupported schemes, regardless of everything else. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:127: // Remember a |decision| more specific that `ACTIVATION_DISABLED`, if any. On 2017/05/05 13:29:58, pkalinnikov wrote: > On 2017/05/05 12:25:41, engedy wrote: > > On 2017/05/04 12:04:22, pkalinnikov wrote: > > > If we have 2 configs, and both return different non-disabled |decision|s, do > > we > > > not care which one returned what? > > > > > > Looks hacky, but makes sense to me because this class seems only caring > about > > > UNKNOWN, ACTIVATED and ACTIVATION_DISABLED. > > > > Yes and yes, but agreed that it's a bit hacky. Glad to improve this if you > have > > any ideas. > > Maybe you could add a short explicit comment that this is deliberate? E.g.: > // It doesn't matter which one of multiple possible decisions is recorded. I cleaned this up a bit, so this is no longer a concern. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features.h:78: On 2017/05/05 14:00:08, Charlie Harrison wrote: > On 2017/05/05 13:29:58, pkalinnikov wrote: > > On 2017/05/05 12:25:42, engedy wrote: > > > On 2017/05/03 14:52:18, Charlie Harrison wrote: > > > > Please add a comment somewhere around here to update operator== and any > > other > > > > necessary methods if any new members are added. > > > > > > Done. Do you know of any cool ways to static_assert things like this? > > > > One weird way would be smth like: > > static_assert(sizeof(Configuration) == ###, "Have you updated operator==() and > > this assert?"); > > > > Not sure this would be portable. > > 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). Yeah, that's very not portable. Even within the same platform, the size is different based on DCHECKs being enabled. https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/80001/components/subresource_... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:437: Configuration() /* default constructor */ On 2017/05/05 13:29:58, pkalinnikov wrote: > On 2017/05/05 12:25:42, engedy wrote: > > On 2017/05/04 12:04:23, pkalinnikov wrote: > > > nit: comma? > > > > I am not sure I understand. We don't have trailing commas anywhere in this > test? > > I suggested to put comma after /* default_constructor */, but I leave it up to > you. OK, for consistency, let's not have a trailing comma. https://codereview.chromium.org/2844063002/diff/120001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/120001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:85: // Do not forget updatin operator==, operator<<, and any other necessary On 2017/05/05 13:29:59, pkalinnikov wrote: > nit: updating Done.
*its*
Also, this is getting too big already, let's do add the TRACE's in a follow-up CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This generally looks good, I think the new logic is much easier to comprehend https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:117: [url, this](const Configuration& config) { Just making sure, does this properly omit the copy of |url| because it is already a reference? https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:156: DCHECK(url.SchemeIsHTTPOrHTTPS() || I think this DCHECK is redundant now with the explicit check above. Let's remove it and the comment. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:67: // Did not activate because altough there was a configuration whose nit: although https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:45: // filtering should be activated. The comment could be improved. Maybe "the subset of page loads where this subresource filtering Configuration is enabled"? A high priority configuration with ActivationScope::NO_SITES basically does nothing, which might be confusing if people expect it to block the feature entirely. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:511: EnabledConfigurations_FeatureEnabledWithNoParameters) { Same as the previous test? Or maybe my eyes are just glazing over? https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:555: std::vector<Configuration> reverse_order( Optional: I think a test which does 2 1 3 or 3 1 2 is more general than just reversing the priorities. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:591: Configuration())); Where does this one come from?
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
PTAL. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:117: [url, this](const Configuration& config) { On 2017/05/05 21:16:37, Charlie Harrison wrote: > Just making sure, does this properly omit the copy of |url| because it is > already a reference? I think so, but let's make that explicit. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:156: DCHECK(url.SchemeIsHTTPOrHTTPS() || On 2017/05/05 21:16:37, Charlie Harrison wrote: > I think this DCHECK is redundant now with the explicit check above. Let's remove > it and the comment. Gladly. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:67: // Did not activate because altough there was a configuration whose On 2017/05/05 21:16:37, Charlie Harrison wrote: > nit: although Done. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:45: // filtering should be activated. On 2017/05/05 21:16:37, Charlie Harrison wrote: > The comment could be improved. Maybe "the subset of page loads where this > subresource filtering Configuration is enabled"? > > A high priority configuration with ActivationScope::NO_SITES basically does > nothing, which might be confusing if people expect it to block the feature > entirely. How about this (and below)? https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:511: EnabledConfigurations_FeatureEnabledWithNoParameters) { On 2017/05/05 21:16:37, Charlie Harrison wrote: > Same as the previous test? Or maybe my eyes are just glazing over? No, it's my eyes. Fixed. :) https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:555: std::vector<Configuration> reverse_order( On 2017/05/05 21:16:37, Charlie Harrison wrote: > Optional: I think a test which does 2 1 3 or 3 1 2 is more general than just > reversing the priorities. Agreed. Done. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:591: Configuration())); On 2017/05/05 21:16:38, Charlie Harrison wrote: > Where does this one come from? This is the experimental configuration. Added comment. Could be convinced to remove this, it just makes for really ugly special casing in tests.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This generally LGTM but let's wait for Pavel to review too. Also, let's make sure we update the local / remote variation parameters to start passing in the priority explicitly (rather than implicitly setting to 0) https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:45: // filtering should be activated. On 2017/05/05 21:31:53, engedy wrote: > On 2017/05/05 21:16:37, Charlie Harrison wrote: > > The comment could be improved. Maybe "the subset of page loads where this > > subresource filtering Configuration is enabled"? > > > > A high priority configuration with ActivationScope::NO_SITES basically does > > nothing, which might be confusing if people expect it to block the feature > > entirely. > > How about this (and below)? Looks good! https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:572: const std::vector<Configuration> kPhisingAndDefaultConfigs = { kPhishing
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nits. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:44: return !!std::count_if(pieces_.begin(), pieces_.end(), predicate); nit: Could make it std::find(..) != pieces_.end() to avoid full scan on match. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:119: bool enabled_by_default; nit: This seems a no-op currently (can assume it false everywhere). Is it because new |enabled_by_default| presents are going to be added, or just the current defaults to be changed? If not, would it suffice for now to drop the DisablePresets parameter and |enabled_by_default|? https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:286: config.activation_conditions.priority = 50; Can we make this like 500 (and the above - 1000), to leave more free space for maneuvering with new priorities in between? https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:143: const std::string lexicographically_greatest_ruleset_flavor_; optional nitty nit: This one is a copy of one of the strings owned by |configs_by_decreasing_priority_|. Could be a base::StringPiece. But the current approach seems less error-prone.
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Patchset #15 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All done, please take a final look. https://codereview.chromium.org/2844063002/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:45: // filtering should be activated. On 2017/05/05 21:52:51, Charlie Harrison wrote: > On 2017/05/05 21:31:53, engedy wrote: > > On 2017/05/05 21:16:37, Charlie Harrison wrote: > > > The comment could be improved. Maybe "the subset of page loads where this > > > subresource filtering Configuration is enabled"? > > > > > > A high priority configuration with ActivationScope::NO_SITES basically does > > > nothing, which might be confusing if people expect it to block the feature > > > entirely. > > > > How about this (and below)? > > Looks good! Acknowledged. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:44: return !!std::count_if(pieces_.begin(), pieces_.end(), predicate); On 2017/05/08 11:48:43, pkalinnikov wrote: > nit: Could make it std::find(..) != pieces_.end() to avoid full scan on match. Done. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:119: bool enabled_by_default; On 2017/05/08 11:48:43, pkalinnikov wrote: > nit: This seems a no-op currently (can assume it false everywhere). Is it > because new |enabled_by_default| presents are going to be added, or just the > current defaults to be changed? > > If not, would it suffice for now to drop the DisablePresets parameter and > |enabled_by_default|? Yes, I am planning to make |kPresetLiveRunOnPhishingSites| enabled by default in a follow-up CL, and I want to make that CL as small as possible in case it causes problems and needs to be reverted. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:286: config.activation_conditions.priority = 50; On 2017/05/08 11:48:43, pkalinnikov wrote: > Can we make this like 500 (and the above - 1000), to leave more free space for > maneuvering with new priorities in between? Done. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:143: const std::string lexicographically_greatest_ruleset_flavor_; On 2017/05/08 11:48:43, pkalinnikov wrote: > optional nitty nit: This one is a copy of one of the strings owned by > |configs_by_decreasing_priority_|. Could be a base::StringPiece. > But the current approach seems less error-prone. Actually, from the perspective of the caller, it has the same level of error-proneness. Done. https://codereview.chromium.org/2844063002/diff/280001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/280001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:572: const std::vector<Configuration> kPhisingAndDefaultConfigs = { On 2017/05/05 21:52:51, Charlie Harrison wrote: > kPhishing Done.
engedy@chromium.org changed reviewers: + asvitkine@chromium.org, nparker@chromium.org, sorin@chromium.org
Dear owners, please review: @Sorin: c/b/component_updater/subresource_filter_component_installer* @Alexei: testing/variations/fieldtrial_testing_config.json @Nathan: c/b/safe_browsing/safe_browsing_service_browsertest.cc
Description was changed from ========== Add support for multiple simultaneous subresource_filter::Configurations. BUG=708181 ========== to ========== Add support for multiple simultaneous subresource_filter::Configurations. Also introduce the concept of presets, which will be used in a follow-up CL to enable the feature on phishing sites by default. BUG=708181 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Thank you! component updater lgtm https://codereview.chromium.org/2844063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:90: std::string ruleset_flavor) { can we pass by ref to const and avoid the move below?
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2844063002/diff/320001/chrome/browser/compone... File chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc (right): https://codereview.chromium.org/2844063002/diff/320001/chrome/browser/compone... chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc:90: std::string ruleset_flavor) { On 2017/05/09 01:49:08, Sorin Jianu wrote: > can we pass by ref to const and avoid the move below? Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@Nathan, friendly ping.
safe_browsing* LGTM
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, asvitkine@chromium.org, pkalinnikov@chromium.org, sorin@chromium.org Link to the patchset: https://codereview.chromium.org/2844063002/#ps360001 (title: "Rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1494421710205390,
"parent_rev": "19f212960eb19b495d895733c52d825b42ea3bd5", "commit_rev":
"610e540fa3179c3a5ed8eb91b7d48e935c67d86d"}
Message was sent while issue was closed.
Description was changed from ========== Add support for multiple simultaneous subresource_filter::Configurations. Also introduce the concept of presets, which will be used in a follow-up CL to enable the feature on phishing sites by default. BUG=708181 ========== to ========== Add support for multiple simultaneous subresource_filter::Configurations. Also introduce the concept of presets, which will be used in a follow-up CL to enable the feature on phishing sites by default. BUG=708181 Review-Url: https://codereview.chromium.org/2844063002 Cr-Commit-Position: refs/heads/master@{#470559} Committed: https://chromium.googlesource.com/chromium/src/+/610e540fa3179c3a5ed8eb91b7d4... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:360001) as https://chromium.googlesource.com/chromium/src/+/610e540fa3179c3a5ed8eb91b7d4...
Message was sent while issue was closed.
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2844063002/diff/360001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:107: if (!url.SchemeIsHTTPOrHTTPS()) { looking at this now, does this mean that if the system is disabled but the URL isn't a valid scheme, behavior change from ACTIVATION_DISABLED to UNSUPPORTED_SCHEME? from a metrics tracking standpoint i find that a little odd - we shouldn't even be evaluating / applying policy if the system is disabled. is this change in behavior important to other code? would it be possible for us to restore the original order?
Message was sent while issue was closed.
https://codereview.chromium.org/2844063002/diff/360001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2844063002/diff/360001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:107: if (!url.SchemeIsHTTPOrHTTPS()) { On 2017/05/17 13:55:48, Bryan McQuade wrote: > looking at this now, does this mean that if the system is disabled but the URL > isn't a valid scheme, behavior change from ACTIVATION_DISABLED to > UNSUPPORTED_SCHEME? from a metrics tracking standpoint i find that a little odd > - we shouldn't even be evaluating / applying policy if the system is disabled. > is this change in behavior important to other code? would it be possible for us > to restore the original order? Unfortunately, it would make the code here less clear. Do we really need this enum value? Can we just obsolete this and record ACTIVATION_CONDITIONS_NOT_MET in these cases? Otherwise, DoesMainFrameURLSatisfyActivationConditions would have to return 2 configs: a matching, and another one that is non-matching only because of the wrong scheme (which would cause UNSUPPORTED_SCHEME to be reported for ALL_SITES and ACTIVATION_LIST scopes, but not for NO_SITES).
Message was sent while issue was closed.
On 2017/05/17 at 14:21:41, engedy wrote: > https://codereview.chromium.org/2844063002/diff/360001/components/subresource... > File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): > > https://codereview.chromium.org/2844063002/diff/360001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:107: if (!url.SchemeIsHTTPOrHTTPS()) { > On 2017/05/17 13:55:48, Bryan McQuade wrote: > > looking at this now, does this mean that if the system is disabled but the URL > > isn't a valid scheme, behavior change from ACTIVATION_DISABLED to > > UNSUPPORTED_SCHEME? from a metrics tracking standpoint i find that a little odd > > - we shouldn't even be evaluating / applying policy if the system is disabled. > > is this change in behavior important to other code? would it be possible for us > > to restore the original order? > > Unfortunately, it would make the code here less clear. Do we really need this enum value? Can we just obsolete this and record ACTIVATION_CONDITIONS_NOT_MET in these cases? > > Otherwise, DoesMainFrameURLSatisfyActivationConditions would have to return 2 configs: a matching, and another one that is non-matching only because of the wrong scheme (which would cause UNSUPPORTED_SCHEME to be reported for ALL_SITES and ACTIVATION_LIST scopes, but not for NO_SITES). We want to know how often urls are whitelisted, since this helps us to understand the effect of page reloads in the experiment. Charles and I discussed reporting disabled in the API that the page load metrics code calls, in cases where we are disabled. Can you all cc me on any changes that might result in behavior changes to metrics like this? It's important that I stay on top of these changes so we're not surprised by data coming in from the field.
Message was sent while issue was closed.
On 2017/05/17 14:21:41, engedy wrote: > https://codereview.chromium.org/2844063002/diff/360001/components/subresource... > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > (right): > > https://codereview.chromium.org/2844063002/diff/360001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:107: > if (!url.SchemeIsHTTPOrHTTPS()) { > On 2017/05/17 13:55:48, Bryan McQuade wrote: > > looking at this now, does this mean that if the system is disabled but the URL > > isn't a valid scheme, behavior change from ACTIVATION_DISABLED to > > UNSUPPORTED_SCHEME? from a metrics tracking standpoint i find that a little > odd > > - we shouldn't even be evaluating / applying policy if the system is disabled. > > is this change in behavior important to other code? would it be possible for > us > > to restore the original order? > > Unfortunately, it would make the code here less clear. Do we really need this > enum value? Can we just obsolete this and record ACTIVATION_CONDITIONS_NOT_MET > in these cases? > > Otherwise, DoesMainFrameURLSatisfyActivationConditions would have to return 2 > configs: a matching, and another one that is non-matching only because of the > wrong scheme (which would cause UNSUPPORTED_SCHEME to be reported for ALL_SITES > and ACTIVATION_LIST scopes, but not for NO_SITES). engedy: What about just returning this activation decision if at least one configuration matched?
Message was sent while issue was closed.
> We want to know how often urls are whitelisted, since this helps us to > understand the effect of page reloads in the experiment. I'm not sure I understand. We were talking about unsupported schemes, not whitelisting? > Charles and I discussed reporting disabled in the API that the page load metrics > code calls, in cases where we are disabled. > > Can you all cc me on any changes that might result in behavior changes to > metrics like this? It's important that I stay on top of these changes so we're > not surprised by data coming in from the field. I actually wanted to add you as reviewer on this, but then forgot. Sorry about that.
Message was sent while issue was closed.
> engedy: What about just returning this activation decision if at least one > configuration matched? That's somewhat unfortunate, as activation-list-scoped configs will never match (once we add the extra check), so if you navigate to a site with non-HTTP(s) scheme, we a different enum would be recorded based on whether the non-matching config was ALL_SITES or ACTIVATION_LIST. But if we can live with that, I'm fine with it. |
