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

Issue 2814733002: Add the SocEng as a type for checking in CheckUrlForSubresourceFilter.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 3 days ago by melandory
Modified:
1 day, 13 hours ago
CC:
chromium-reviews, subresource-filter-reviews_chromium.org, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, jam, Randy Smith (Not in Mondays), timvolodine, darin-cc_chromium.org, asvitkine+watch_chromium.org, blundell+watchlist_chromium.org, loading-reviews_chromium.org, mmenke
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add the SocEng as a type for checking in CheckUrlForSubresourceFilter. The main change in this CL -- CheckUrlForSubresourceFilter will check not only against GetUrlSubresourceFilterId(), but also against GetUrlSocEngId(). This CL also removes presence of subresource filter code from the SafeBrowsingResourceThrottle. BUG=671962

Patch Set 1 : remove tests which are not neede anymore #

Total comments: 29

Patch Set 2 : . #

Patch Set 3 : some engedy@ comments #

Total comments: 3

Patch Set 4 : . #

Patch Set 5 : fix windows #

Patch Set 6 : . #

Total comments: 43

Patch Set 7 : some engedy@ comments #

Total comments: 3

Patch Set 8 : engedy@ comments #

Patch Set 9 : . #

Patch Set 10 : . #

Patch Set 11 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -409 lines) Patch
M chrome/browser/loader/safe_browsing_resource_throttle.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc View 1 2 3 4 5 6 6 chunks +0 lines, -148 lines 0 comments Download
M chrome/browser/subresource_filter/subresource_filter_browsertest.cc View 1 2 3 4 5 6 7 8 9 chunks +10 lines, -30 lines 0 comments Download
M components/safe_browsing/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M components/safe_browsing/DEPS View 1 chunk +0 lines, -1 line 0 comments Download
M components/safe_browsing/base_resource_throttle.cc View 2 chunks +0 lines, -32 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.cc View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc View 1 2 3 4 5 6 7 3 chunks +20 lines, -43 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 3 4 5 6 7 9 27 chunks +111 lines, -91 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -4 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc View 1 2 3 4 5 6 7 9 chunks +99 lines, -50 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 3 chunks +20 lines, -2 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 98 (72 generated)
melandory
PTAL, thanks!
2 weeks, 2 days ago (2017-04-12 14:27:31 UTC) #15
engedy
Are you sure you uploaded all files, e.g., the SubresourceFilterSafeBrowsingActivationThrottle? :-)
2 weeks, 2 days ago (2017-04-12 18:53:55 UTC) #18
melandory
On 2017/04/12 18:53:55, engedy wrote: > Are you sure you uploaded all files, e.g., the ...
1 week, 3 days ago (2017-04-18 13:57:16 UTC) #27
melandory
nparker@chromium.org I have a question to you, it's in components/safe_browsing_db/v4_local_database_manager.cc Thanks! https://codereview.chromium.org/2814733002/diff/90001/components/safe_browsing_db/v4_local_database_manager.cc File components/safe_browsing_db/v4_local_database_manager.cc (right): ...
1 week, 2 days ago (2017-04-19 14:47:11 UTC) #31
chromium-reviews
Those fetch_stragegies are there because non-Chrome-branded builds aren't authorized to fetch certain lists. Safe Browsing ...
1 week, 2 days ago (2017-04-20 00:28:54 UTC) #34
Nathan Parker (ooo till May 1)
Oops, I shouldn't reply from Google email.. Trying again: Those fetch_stragegies are there because non-Chrome-branded ...
1 week, 2 days ago (2017-04-20 00:30:18 UTC) #35
engedy
Looking good, a couple of comments. https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#oldcode907 chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { ...
1 week, 1 day ago (2017-04-20 11:16:11 UTC) #36
melandory
On 2017/04/20 00:30:18, Nathan Parker wrote: > Oops, I shouldn't reply from Google email.. Trying ...
1 week, 1 day ago (2017-04-20 12:59:31 UTC) #37
Nathan Parker (ooo till May 1)
Ok, I filed http://crbug.com/713902 to investigate what I think you're describing. As for this change, ...
1 week, 1 day ago (2017-04-20 22:30:18 UTC) #38
melandory
On 2017/04/20 22:30:18, Nathan Parker wrote: > Ok, I filed http://crbug.com/713902 to investigate what I ...
1 week ago (2017-04-21 12:02:37 UTC) #41
Nathan Parker (ooo till May 1)
Yes that was the intention, since the code that follows that checks the DB will ...
1 week ago (2017-04-21 17:33:06 UTC) #52
melandory
On 2017/04/21 17:33:06, Nathan Parker wrote: > Yes that was the intention, since the code ...
4 days, 16 hours ago (2017-04-24 14:51:58 UTC) #54
Nathan Parker (ooo till May 1)
https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsing_db/v4_database.cc#newcode237 components/safe_browsing_db/v4_database.cc:237: if (!IsStoreAvailable(identifier)) I thought you were going to sync ...
4 days, 9 hours ago (2017-04-24 22:17:50 UTC) #55
Nathan Parker (ooo till May 1)
https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/150001/components/safe_browsing_db/v4_database.cc#newcode224 components/safe_browsing_db/v4_database.cc:224: if (IsStoreAvailable(identifier)) (codereview ate my comment) I think we'd ...
4 days, 9 hours ago (2017-04-24 22:20:18 UTC) #56
melandory
https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#oldcode907 chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { On 2017/04/20 11:16:10, engedy wrote: > ...
3 days, 17 hours ago (2017-04-25 13:48:14 UTC) #63
melandory
jwd, PTAL at tools/metrics/histograms/histograms.xml
3 days, 17 hours ago (2017-04-25 13:48:56 UTC) #65
melandory
jwd, PTAL at tools/metrics/histograms/histograms.xml
3 days, 17 hours ago (2017-04-25 13:48:59 UTC) #66
engedy
LGTM % comments, thanks! https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (left): https://codereview.chromium.org/2814733002/diff/90001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#oldcode907 chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:907: IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, SubresourceFilterEndToEndTest) { On 2017/04/25 ...
2 days, 17 hours ago (2017-04-26 13:47:10 UTC) #77
melandory
csharrison@: Please review changes in chrome/browser/loader/safe_browsing_resource_throttle.cc https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc#newcode43 chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:43: #include "chrome/browser/subresource_filter/test_ruleset_publisher.h" On ...
2 days, 16 hours ago (2017-04-26 15:02:21 UTC) #79
melandory
jwd@ friendlt ping nparker@ please take another look
2 days, 16 hours ago (2017-04-26 15:04:31 UTC) #80
engedy
Did you upload the latest patch set?
2 days, 15 hours ago (2017-04-26 15:25:59 UTC) #81
jwd
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml#oldcode127174 tools/metrics/histograms/histograms.xml:127174: - <suffix name="SubresourceFilterOnly" label="subresource filter only patern"/> Rather than ...
2 days, 7 hours ago (2017-04-26 23:34:10 UTC) #82
Charlie (ooo-ish until may 2)
chrome/browser/loader LGTM but I didn't take a close look at the other files.
1 day, 18 hours ago (2017-04-27 12:42:21 UTC) #89
melandory
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml#oldcode127174 tools/metrics/histograms/histograms.xml:127174: - <suffix name="SubresourceFilterOnly" label="subresource filter only patern"/> On 2017/04/26 ...
1 day, 14 hours ago (2017-04-27 17:10:12 UTC) #96
melandory
https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2814733002/diff/230001/tools/metrics/histograms/histograms.xml#newcode70930 tools/metrics/histograms/histograms.xml:70930: + Obsolete, since the don't have correct data to ...
1 day, 14 hours ago (2017-04-27 17:10:13 UTC) #97
jwd
1 day, 13 hours ago (2017-04-27 17:51:06 UTC) #98
lgtm
Sign in to reply to this message.

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