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

Issue 2820933002: [subresource_filter] add //chrome level unit test harness (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 6 days ago by Charlie (ooo-ish until may 2)
Modified:
1 week, 1 day ago
Reviewers:
clamy, engedy
CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[subresource_filter] add //chrome level unit test harness The subresource_filter code is getting more dependent on //chrome level things like content settings. Right now it is tricky to get end to end testing without bringing up the whole browser, even for very simple tests. This patch adds a test harness which sets up all the necessary dependencies for subresource filtering. This should make testing new features easy and fast. BUG=709768 Review-Url: https://codereview.chromium.org/2820933002 Cr-Commit-Position: refs/heads/master@{#466270} Committed: https://chromium.googlesource.com/chromium/src/+/ac0e81d240f79c8fe074ba135e28bf34e6ed6ed8

Patch Set 1 #

Patch Set 2 : add some GN deps #

Patch Set 3 : add infobar serivice tab helper (fix android) #

Patch Set 4 : add a few #includes (trybots prev) #

Total comments: 24

Patch Set 5 : engedy review #

Patch Set 6 : fix leaks #

Patch Set 7 : remove dep #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -65 lines) Patch
M chrome/browser/subresource_filter/chrome_subresource_filter_client.h View 1 2 3 4 5 6 1 chunk +5 lines, -1 line 0 comments Download
M chrome/browser/subresource_filter/chrome_subresource_filter_client.cc View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
A chrome/browser/subresource_filter/subresource_filter_unittest.cc View 1 2 3 4 5 1 chunk +185 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M components/subresource_filter/content/browser/BUILD.gn View 1 2 3 4 2 chunks +7 lines, -1 line 0 comments Download
A components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h View 1 2 3 4 1 chunk +50 lines, -0 lines 0 comments Download
A components/subresource_filter/content/browser/fake_safe_browsing_database_manager.cc View 1 2 3 1 chunk +68 lines, -0 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 2 chunks +1 line, -58 lines 0 comments Download
M content/public/test/navigation_simulator.cc View 6 2 chunks +8 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 46 (31 generated)
Charlie (ooo-ish until may 2)
engedy WDYT about this patch? I felt like we've been relatively constrained in //chrome land, ...
1 week, 5 days ago (2017-04-16 22:01:07 UTC) #12
engedy
I will never say `no` to more tests, so I think this is awesome, thanks ...
1 week, 2 days ago (2017-04-20 12:34:33 UTC) #13
Charlie (ooo-ish until may 2)
Tried to address most comments https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/chrome_subresource_filter_client.h File chrome/browser/subresource_filter/chrome_subresource_filter_client.h (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/chrome_subresource_filter_client.h#newcode73 chrome/browser/subresource_filter/chrome_subresource_filter_client.h:73: bool shown_for_navigation() { return ...
1 week, 1 day ago (2017-04-20 15:55:21 UTC) #15
engedy
LGTM. https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc#newcode67 chrome/browser/subresource_filter/subresource_filter_unittest.cc:67: auto* safe_browsing_service = On 2017/04/20 15:55:20, Charlie Harrison ...
1 week, 1 day ago (2017-04-20 16:36:53 UTC) #17
Charlie (ooo-ish until may 2)
Thanks Balazs, this should make the smart UI changes much more testable.
1 week, 1 day ago (2017-04-20 16:37:25 UTC) #20
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/2820933002/80001
1 week, 1 day ago (2017-04-20 16:37:50 UTC) #21
Charlie (ooo-ish until may 2)
Let me rebase this on the NavigationSimulator fixup CL, actually
1 week, 1 day ago (2017-04-20 16:39:57 UTC) #23
Charlie (ooo-ish until may 2)
NVM it isn't necessary. +clamy would you PTAL at this trivial nav simulator CL?
1 week, 1 day ago (2017-04-20 16:42:36 UTC) #25
clamy
Thanks! Lgtm.
1 week, 1 day ago (2017-04-20 17:00:49 UTC) #26
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/2820933002/80001
1 week, 1 day ago (2017-04-20 17:06:03 UTC) #28
Charlie (ooo-ish until may 2)
https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc#newcode112 chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 16:36:53, engedy wrote: > On 2017/04/20 ...
1 week, 1 day ago (2017-04-20 19:32:48 UTC) #31
engedy
https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc File chrome/browser/subresource_filter/subresource_filter_unittest.cc (right): https://codereview.chromium.org/2820933002/diff/60001/chrome/browser/subresource_filter/subresource_filter_unittest.cc#newcode112 chrome/browser/subresource_filter/subresource_filter_unittest.cc:112: TestingBrowserProcess::GetGlobal()->safe_browsing_service()->ShutDown(); On 2017/04/20 19:32:48, Charlie Harrison wrote: > On ...
1 week, 1 day ago (2017-04-20 19:35:20 UTC) #32
commit-bot: I haz the power
This CL has an open dependency (Issue 2826143002 Patch 1). Please resolve the dependency and ...
1 week, 1 day ago (2017-04-21 04:09:30 UTC) #39
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/2820933002/120001
1 week, 1 day ago (2017-04-21 04:30:12 UTC) #42
commit-bot: I haz the power
1 week, 1 day ago (2017-04-21 06:14:34 UTC) #46
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ac0e81d240f79c8fe074ba135e28...
Sign in to reply to this message.

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