Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2186233003:
Introduce activation scope logic. (Closed)
DescriptionIntroduce activation scope logic.
For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site.
BUG=609747
Committed: https://crrev.com/05b1895adf4d64a7ae96642379fe83f008b09cfd
Cr-Commit-Position: refs/heads/master@{#410014}
Patch Set 1 #Patch Set 2 : split #Patch Set 3 : cleanup #Patch Set 4 : forgotten factory code #
Total comments: 3
Patch Set 5 : one feature #Patch Set 6 : one feature #
Total comments: 5
Patch Set 7 : fix tests #
Total comments: 22
Patch Set 8 : adressed engedy@ comments #
Total comments: 7
Patch Set 9 : adressed engedy@ comments #Patch Set 10 : adressed-comments #Messages
Total messages: 102 (86 generated)
The CQ bit was checked by melandory@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...
Description was changed from ========== Remove old activation logic. BUG=609747 ========== to ========== Remove deprecated activation logic. BUG=609747 ==========
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_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Remove deprecated activation logic. BUG=609747 ========== to ========== Introduce activation scope logic. BUG=609747 ==========
Balazs, can you please look at changes in files and comment on approach not code itself, if approach looks good, I'll refine the code and tests. Thank you! components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc components/subresource_filter/core/browser/subresource_filter_features.h components/subresource_filter/core/browser/subresource_filter_features_test_support.h components/subresource_filter/core/common/activation_scope.h components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc components/subresource_filter/content/browser/subresource_filter_navigation_throttle.cc components/subresource_filter/core/browser/subresource_filter_features.h components/subresource_filter/core/browser/subresource_filter_features_test_support.h components/subresource_filter/core/common/activation_scope.h
The CQ bit was checked by melandory@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@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_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 melandory@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Introduce activation scope logic. BUG=609747 ========== to ========== Introduce activation scope logic. For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site. BUG=609747 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was checked by melandory@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...
Patchset #1 (id:100001) has been deleted
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/03 13:20:39, melandory wrote: > PTAL, thanks! I'll try to split this CL onto 2: some refactoring and then "scope" logic. So wait a bit with reviewing.
The CQ bit was checked by melandory@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...
Patchset #2 (id:140001) has been deleted
The CQ bit was checked by melandory@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 melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@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...
engedy@chromium.org changed reviewers: + engedy@chromium.org
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_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Some initial comments. https://codereview.chromium.org/2186233003/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2186233003/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:16: const base::Feature kSafeBrowsingSubresourceFilterScope{ How about just reusing the feature above? I think we could just have the ActivationScope as a variation parameter, like we do with the ActivationState. https://codereview.chromium.org/2186233003/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_test_support.h (right): https://codereview.chromium.org/2186233003/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_test_support.h:39: base::FeatureList::OverrideState scope_feature_state, If we don't have the second Feature, we could just add the |activation_scope| to the class above, and simplify things.
The CQ bit was checked by melandory@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_chromeos_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 melandory@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Patchset #5 (id:220001) 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...
Patchset #5 (id:240001) has been deleted
The CQ bit was checked by melandory@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...
Patchset #6 (id:280001) has been deleted
ptal https://codereview.chromium.org/2186233003/diff/200001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2186233003/diff/200001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:16: const base::Feature kSafeBrowsingSubresourceFilterScope{ On 2016/08/04 08:37:49, engedy wrote: > How about just reusing the feature above? I think we could just have the > ActivationScope as a variation parameter, like we do with the ActivationState. Done.
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by melandory@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_asan_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 melandory@chromium.org to run a CQ dry run
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Patchset #6 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 melandory@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 % comments. Thanks! https://codereview.chromium.org/2186233003/diff/320001/components/subresource... File components/subresource_filter/core/common/activation_scope.h (right): https://codereview.chromium.org/2186233003/diff/320001/components/subresource... components/subresource_filter/core/common/activation_scope.h:14: DISABLED, `Disabled` sounds strange for a `scope`. What do you think about NOWHERE or NO_SITES? https://codereview.chromium.org/2186233003/diff/320001/components/subresource... components/subresource_filter/core/common/activation_scope.h:15: ALL_SITES, nit: Could you please document what ALL_SITES and ACTIVATION_LIST means? For ALL_SITES, also mention that it is used for testing. https://codereview.chromium.org/2186233003/diff/320001/components/subresource... components/subresource_filter/core/common/activation_scope.h:15: ALL_SITES, Let's flip line 15 and 16 so the enumerators are in order of increasing extent. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:115: base::FeatureList::OVERRIDE_DISABLE_FEATURE, kActivationStateDisabled, nit: I'd specify |kActivationStateEnabled|, so the test enforces that the production code makes the decision based on the activation scope. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:118: factory()->ActivateForFrameHostIfNeeded(main_rfh(), GURL("https://test.com")); Hang on, this looks like code from another CL? https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:134: base::FeatureList::OVERRIDE_DISABLE_FEATURE, kActivationStateDisabled, nit: same here. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:175: Could you please add a unittest that exercises kActivationScopeAllSites? https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:17: extern const base::Feature kSafeBrowsingSubresourceFilterScope; No longer used. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:35: // Returns current activation scope, so the appropriate activation signal can be Phrasing nit: Returns the current activation scope, that is, the subset of page loads where subresource filtering should be activated. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:36: // send to the RenderFrame. The function returns ActivationScope::DESABLED typo: DISABLED https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:38: // activation state. nit: a wider activation scope. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:78: kActivationScopeDisabled, test_case.activation_scope_param); nit: kActivationStateDisabled https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:85: TEST(SubresourceFilterFeaturesTest, ActivationAStateAndScope) { typo: extra 'A' https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:93: {false, kActivationStateDisabled, ActivationState::DISABLED, In addition to this test case, let's have one at the very end with: false, kActivationStateEnable, kActivationScopeAllSites This verifies that the master toggle rules them all.
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 melandory@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/2186233003/diff/320001/components/subresource... File components/subresource_filter/core/common/activation_scope.h (right): https://codereview.chromium.org/2186233003/diff/320001/components/subresource... components/subresource_filter/core/common/activation_scope.h:15: ALL_SITES, On 2016/08/04 13:26:55, engedy wrote: > Let's flip line 15 and 16 so the enumerators are in order of increasing extent. Done. https://codereview.chromium.org/2186233003/diff/320001/components/subresource... components/subresource_filter/core/common/activation_scope.h:15: ALL_SITES, On 2016/08/04 13:26:54, engedy wrote: > nit: Could you please document what ALL_SITES and ACTIVATION_LIST means? For > ALL_SITES, also mention that it is used for testing. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:115: base::FeatureList::OVERRIDE_DISABLE_FEATURE, kActivationStateDisabled, On 2016/08/04 13:26:55, engedy wrote: > nit: I'd specify |kActivationStateEnabled|, so the test enforces that the > production code makes the decision based on the activation scope. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:118: factory()->ActivateForFrameHostIfNeeded(main_rfh(), GURL("https://test.com")); On 2016/08/04 13:26:55, engedy wrote: > Hang on, this looks like code from another CL? Not really, it seems that just diff looks too confusing. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:134: base::FeatureList::OVERRIDE_DISABLE_FEATURE, kActivationStateDisabled, On 2016/08/04 13:26:55, engedy wrote: > nit: same here. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:17: extern const base::Feature kSafeBrowsingSubresourceFilterScope; On 2016/08/04 13:26:55, engedy wrote: > No longer used. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:35: // Returns current activation scope, so the appropriate activation signal can be On 2016/08/04 13:26:55, engedy wrote: > Phrasing nit: Returns the current activation scope, that is, the subset of page > loads where subresource filtering should be activated. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:36: // send to the RenderFrame. The function returns ActivationScope::DESABLED On 2016/08/04 13:26:55, engedy wrote: > typo: DISABLED Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:38: // activation state. On 2016/08/04 13:26:55, engedy wrote: > nit: a wider activation scope. Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:78: kActivationScopeDisabled, test_case.activation_scope_param); On 2016/08/04 13:26:55, engedy wrote: > nit: kActivationStateDisabled Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:85: TEST(SubresourceFilterFeaturesTest, ActivationAStateAndScope) { On 2016/08/04 13:26:55, engedy wrote: > typo: extra 'A' Done. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:93: {false, kActivationStateDisabled, ActivationState::DISABLED, On 2016/08/04 13:26:55, engedy wrote: > In addition to this test case, let's have one at the very end with: > > false, kActivationStateEnable, kActivationScopeAllSites > > This verifies that the master toggle rules them all. Done.
melandory@chromium.org changed reviewers: + waffles@chromium.org
waffles@chromium.org: Please review changes in chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
waffles@chromium.org: Please review changes in chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
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_...)
The CQ bit was checked by melandory@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...
Still LGTM % nits. https://codereview.chromium.org/2186233003/diff/340001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/340001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:175: On 2016/08/04 13:26:55, engedy wrote: > Could you please add a unittest that exercises kActivationScopeAllSites? Don't forget this either. https://codereview.chromium.org/2186233003/diff/360001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (right): https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:35: // ActivationScope::DISABLED unless the feature is enabled and variation nit: NO_SITES https://codereview.chromium.org/2186233003/diff/360001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:57: {false, "disabled", ActivationScope::NO_SITES}, nit: no_sites https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:62: {true, "disable", ActivationScope::NO_SITES}, nit x 3: nosites, no_sites, No_Sites, respectively. https://codereview.chromium.org/2186233003/diff/360001/components/subresource... File components/subresource_filter/core/common/activation_scope.cc (right): https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/common/activation_scope.cc:16: os << "DISABLED"; nit: NO_SITES
The CQ bit was checked by melandory@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/2186233003/diff/360001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features_unittest.cc (right): https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:57: {false, "disabled", ActivationScope::NO_SITES}, On 2016/08/04 14:26:26, engedy wrote: > nit: no_sites Done. https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features_unittest.cc:62: {true, "disable", ActivationScope::NO_SITES}, On 2016/08/04 14:26:26, engedy wrote: > nit x 3: nosites, no_sites, No_Sites, respectively. Done. https://codereview.chromium.org/2186233003/diff/360001/components/subresource... File components/subresource_filter/core/common/activation_scope.cc (right): https://codereview.chromium.org/2186233003/diff/360001/components/subresource... components/subresource_filter/core/common/activation_scope.cc:16: os << "DISABLED"; On 2016/08/04 14:26:26, engedy wrote: > nit: NO_SITES Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc lgtm
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2186233003/#ps170013 (title: "adressed-comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Introduce activation scope logic. For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site. BUG=609747 ========== to ========== Introduce activation scope logic. For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site. BUG=609747 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170013)
Message was sent while issue was closed.
Description was changed from ========== Introduce activation scope logic. For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site. BUG=609747 ========== to ========== Introduce activation scope logic. For the testing purposes we need to distinguish several cases, when Safe Browsing Subresource Filter logic is triggered: either we perform check against the Safe Browsing list or we trigger the logic on every site. BUG=609747 Committed: https://crrev.com/05b1895adf4d64a7ae96642379fe83f008b09cfd Cr-Commit-Position: refs/heads/master@{#410014} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/05b1895adf4d64a7ae96642379fe83f008b09cfd Cr-Commit-Position: refs/heads/master@{#410014} |
