Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731013002:
ActivationStateComputingThrottle unit tests (Closed)
|
Created:
3 years, 9 months ago by Charlie Harrison Modified:
3 years, 9 months ago Reviewers:
engedy CC:
chromium-reviews, darin-cc_chromium.org, jam, subresource-filter-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionActivationStateComputingThrottle unit tests
Add unit tests using the new NavigationSimulator infrastructure
BUG=637415
Review-Url: https://codereview.chromium.org/2731013002
Cr-Commit-Position: refs/heads/master@{#456138}
Committed: https://chromium.googlesource.com/chromium/src/+/c83741624e8adee522ee85b96a237f9c73daee2d
Patch Set 1 #Patch Set 2 : rebase and touch up #
Total comments: 41
Patch Set 3 : engedy review #Patch Set 4 : use default args for whitelist domains #
Total comments: 16
Patch Set 5 : engedy review2 #
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + engedy@chromium.org
engedy: here is the ActivationStateComputing tests, ported to NavigationSimulator.
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 csharrison@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...
FYI: now rebased off of the l-g-t-m'd NavSimulator code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM % comments. I pointed out a couple of test cases we should ultimately have on some level (but I could be convinced to implement them for more high-level ThrottleManager tests). https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:24: #include "content/public/common/referrer.h" nit: Unused? https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:53: // content::WebContentsObserver: nit: Could you please move these further down into a protected: section? https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:85: "whitelist.com", proto::ACTIVATION_TYPE_DOCUMENT, {"parent.com"})); nit: Let's call these whitelisted.com and allow-child-to-be-whitelisted.com https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:100: NavigateAndCommit(document_url); Apart from this line, this function has no effect (i.e. |async_filter_| never used). I am fine with solutions on both end of the spectrum: -- Let the tests for subframe activation always use a hardcoded parent activation state, in which case this method can be replaced with just calling NavigateAndCommit. -- For these tests, trigger a real calculation of ActivationState in the parent, which is then somehow implicitly read by CreateSubframeAndInitTestNavigation. This might make some propagation tests a lot harder, though. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:162: return last_activation_state_.value(); nit: This essentially EXPECT's that las_ will have a value, but DCHECK if the expectation is not met, which is impolite as it will bring down the entire unittest. How about: EXPECT_TRUE(las.has_value()); return las.value_or(...); https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:181: DISALLOW_COPY_AND_ASSIGN(ActivationStateComputingNavigationThrottleTest); #include "base/macros.h" https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:189: NotifyPageActivation(ActivationState(ActivationLevel::ENABLED)); nit for follow-up CL: Ultimately we should have a version of this test where the notification arrives before WillStartRequest. This is an edge case that might happen if we have the Safe Browsing result cached. Currently it looks like this might be hard to simulate. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:199: CreateTestNavigationForMainFrame(GURL("http://example.test/inactivate.html")); nit: Let's remove the paths in all these URL, their presence suggests they play a role, which is confusing. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:218: // Notify that the page level state is explicitly disabled. Should be nit: s/level state/(level) activation/ https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:227: TEST_F(ActivationStateComputingNavigationThrottleTest, In addition to (or instead of) this test, let's have one that verifies that a subframe won't be whitelisted if it is embedded into "disallows-child-to-be-whitelisted.com". https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:227: TEST_F(ActivationStateComputingNavigationThrottleTest, Let's also have a test where the main frame is actually whitelisted (no domain filter option). https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:228: ActivateMainFrameWhitelistedSubframe) { nit: s/ActivateMainFrameWhitelistedSubframe/MainFrame_ActivationBecauseWhitelistNotApplies/ https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:261: DoNotActivateWhitelistedSubframe) { We should also a version to verify that proto::ACTIVATION_TYPE_GENERICBLOCK translates into ActivationState::generic_blocking_rules_disabled = true. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:274: EnsureDryRunIsPropagated) { We should also have a version to verify that page activation with DRYRUN translates into main frame activation with DRYRUN. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:284: EXPECT_EQ(ActivationLevel::DRYRUN, state.activation_level); nit: EXPECT_FALSE(state.filtering_disabled_for_document); EXPECT_FALSE(state.generic_blocking_rules_disabled); https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:306: DisabledStatePropagatedGeneric) { We should have a test (a new one or extend an existing) to verify that: (state.generic_blocking_rules_disabled = 1 for parent) + (completely whitelisted child) = (state.filtering_disabled_for_document = 1 for child) https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.cc:7: #include "testing/gtest/include/gtest/gtest.h" nit: Also #include base/bind.h and bind_helpers.h here. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:8: #include "components/subresource_filter/content/browser/async_document_subresource_filter.h" nit: Include activation_state.h instead and get rid of this? https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:10: #include "base/bind.h" nit: callback_forward.h
https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:185: TEST_F(ActivationStateComputingNavigationThrottleTest, ActivateMainFrame) { optional nit: If we had two test fixtures, ActivationStateComputingThrottle{Main|Sub}FrameTest, we would have an easier time naming the tests with the usual Scenario_ExpectatedBehavior pattern.
The CQ bit was checked by csharrison@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...
Thanks engedy. There were some fairly significant changes so you might want to look over again. Your call. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:24: #include "content/public/common/referrer.h" On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Unused? Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:53: // content::WebContentsObserver: On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Could you please move these further down into a protected: section? Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:85: "whitelist.com", proto::ACTIVATION_TYPE_DOCUMENT, {"parent.com"})); On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Let's call these http://whitelisted.com and http://allow-child-to-be-whitelisted.com Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:100: NavigateAndCommit(document_url); On 2017/03/10 10:56:57, engedy (slow) wrote: > Apart from this line, this function has no effect (i.e. |async_filter_| never > used). > > I am fine with solutions on both end of the spectrum: > -- Let the tests for subframe activation always use a hardcoded parent > activation state, in which case this method can be replaced with just calling > NavigateAndCommit. > -- For these tests, trigger a real calculation of ActivationState in the > parent, which is then somehow implicitly read by > CreateSubframeAndInitTestNavigation. This might make some propagation tests a > lot harder, though. Oops, sorry about that. I've reworked the tests to do your second idea, by persisting both the last activation state calculation, as well as the last committed render frame host. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:162: return last_activation_state_.value(); On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: This essentially EXPECT's that las_ will have a value, but DCHECK if the > expectation is not met, which is impolite as it will bring down the entire > unittest. How about: > > EXPECT_TRUE(las.has_value()); > return las.value_or(...); Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:181: DISALLOW_COPY_AND_ASSIGN(ActivationStateComputingNavigationThrottleTest); On 2017/03/10 10:56:57, engedy (slow) wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:185: TEST_F(ActivationStateComputingNavigationThrottleTest, ActivateMainFrame) { On 2017/03/10 11:53:41, engedy (slow) wrote: > optional nit: If we had two test fixtures, > ActivationStateComputingThrottle{Main|Sub}FrameTest, we would have an easier > time naming the tests with the usual Scenario_ExpectatedBehavior pattern. Done. I also rewrote some of the tests to fit the pattern better. Note that the fixtures need to be basically the same, so I made them both empty subclasses of the main one. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:189: NotifyPageActivation(ActivationState(ActivationLevel::ENABLED)); On 2017/03/10 10:56:57, engedy (slow) wrote: > nit for follow-up CL: Ultimately we should have a version of this test where the > notification arrives before WillStartRequest. This is an edge case that might > happen if we have the Safe Browsing result cached. Currently it looks like this > might be hard to simulate. Would this be before WillStartRequest but after DidStartNavigation? Let's do this in the ThrottleManager where we can have the mock page state activation throttle notify us early. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:199: CreateTestNavigationForMainFrame(GURL("http://example.test/inactivate.html")); On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Let's remove the paths in all these URL, their presence suggests they play > a role, which is confusing. Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:218: // Notify that the page level state is explicitly disabled. Should be On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: s/level state/(level) activation/ Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:227: TEST_F(ActivationStateComputingNavigationThrottleTest, On 2017/03/10 10:56:57, engedy (slow) wrote: > Let's also have a test where the main frame is actually whitelisted (no domain > filter option). Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:227: TEST_F(ActivationStateComputingNavigationThrottleTest, On 2017/03/10 10:56:57, engedy (slow) wrote: > In addition to (or instead of) this test, let's have one that verifies that a > subframe won't be whitelisted if it is embedded into > "disallows-child-to-be-whitelisted.com". Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:228: ActivateMainFrameWhitelistedSubframe) { On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: > s/ActivateMainFrameWhitelistedSubframe/MainFrame_ActivationBecauseWhitelistNotApplies/ Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:261: DoNotActivateWhitelistedSubframe) { On 2017/03/10 10:56:57, engedy (slow) wrote: > We should also a version to verify that proto::ACTIVATION_TYPE_GENERICBLOCK > translates into ActivationState::generic_blocking_rules_disabled = true. Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:274: EnsureDryRunIsPropagated) { On 2017/03/10 10:56:57, engedy (slow) wrote: > We should also have a version to verify that page activation with DRYRUN > translates into main frame activation with DRYRUN. This test now verifies that with the refactoring I did w/ page activation. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:284: EXPECT_EQ(ActivationLevel::DRYRUN, state.activation_level); On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: > > EXPECT_FALSE(state.filtering_disabled_for_document); > EXPECT_FALSE(state.generic_blocking_rules_disabled); Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:306: DisabledStatePropagatedGeneric) { On 2017/03/10 10:56:57, engedy (slow) wrote: > We should have a test (a new one or extend an existing) to verify that: > > (state.generic_blocking_rules_disabled = 1 for parent) + (completely whitelisted > child) = (state.filtering_disabled_for_document = 1 for child) Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.cc:7: #include "testing/gtest/include/gtest/gtest.h" On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Also #include base/bind.h and bind_helpers.h here. Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:8: #include "components/subresource_filter/content/browser/async_document_subresource_filter.h" On 2017/03/10 10:56:57, engedy (slow) wrote: > nit: Include activation_state.h instead and get rid of this? Done. https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h:10: #include "base/bind.h" On 2017/03/10 10:56:58, engedy (slow) wrote: > nit: callback_forward.h Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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.
Still LGTM % mostly nits, thanks a lot! https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/20001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:189: NotifyPageActivation(ActivationState(ActivationLevel::ENABLED)); On 2017/03/10 17:12:47, Charlie Harrison wrote: > On 2017/03/10 10:56:57, engedy (slow) wrote: > > nit for follow-up CL: Ultimately we should have a version of this test where > the > > notification arrives before WillStartRequest. This is an edge case that might > > happen if we have the Safe Browsing result cached. Currently it looks like > this > > might be hard to simulate. > > Would this be before WillStartRequest but after DidStartNavigation? Let's do > this in the ThrottleManager where we can have the mock page state activation > throttle notify us early. Exactly, the overall navigation will be in the WillStartRequest state, but WillStartRequest will not have been called on this throttle yet. The solution you suggested sounds great. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:85: void NavigateAndCommitToMainFrameWithPageActivation( nit: NavigateAndCommitMainFrameWithPageActivationState https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:210: class ActivationStateComputingThrottleMainFrameTest nit: Would declaring a type alias here work with GTest? https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:225: NoPageStateNotification_NoActivation) { nit: s/State/Activation/g here and in the test below. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:254: CreateTestNavigationForMainFrame(GURL("http://whitelisted.com/")); nit: Lines 254--260 are equivalent to another NavigateAndCommitToMainFrameWithPageActivation() call. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:263: EXPECT_FALSE(state.filtering_disabled_for_document); Let's check |generic_blocking_rules_disabled| here and in the tests below that check |filtering_disabled_for_document|. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:268: Whitelisted_DisablesDocumentFiltering) { nit: s/DisablesDocumentFiltering/DisablesFiltering/g, it's ambiguous what `document` means. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:278: // Subframe tests nit: Either remove this line or make it a section with: <blank> <blank> // Subframe tests ------------------------------------------------------------ <blank> https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:331: Whitelisted_DisablesGenericBlocking) { nit: DisablesGenericRules
Appreciate the prompt review https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:85: void NavigateAndCommitToMainFrameWithPageActivation( On 2017/03/10 18:33:43, engedy (slow) wrote: > nit: NavigateAndCommitMainFrameWithPageActivationState Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:210: class ActivationStateComputingThrottleMainFrameTest On 2017/03/10 18:33:43, engedy (slow) wrote: > nit: Would declaring a type alias here work with GTest? Yep, changed to typedefs. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:225: NoPageStateNotification_NoActivation) { On 2017/03/10 18:33:42, engedy (slow) wrote: > nit: s/State/Activation/g here and in the test below. Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:254: CreateTestNavigationForMainFrame(GURL("http://whitelisted.com/")); On 2017/03/10 18:33:42, engedy (slow) wrote: > nit: Lines 254--260 are equivalent to another > NavigateAndCommitToMainFrameWithPageActivation() call. Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:263: EXPECT_FALSE(state.filtering_disabled_for_document); On 2017/03/10 18:33:43, engedy (slow) wrote: > Let's check |generic_blocking_rules_disabled| here and in the tests below that > check |filtering_disabled_for_document|. Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:268: Whitelisted_DisablesDocumentFiltering) { On 2017/03/10 18:33:42, engedy (slow) wrote: > nit: s/DisablesDocumentFiltering/DisablesFiltering/g, it's ambiguous what > `document` means. Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:278: // Subframe tests On 2017/03/10 18:33:43, engedy (slow) wrote: > nit: Either remove this line or make it a section with: > > <blank> > <blank> > // Subframe tests ------------------------------------------------------------ > <blank> Done. https://codereview.chromium.org/2731013002/diff/60001/components/subresource_... components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc:331: Whitelisted_DisablesGenericBlocking) { On 2017/03/10 18:33:42, engedy (slow) wrote: > nit: DisablesGenericRules Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2731013002/#ps80001 (title: "engedy review2")
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": 80001, "attempt_start_ts": 1489171740550870,
"parent_rev": "cc3b684f89c787d632a25c6037edbc5ba71c7da6", "commit_rev":
"c83741624e8adee522ee85b96a237f9c73daee2d"}
Message was sent while issue was closed.
Description was changed from ========== ActivationStateComputingThrottle unit tests Add unit tests using the new NavigationSimulator infrastructure BUG=637415 ========== to ========== ActivationStateComputingThrottle unit tests Add unit tests using the new NavigationSimulator infrastructure BUG=637415 Review-Url: https://codereview.chromium.org/2731013002 Cr-Commit-Position: refs/heads/master@{#456138} Committed: https://chromium.googlesource.com/chromium/src/+/c83741624e8adee522ee85b96a23... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/c83741624e8adee522ee85b96a23... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
