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:
1 week, 6 days ago by melandory
Modified:
2 hours, 17 minutes ago
Reviewers:
Nathan Parker, engedy
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: 15

Patch Set 2 : . #

Patch Set 3 : some engedy@ comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -363 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 chunks +0 lines, -138 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/test_database_manager.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M components/safe_browsing_db/v4_database.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M components/safe_browsing_db/v4_database.cc View 1 2 chunks +18 lines, -8 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.cc View 1 2 chunks +3 lines, -1 line 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h View 1 2 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 chunks +14 lines, -42 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 18 chunks +67 lines, -74 lines 0 comments Download
M components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc View 1 2 9 chunks +78 lines, -51 lines 0 comments Download
M components/subresource_filter/content/renderer/subresource_filter_agent.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M components/subresource_filter/content/renderer/subresource_filter_agent.cc View 1 2 4 chunks +4 lines, -4 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 4 chunks +16 lines, -6 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 54 (42 generated)
melandory
PTAL, thanks!
1 week, 5 days ago (2017-04-12 14:27:31 UTC) #15
engedy
Are you sure you uploaded all files, e.g., the SubresourceFilterSafeBrowsingActivationThrottle? :-)
1 week, 4 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 ...
6 days, 3 hours 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 days, 2 hours 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 ...
4 days, 16 hours 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 ...
4 days, 16 hours 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) { ...
4 days, 5 hours 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 ...
4 days, 4 hours 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, ...
3 days, 18 hours 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 ...
3 days, 5 hours 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 ...
2 days, 23 hours ago (2017-04-21 17:33:06 UTC) #52
melandory
2 hours, 17 minutes ago (2017-04-24 14:51:58 UTC) #54
On 2017/04/21 17:33:06, Nathan Parker wrote:
> Yes that was the intention, since the code that follows that checks the DB
will
> DCHECK if you ask it for a store that's not available. I think it's important
> that the default action what to do when store(s) are unavailable should be
> handled up top in the Check*() calls, since that's where it's most clear what
> the caller expects.
> 
> So I'd go for #1 if that's possible (not sure if backend will reject it), or
> else explicitly check the availability of each list separately and craft a
check
> that asks just for the store(s) that are available.

Ok, will do #1 and check with awoz@ if it doesn't break anything. Thanks!
Sign in to reply to this message.

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