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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by Charlie Harrison
Modified:
3 months 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 Harrison
engedy WDYT about this patch? I felt like we've been relatively constrained in //chrome land, ...
3 months 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 ...
3 months ago (2017-04-20 12:34:33 UTC) #13
Charlie Harrison
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 ...
3 months 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 ...
3 months ago (2017-04-20 16:36:53 UTC) #17
Charlie Harrison
Thanks Balazs, this should make the smart UI changes much more testable.
3 months 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
3 months ago (2017-04-20 16:37:50 UTC) #21
Charlie Harrison
Let me rebase this on the NavigationSimulator fixup CL, actually
3 months ago (2017-04-20 16:39:57 UTC) #23
Charlie Harrison
NVM it isn't necessary. +clamy would you PTAL at this trivial nav simulator CL?
3 months ago (2017-04-20 16:42:36 UTC) #25
clamy
Thanks! Lgtm.
3 months 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
3 months ago (2017-04-20 17:06:03 UTC) #28
Charlie Harrison
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 ...
3 months 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 ...
3 months 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 ...
3 months 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
3 months ago (2017-04-21 04:30:12 UTC) #42
commit-bot: I haz the power
3 months 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 25c286973