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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by melandory
Modified:
1 year 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 41 (30 generated)
melandory
PTAL. There is literally 0 tests for now, but it is what I have.
1 year 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 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 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 ago (2016-08-25 23:38:27 UTC) #15
Lei Zhang
lgtm
1 year 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 ago (2016-08-26 08:47:05 UTC) #34
commit-bot: I haz the power
Committed patchset #7 (id:120001)
1 year 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 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 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 ago (2016-08-26 17:37:18 UTC) #40
melandory
1 year 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b