Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770153004:
Unittests for the Safe Browsing Subresource filter navigation throttle (Closed)
DescriptionUnittests for the Safe Browsing Subresource filter navigation throttle.
The CL moves the SafeBrowsingActivationThrottle to production and removes the feature (kSubresourceFilterSafeBrowsingActivationThrottle, which has guarded it's creation.
BUG=671962
Review-Url: https://codereview.chromium.org/2770153004
Cr-Commit-Position: refs/heads/master@{#462823}
Committed: https://chromium.googlesource.com/chromium/src/+/8bc2871191cf68aa1989f57f9675b90e9b40dd5b
Patch Set 1 : . #Patch Set 2 : . #
Total comments: 28
Patch Set 3 : fix tests according to new architecture #
Total comments: 10
Patch Set 4 : comments #
Total comments: 22
Patch Set 5 : . #
Messages
Total messages: 50 (38 generated)
Patchset #1 (id:1) has been deleted
melandory@chromium.org changed reviewers: + engedy@chromium.org
engedy@, could you please take a look before going to OOO, Thanks!
Patchset #1 (id:20001) has been deleted
melandory@chromium.org changed reviewers: + csharrison@chromium.org
csharrison@, could you please take a look. I don't want to land https://codereview.chromium.org/2645283007/ before this one is reviewed. Thanks in advance.
Patchset #1 (id:40001) has been deleted
A few initial comments. I wonder if we could be testing some other state other than the redirect chain histograms? Can we query the driver factory? https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:24: } // namespace nit: " // namespace" is the proper style, ditto in the test file. https://google.github.io/styleguide/cppguide.html#Namespaces https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:31: namespace { nit: nest anonymous namespaces inside the subresource_filter namespace. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:45: enum RedirectChainMatchPattern { Can you add a test for other redirect patterns other than NO_REDIRECTS_HIT? https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:66: void AddBlacklistedURL(const GURL& url, nit: prefer "Url" to "URL" if it is possible to be consistent in a file. ditto in other places here. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:71: void simulateTimeout() { simulate_timeout_ = true; } nit: SimulateTimeout() https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:112: bool simulate_timeout_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:141: client_ = new MockSubresourceFilterClient(); nit: I slightly prefer auto client = base::MakeUnique<MockSubresourceFilterClient>(); client_ = client.get(); ...CreateForWebContents...std::move(client)...; to avoid bare "new" https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:149: void TearDown() override { content::RenderViewHostTestHarness::TearDown(); } Should not be needed. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:160: void RunUntilIdle() { base::RunLoop().RunUntilIdle(); } Unused method (also can remove run_loop.h) https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:162: void SimulateStartAndExpectResult( The Simulate*AndExpectResult methods are unused. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:204: void SimulateRedirectAndExpectToProceed(const GURL& new_url) { Unused method (but you should use it, as mentioned above). https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:216: MockSubresourceFilterClient* client() { return client_; } Don't think you need to expose this as a method. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: content::NavigationThrottle::ThrottleCheckResult SimulateWillStart( These two Simulate* methods are unused. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:243: std::unique_ptr<content::NavigationHandle> test_handle_; Remove unused member.
Yes, it would be great if we could either query the driver factory, or inject a mock of it to the WebContents under test and make expectations on that.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:100001) has been deleted
PTAL https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc (right): https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:24: } // namespace On 2017/03/24 17:46:50, Charlie Harrison wrote: > nit: > " // namespace" is the proper style, ditto in the test file. > https://google.github.io/styleguide/cppguide.html#Namespaces Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:31: namespace { On 2017/03/24 17:46:51, Charlie Harrison wrote: > nit: nest anonymous namespaces inside the subresource_filter namespace. Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:45: enum RedirectChainMatchPattern { On 2017/03/24 17:46:51, Charlie Harrison wrote: > Can you add a test for other redirect patterns other than NO_REDIRECTS_HIT? I can, but it doesn't make much sense. Currently, navigation throttle checks only last URL. And in a future many of these states will be removed, since SocEng check also will be part of Nav. Throttle and we're not planning to check all urls, only first and last. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:66: void AddBlacklistedURL(const GURL& url, On 2017/03/24 17:46:50, Charlie Harrison wrote: > nit: prefer "Url" to "URL" if it is possible to be consistent in a file. ditto > in other places here. Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:71: void simulateTimeout() { simulate_timeout_ = true; } On 2017/03/24 17:46:51, Charlie Harrison wrote: > nit: SimulateTimeout() Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:112: bool simulate_timeout_; On 2017/03/24 17:46:50, Charlie Harrison wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:141: client_ = new MockSubresourceFilterClient(); On 2017/03/24 17:46:50, Charlie Harrison wrote: > nit: I slightly prefer > > auto client = base::MakeUnique<MockSubresourceFilterClient>(); > client_ = client.get(); > ...CreateForWebContents...std::move(client)...; > > to avoid bare "new" Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:149: void TearDown() override { content::RenderViewHostTestHarness::TearDown(); } On 2017/03/24 17:46:50, Charlie Harrison wrote: > Should not be needed. Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:160: void RunUntilIdle() { base::RunLoop().RunUntilIdle(); } On 2017/03/24 17:46:50, Charlie Harrison wrote: > Unused method (also can remove run_loop.h) Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:162: void SimulateStartAndExpectResult( On 2017/03/24 17:46:50, Charlie Harrison wrote: > The Simulate*AndExpectResult methods are unused. Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:204: void SimulateRedirectAndExpectToProceed(const GURL& new_url) { On 2017/03/24 17:46:50, Charlie Harrison wrote: > Unused method (but you should use it, as mentioned above). Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:218: content::NavigationThrottle::ThrottleCheckResult SimulateWillStart( On 2017/03/24 17:46:50, Charlie Harrison wrote: > These two Simulate* methods are unused. Done. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:243: std::unique_ptr<content::NavigationHandle> test_handle_; On 2017/03/24 17:46:51, Charlie Harrison wrote: > Remove unused member. Done.
Generally looks good % comments. https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/80001/components/subresource_... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:45: enum RedirectChainMatchPattern { On 2017/04/05 14:23:44, melandory wrote: > On 2017/03/24 17:46:51, Charlie Harrison wrote: > > Can you add a test for other redirect patterns other than NO_REDIRECTS_HIT? > > I can, but it doesn't make much sense. Currently, navigation throttle checks > only last URL. And in a future many of these states will be removed, since > SocEng check also will be part of Nav. Throttle and we're not planning to check > all urls, only first and last. Acknowledged. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:195: void SimulateStartAndExpectResult( I prefer the ExpectToProceed methods, because you'll never do anything except proceed. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:202: void SimulateRedirectAndExpectResult( Still unused, but you should use it :) https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:241: base::HistogramTester tester; nit: since all these tests use histogram tester, maybe just include it as an exposed member variable on the test harness? https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:245: EXPECT_THAT( Why not tester.ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix, 0) ? ditto for many of these checks. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:266: tester.GetAllSamples(kMatchesPatternHistogramNameSubresourceFilterSuffix), Why not: tester.ExpectUniqueSample(kMatchesPatternHistogramNameSubresourceFilterSuffix, NO_REDIRECTS_HIT, 1) ditto below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
https://codereview.chromium.org/2770153004/diff/160001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:195: void SimulateStartAndExpectResult( On 2017/04/05 15:33:04, Charlie Harrison wrote: > I prefer the ExpectToProceed methods, because you'll never do anything except > proceed. Done. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:202: void SimulateRedirectAndExpectResult( On 2017/04/05 15:33:04, Charlie Harrison wrote: > Still unused, but you should use it :) Done. For real now https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:241: base::HistogramTester tester; On 2017/04/05 15:33:04, Charlie Harrison wrote: > nit: since all these tests use histogram tester, maybe just include it as an > exposed member variable on the test harness? Done. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:245: EXPECT_THAT( On 2017/04/05 15:33:04, Charlie Harrison wrote: > Why not > tester.ExpectTotalCount(kMatchesPatternHistogramNameSubresourceFilterSuffix, 0) > ? > > ditto for many of these checks. Done. https://codereview.chromium.org/2770153004/diff/160001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:266: tester.GetAllSamples(kMatchesPatternHistogramNameSubresourceFilterSuffix), On 2017/04/05 15:33:04, Charlie Harrison wrote: > Why not: > tester.ExpectUniqueSample(kMatchesPatternHistogramNameSubresourceFilterSuffix, > NO_REDIRECTS_HIT, 1) > > ditto below. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
lgtm
Looks great, LGTM % comments. Thanks! https://codereview.chromium.org/2770153004/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.cc (right): https://codereview.chromium.org/2770153004/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.cc:17: if (navigation_handle->IsInMainFrame() && safe_browsing_service && Could you please point out in the CL description that this not only adds tests, but moves the production code for the throttle out from behind the feature flag. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:15: #include "components/subresource_filter/core/browser/subresource_filter_features.h" nit: Do we use this? https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:17: #include "components/subresource_filter/core/common/activation_level.h" nit: I think 17--20 are not used. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:26: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" nit: I think this is included by the one below. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:135: // Used to simulate WillProcessResonse for the factory. phrasing nit: // Throttle to call WillProcessResponse on the factory, which is otherwise called by the ThrottleManager. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:136: class ForwardingNavigationThrottle : public content::NavigationThrottle { naming nit: TestForwardingNavigationThrottle https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:176: NavigateAndCommit(GURL(kURL)); Let's navigate to some other URL here, it's unclear if navigating to this same URL again later influences the results. (I think this navigation could be just removed?) https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:233: std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle> nit: Could you please move this next to |field_trial_list_|? https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:240: TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, NoHitNavigation) { nit: Could we name these test cases using the pattern `scenario_result`, e.g., "ListNotMatched_NoActivation", "ListMatched_Activation", "ListMatchedAfterRedirect_...". https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:313: } // namespace subresource_filter Could you please add one more test that destroys the Navigation along with corresponding throttles while the SB check is pending? (To be run by ASAN bots to ensure no use-after-free.) https://codereview.chromium.org/2770153004/diff/180001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (left): https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:22: extern const base::Feature kSubresourceFilterSafeBrowsingActivationThrottle; ... and also mention that this CL removes the feature that used to guard it, too.
Description was changed from ========== Unittests for the Safe Browsing Subresource filter navigation throttle BUG=671962 ========== to ========== Unittests for the Safe Browsing Subresource filter navigation throttle. The CL moves the SafeBrowsingActivationThrottle to production and removes the feature (kSubresourceFilterSafeBrowsingActivationThrottle, which has guarded it's creation. BUG=671962 ==========
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2770153004/diff/180001/chrome/browser/subreso... File chrome/browser/subresource_filter/navigation_throttle_util.cc (right): https://codereview.chromium.org/2770153004/diff/180001/chrome/browser/subreso... chrome/browser/subresource_filter/navigation_throttle_util.cc:17: if (navigation_handle->IsInMainFrame() && safe_browsing_service && On 2017/04/06 13:50:38, engedy wrote: > Could you please point out in the CL description that this not only adds tests, > but moves the production code for the throttle out from behind the feature flag. Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... File components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc (right): https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:15: #include "components/subresource_filter/core/browser/subresource_filter_features.h" On 2017/04/06 13:50:38, engedy wrote: > nit: Do we use this? Yes, for feature setup https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:17: #include "components/subresource_filter/core/common/activation_level.h" On 2017/04/06 13:50:38, engedy wrote: > nit: I think 17--20 are not used. Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:26: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" On 2017/04/06 13:50:38, engedy wrote: > nit: I think this is included by the one below. Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:135: // Used to simulate WillProcessResonse for the factory. On 2017/04/06 13:50:38, engedy wrote: > phrasing nit: > > // Throttle to call WillProcessResponse on the factory, which is otherwise > called by the ThrottleManager. Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:136: class ForwardingNavigationThrottle : public content::NavigationThrottle { On 2017/04/06 13:50:38, engedy wrote: > naming nit: TestForwardingNavigationThrottle Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:176: NavigateAndCommit(GURL(kURL)); On 2017/04/06 13:50:38, engedy wrote: > Let's navigate to some other URL here, it's unclear if navigating to this same > URL again later influences the results. > > (I think this navigation could be just removed?) Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:233: std::unique_ptr<testing::ScopedSubresourceFilterFeatureToggle> On 2017/04/06 13:50:38, engedy wrote: > nit: Could you please move this next to |field_trial_list_|? Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:240: TEST_F(SubresourceFilterSafeBrowsingActivationThrottleTest, NoHitNavigation) { On 2017/04/06 13:50:38, engedy wrote: > nit: Could we name these test cases using the pattern `scenario_result`, e.g., > "ListNotMatched_NoActivation", "ListMatched_Activation", > "ListMatchedAfterRedirect_...". Done. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc:313: } // namespace subresource_filter On 2017/04/06 13:50:38, engedy wrote: > Could you please add one more test that destroys the Navigation along with > corresponding throttles while the SB check is pending? (To be run by ASAN bots > to ensure no use-after-free.) As per offline discussion, it's tricky to implement before we implement different check logic, so TODO is added. https://codereview.chromium.org/2770153004/diff/180001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.h (left): https://codereview.chromium.org/2770153004/diff/180001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.h:22: extern const base::Feature kSubresourceFilterSafeBrowsingActivationThrottle; On 2017/04/06 13:50:38, engedy wrote: > ... and also mention that this CL removes the feature that used to guard it, > too. Done.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2770153004/#ps200001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491558518240620,
"parent_rev": "61577bba5aca548c75a023abdc4d7c303d465a62", "commit_rev":
"8bc2871191cf68aa1989f57f9675b90e9b40dd5b"}
Message was sent while issue was closed.
Description was changed from ========== Unittests for the Safe Browsing Subresource filter navigation throttle. The CL moves the SafeBrowsingActivationThrottle to production and removes the feature (kSubresourceFilterSafeBrowsingActivationThrottle, which has guarded it's creation. BUG=671962 ========== to ========== Unittests for the Safe Browsing Subresource filter navigation throttle. The CL moves the SafeBrowsingActivationThrottle to production and removes the feature (kSubresourceFilterSafeBrowsingActivationThrottle, which has guarded it's creation. BUG=671962 Review-Url: https://codereview.chromium.org/2770153004 Cr-Commit-Position: refs/heads/master@{#462823} Committed: https://chromium.googlesource.com/chromium/src/+/8bc2871191cf68aa1989f57f9675... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8bc2871191cf68aa1989f57f9675... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
