Refactor subresource filter activation to support PlzNavigate cleanly.

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

Index: components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
diff --git a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
index fa76b0dccfd366a76b9eec3b7f30c55ce7b5b9e3..b793d20e7db36903b6095882d015eab7ee904675 100644
--- a/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
+++ b/components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
@@ -143,8 +143,7 @@ class MockSubresourceFilterDriver : public ContentSubresourceFilterDriver {
 
   ~MockSubresourceFilterDriver() override = default;
 
-  MOCK_METHOD3(ActivateForProvisionalLoad,
-               void(ActivationLevel, const GURL&, bool));
+  MOCK_METHOD2(ActivateForNextCommittedLoad, void(ActivationLevel, bool));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockSubresourceFilterDriver);
@@ -206,6 +205,17 @@ class ContentSubresourceFilterDriverFactoryTest
   MockSubresourceFilterDriver* subframe_driver() { return subframe_driver_; }
   content::RenderFrameHost* subframe_rfh() { return subframe_rfh_; }
 
+  void SimulateNavigationCommit(content::RenderFrameHost* rfh,
+                                const GURL& url) {
+    // RenderFrameHostTester currently only simulates a call to
+    // ReadyToCommitNavigation when browser-side-navigation is enabled.

[clamy, 2017/02/09 10:32:06]

Can you add a TODO to remove this function when crbug.com/688393 is fixed?
Thanks!

[engedy, 2017/02/09 13:42:01]

Done.

+    if (!content::IsBrowserSideNavigationEnabled()) {
+      factory()->ReadyToCommitNavigationInternal(rfh, url,
+                                                 false /* failed_navigation */);
+    }
+    content::RenderFrameHostTester::For(rfh)->SimulateNavigationCommit(url);
+  }
+
   void BlacklistURLWithRedirectsNavigateAndCommit(
       const std::vector<bool>& blacklisted_urls,
       const std::vector<GURL>& navigation_chain,
@@ -234,16 +244,11 @@ class ContentSubresourceFilterDriverFactoryTest
       }
       rfh_tester->SimulateRedirect(url);
     }
-    EXPECT_CALL(*driver(),
-                ActivateForProvisionalLoad(::testing::_, ::testing::_,
-                                           expected_measure_performance()))
-        .Times(expected_activation);
-    if (!content::IsBrowserSideNavigationEnabled()) {
-      factory()->ReadyToCommitNavigationInternal(main_rfh(),
-                                                 navigation_chain.back());
-    }
 
-    rfh_tester->SimulateNavigationCommit(navigation_chain.back());
+    EXPECT_CALL(*driver(), ActivateForNextCommittedLoad(
+                               ::testing::_, expected_measure_performance()))
+        .Times(expected_activation);
+    SimulateNavigationCommit(main_rfh(), navigation_chain.back());
     ::testing::Mock::VerifyAndClearExpectations(driver());
 
     if (expected_pattern != EMPTY) {
@@ -263,12 +268,14 @@ class ContentSubresourceFilterDriverFactoryTest
 
   void NavigateAndCommitSubframe(const GURL& url, bool expected_activation) {
     EXPECT_CALL(*subframe_driver(),
-                ActivateForProvisionalLoad(::testing::_, ::testing::_,
-                                           expected_measure_performance()))
+                ActivateForNextCommittedLoad(::testing::_,
+                                             expected_measure_performance()))
         .Times(expected_activation);
     EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
 
-    factory()->ReadyToCommitNavigationInternal(subframe_rfh(), url);
+    content::RenderFrameHostTester::For(subframe_rfh())
+        ->SimulateNavigationStart(url);
+    SimulateNavigationCommit(subframe_rfh(), url);
     ::testing::Mock::VerifyAndClearExpectations(subframe_driver());
     ::testing::Mock::VerifyAndClearExpectations(client());
   }
@@ -305,6 +312,23 @@ class ContentSubresourceFilterDriverFactoryTest
         main_rfh());
   }
 
+  void EmulateFailedNavigationAndExpectNoActivation(const GURL& url) {
+    EXPECT_CALL(*client(), ToggleNotificationVisibility(false)).Times(1);
+    EXPECT_CALL(*driver(),
+                ActivateForNextCommittedLoad(::testing::_, ::testing::_))
+        .Times(0);
+
+    // ReadyToCommitNavigation with browser-side navigation disabled is not
+    // called in production code. It is called with browser-side navigation

[clamy, 2017/02/09 10:32:06]

I'm not sure I understand this comment. We should call ReadyToCommitNavigation
both with and without PlzNavigate in production code. The issue is with the
testing code.

[engedy, 2017/02/09 13:42:01]

What I meant to write here: for *failed* navigations, ReadyToCommitNavigation is
invoked only for PlzNavigate, as otherwise the only hook into the network stack
is the NavigationResourceThrottle, and ResourceThrottle::WillProcessResponse is
not invoked by ThrottlingResourceHandler for failures such as network errors.

Clarified the comment.

+    // enabled, in which case RenderFrameHostTester already calls it.
+    content::RenderFrameHostTester* rfh_tester =
+        content::RenderFrameHostTester::For(main_rfh());
+    rfh_tester->SimulateNavigationStart(url);
+    rfh_tester->SimulateNavigationError(url, 403);
+    ::testing::Mock::VerifyAndClearExpectations(driver());
+    ::testing::Mock::VerifyAndClearExpectations(client());
+  }
+
   void EmulateInPageNavigation(const std::vector<bool>& blacklisted_urls,
                                RedirectChainMatchPattern expected_pattern,
                                bool expected_activation) {
@@ -318,8 +342,8 @@ class ContentSubresourceFilterDriverFactoryTest
 
     NavigateAndExpectActivation(blacklisted_urls, {GURL(kExampleUrl)},
                                 expected_pattern, expected_activation);
-    EXPECT_CALL(*driver(), ActivateForProvisionalLoad(
-                               ::testing::_, ::testing::_, ::testing::_))
+    EXPECT_CALL(*driver(),
+                ActivateForNextCommittedLoad(::testing::_, ::testing::_))
         .Times(0);
     EXPECT_CALL(*client(), ToggleNotificationVisibility(::testing::_)).Times(0);
     content::RenderFrameHostTester::For(main_rfh())
@@ -442,6 +466,17 @@ TEST_F(ContentSubresourceFilterDriverFactoryTest,
                           true /* expected_activation */);
 }
 
+TEST_F(ContentSubresourceFilterDriverFactoryTest, FailedNavigation) {
+  base::FieldTrialList field_trial_list(nullptr);
+  testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(
+      base::FeatureList::OVERRIDE_ENABLE_FEATURE, kActivationLevelEnabled,
+      kActivationScopeAllSites);
+  const GURL url(kExampleUrl);
+  NavigateAndExpectActivation({false}, {url}, EMPTY,
+                              true /* expected_activation */);
+  EmulateFailedNavigationAndExpectNoActivation(url);
+}
+
 TEST_F(ContentSubresourceFilterDriverFactoryTest, RedirectPatternTest) {
   base::FieldTrialList field_trial_list(nullptr);
   testing::ScopedSubresourceFilterFeatureToggle scoped_feature_toggle(