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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by melandory
Modified:
4 months 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 Review-Url: https://codereview.chromium.org/2814733002 Cr-Commit-Position: refs/heads/master@{#472099} Committed: https://chromium.googlesource.com/chromium/src/+/cc872706b26db043caa7d57f4c10661ff9d0ae40

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

Total comments: 29

Patch Set 2 : . #

Total comments: 44

Patch Set 3 : some engedy@ comments #

Total comments: 3

Patch Set 4 : rebased and changes sync strategy #

Total comments: 2

Patch Set 5 : . #

Patch Set 6 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+425 lines, -439 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 6 chunks +0 lines, -145 lines 0 comments Download
M chrome/browser/subresource_filter/subresource_filter_browsertest.cc View 1 2 3 4 4 chunks +26 lines, -29 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 4 2 chunks +0 lines, -32 lines 0 comments Download
M components/safe_browsing_db/v4_database.h View 1 2 3 4 2 chunks +9 lines, -1 line 0 comments Download
M components/safe_browsing_db/v4_database.cc View 1 2 3 4 3 chunks +28 lines, -8 lines 0 comments Download
M components/safe_browsing_db/v4_database_unittest.cc View 1 2 3 4 1 chunk +12 lines, -3 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.h View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.cc View 1 2 3 4 7 chunks +16 lines, -9 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager_unittest.cc View 1 2 3 4 5 1 chunk +7 lines, -1 line 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 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 27 chunks +116 lines, -95 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h View 1 2 3 4 2 chunks +7 lines, -2 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc View 1 2 3 4 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 15 chunks +152 lines, -59 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 3 chunks +20 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 166 (132 generated)
melandory
PTAL, thanks!
5 months, 1 week ago (2017-04-12 14:27:31 UTC) #15
engedy
Are you sure you uploaded all files, e.g., the SubresourceFilterSafeBrowsingActivationThrottle? :-)
5 months, 1 week 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 ...
5 months 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): ...
5 months 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 ...
5 months ago (2017-04-20 00:28:54 UTC) #34
Nathan Parker
Oops, I shouldn't reply from Google email.. Trying again: Those fetch_stragegies are there because non-Chrome-branded ...
5 months 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) { ...
5 months 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 ...
5 months ago (2017-04-20 12:59:31 UTC) #37
Nathan Parker
Ok, I filed http://crbug.com/713902 to investigate what I think you're describing. As for this change, ...
5 months 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 ...
5 months ago (2017-04-21 12:02:37 UTC) #41
Nathan Parker
Yes that was the intention, since the code that follows that checks the DB will ...
5 months 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 months, 3 weeks ago (2017-04-24 14:51:58 UTC) #54
Nathan Parker
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 months, 3 weeks ago (2017-04-24 22:17:50 UTC) #55
Nathan Parker
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 months, 3 weeks 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: > ...
4 months, 3 weeks ago (2017-04-25 13:48:14 UTC) #63
melandory
jwd, PTAL at tools/metrics/histograms/histograms.xml
4 months, 3 weeks ago (2017-04-25 13:48:56 UTC) #65
melandory
jwd, PTAL at tools/metrics/histograms/histograms.xml
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 3 weeks ago (2017-04-26 15:02:21 UTC) #79
melandory
jwd@ friendlt ping nparker@ please take another look
4 months, 3 weeks ago (2017-04-26 15:04:31 UTC) #80
engedy
Did you upload the latest patch set?
4 months, 3 weeks 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 ...
4 months, 3 weeks ago (2017-04-26 23:34:10 UTC) #82
Charlie Harrison
chrome/browser/loader LGTM but I didn't take a close look at the other files.
4 months, 3 weeks 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 ...
4 months, 3 weeks 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 ...
4 months, 3 weeks ago (2017-04-27 17:10:13 UTC) #97
jwd
lgtm
4 months, 3 weeks ago (2017-04-27 17:51:06 UTC) #98
Nathan Parker
LGTM for safe_browsing*/* https://codereview.chromium.org/2814733002/diff/310001/components/safe_browsing_db/v4_local_database_manager.cc File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2814733002/diff/310001/components/safe_browsing_db/v4_local_database_manager.cc#newcode77 components/safe_browsing_db/v4_local_database_manager.cc:77: ListInfo(kSyncAlways, "UrlSubresourceFilter.store", l-g-t-m assuming you've tested ...
4 months, 2 weeks ago (2017-05-01 22:08:44 UTC) #99
engedy
https://codereview.chromium.org/2814733002/diff/210001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2814733002/diff/210001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc#newcode153 components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:153: subresource_filter::kActivationListPhishingInterstitial, On 2017/04/26 15:02:21, melandory wrote: > On 2017/04/26 ...
4 months, 2 weeks ago (2017-05-05 08:19:33 UTC) #108
melandory
nparker@, please take another look. As per e-mail discussion it was decided that we can't ...
4 months, 1 week ago (2017-05-11 15:05:13 UTC) #132
Nathan Parker
https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsing_db/v4_database.cc#newcode207 components/safe_browsing_db/v4_database.cc:207: bool V4Database::AreStoresAvailable( I had the following comment on an ...
4 months, 1 week ago (2017-05-12 17:30:10 UTC) #135
melandory
https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsing_db/v4_database.cc File components/safe_browsing_db/v4_database.cc (right): https://codereview.chromium.org/2814733002/diff/430001/components/safe_browsing_db/v4_database.cc#newcode207 components/safe_browsing_db/v4_database.cc:207: bool V4Database::AreStoresAvailable( On 2017/05/12 17:30:10, Nathan Parker wrote: > ...
4 months ago (2017-05-15 14:57:25 UTC) #147
Nathan Parker
Great, thanks. LGTM.
4 months ago (2017-05-15 16:28:46 UTC) #151
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/2814733002/470002
4 months ago (2017-05-16 14:29:47 UTC) #163
commit-bot: I haz the power
4 months ago (2017-05-16 14:35:03 UTC) #166
Message was sent while issue was closed.
Committed patchset #6 (id:470002) as
https://chromium.googlesource.com/chromium/src/+/cc872706b26db043caa7d57f4c10...
Sign in to reply to this message.

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