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

Issue 2272323002: Allow Safe Browsing Saresource Filter to distinguish between different lists. (Closed)

Created:
1 year, 2 months ago by melandory
Modified:
1 year, 2 months ago
Reviewers:
Lei Zhang, engedy
CC:
chromium-reviews, jam, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Allow Safe Browsing Saresource Filter to distinguish between different lists. Safe browsing subresource filter should be able to make activation descision based on the type of the list and metadata for the list. BUG=609747, 641037 Committed: https://crrev.com/3851251cc17db046f32af57e003a1e86fd375db3 Cr-Commit-Position: refs/heads/master@{#414674}

Patch Set 1 #

Patch Set 2 : Allow Safe Browsing Saresource Filter to distinguish between different lists. #

Total comments: 10

Patch Set 3 : basic unittest for getting activation list variation #

Patch Set 4 : more tests #

Patch Set 5 : fix some tests #

Patch Set 6 : more tests #

Patch Set 7 : fix safe_browsing_service_browsertest #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -53 lines) Patch
M chrome/browser/renderer_host/safe_browsing_resource_throttle.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h View 3 chunks +3 lines, -5 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc View 1 2 3 2 chunks +14 lines, -3 lines 1 comment Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 3 4 5 12 chunks +118 lines, -33 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_navigation_throttle_unittests.cc View 1 2 3 4 8 chunks +19 lines, -10 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features.h View 1 2 3 3 chunks +10 lines, -0 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features.cc View 1 2 3 3 chunks +25 lines, -0 lines 1 comment Download
M components/subresource_filter/core/browser/subresource_filter_features_test_support.h View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features_test_support.cc View 1 2 1 chunk +12 lines, -1 line 0 comments Download
M components/subresource_filter/core/browser/subresource_filter_features_unittest.cc View 1 2 3 1 chunk +52 lines, -0 lines 0 comments Download
A components/subresource_filter/core/common/activation_list.h View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 41 (30 generated)
melandory
PTAL. There is literally 0 tests for now, but it is what I have.
1 year, 2 months ago (2016-08-25 05:59:21 UTC) #6
melandory
On 2016/08/25 05:59:21, melandory wrote: > PTAL. There is literally 0 tests for now, but ...
1 year, 2 months ago (2016-08-25 17:33:51 UTC) #9
engedy
LGTM % comments, thanks for working on this! https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode92 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92: bool ...
1 year, 2 months ago (2016-08-25 21:32:55 UTC) #11
melandory
thestig@chromium.org: Please review changes in chrome/browser/renderer_host/safe_browsing_resource_throttle.cc https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode92 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92: bool proceed = ...
1 year, 2 months ago (2016-08-25 23:38:27 UTC) #15
Lei Zhang
lgtm
1 year, 2 months ago (2016-08-26 00:46:26 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2272323002/120001
1 year, 2 months ago (2016-08-26 08:47:05 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:120001)
1 year, 2 months ago (2016-08-26 08:51:35 UTC) #36
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/3851251cc17db046f32af57e003a1e86fd375db3 Cr-Commit-Position: refs/heads/master@{#414674}
1 year, 2 months ago (2016-08-26 08:53:51 UTC) #38
melandory
https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/core/browser/subresource_filter_features.cc File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2272323002/diff/20001/components/subresource_filter/core/browser/subresource_filter_features.cc#newcode58 components/subresource_filter/core/browser/subresource_filter_features.cc:58: base::SplitString(activation_lists, ",", base::TRIM_WHITESPACE, On 2016/08/25 21:32:54, engedy wrote: > ...
1 year, 2 months ago (2016-08-26 15:06:10 UTC) #39
engedy
Thank you for the rigorous tests! Two important things below. https://codereview.chromium.org/2272323002/diff/120001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2272323002/diff/120001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode100 ...
1 year, 2 months ago (2016-08-26 17:37:18 UTC) #40
melandory
1 year, 2 months ago (2016-08-26 18:20:52 UTC) #41
Message was sent while issue was closed.
On 2016/08/26 17:37:18, engedy wrote:
> Thank you for the rigorous tests!  Two important things below.
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> File
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:100:
> (threat_type == safe_browsing::SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL);
> Could you please double check with Noe if this should be
> SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL or SB_THREAT_TYPE_URL_PHISHING or
both?
Yeah, I decided to do my best guess and wait util morning to fix or confirm. So
fix will arrive shortly.
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> File components/subresource_filter/core/browser/subresource_filter_features.cc
> (right):
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> components/subresource_filter/core/browser/subresource_filter_features.cc:30:
> "social_eng_ads_intertitial";
> Watch out, typo still hiding in this literal.  We should probably change `eng`
> to `engineering` too.
Yeah! I tried to find it in the evening, but no luck.

Powered by Google App Engine
This is Rietveld efc10ee0f