Allow SubresourceFilterAgent::GetInheritedActivationState to be static

Currently, it is a bit confusing that the function is not static, given
its render_frame argument.  Additionally, it currently violates the rule
against virtual methods in constructors. We add an extra layer of
indirection: GetInheritedActivationStateForNewDocument(). This can then
be overridden by the test harness without affecting the constructor.

Bug: 1134747
Change-Id: I0db55c7fdaaf52992dc23ee9a568f82a22d0161b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495911Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Alex Turner <alexmt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823426}

components/subresource_filter/content/renderer/subresource_filter_agent.cc

@@ -113,9 +113,9 @@ void SubresourceFilterAgent::SetIsAdSubframe(
   render_frame()->GetWebFrame()->SetIsAdSubframe(ad_frame_type);
 }
 
+// static
 mojom::ActivationState SubresourceFilterAgent::GetInheritedActivationState(
     content::RenderFrame* render_frame) {
-  DCHECK(ShouldInheritActivation(GetDocumentURL()));
   if (!render_frame)
     return mojom::ActivationState();
 
@@ -222,7 +222,7 @@ void SubresourceFilterAgent::DidCreateNewDocument() {
   first_document_ = false;
 
   const mojom::ActivationState activation_state =
-      ShouldInheritActivation(url) ? GetInheritedActivationState(render_frame())
+      ShouldInheritActivation(url) ? GetInheritedActivationStateForNewDocument()
                                    : activation_state_for_next_document_;
 
   ResetInfoForNextDocument();
@@ -234,6 +234,12 @@ void SubresourceFilterAgent::DidCreateNewDocument() {
   ConstructFilter(activation_state, url);
 }
 
+const mojom::ActivationState
+SubresourceFilterAgent::GetInheritedActivationStateForNewDocument() {
+  DCHECK(ShouldInheritActivation(GetDocumentURL()));
+  return GetInheritedActivationState(render_frame());
+}
+
 void SubresourceFilterAgent::ConstructFilter(
     const mojom::ActivationState activation_state,
     const GURL& url) {

components/subresource_filter/content/renderer/subresource_filter_agent.h

@@ -88,19 +88,20 @@ class SubresourceFilterAgent
       blink::mojom::AdFrameType ad_frame_type) override;
 
  private:
-  // Returns the activation state for the |render_frame| to inherit. Main frames
+  // Returns the activation state for the `render_frame` to inherit. Main frames
   // inherit from their opener frames, and subframes inherit from their parent
   // frames. Assumes that the parent/opener is in a local frame relative to this
-  // one, upon construction. This function is virtual (and not static) to ease
-  // testing.
-  // TODO(crbug.com/1134747): Modify the test harness to allow this to be static
-  virtual mojom::ActivationState GetInheritedActivationState(
+  // one, upon construction.
+  static mojom::ActivationState GetInheritedActivationState(
       content::RenderFrame* render_frame);
 
   void RecordHistogramsOnFilterCreation(
       const mojom::ActivationState& activation_state);
   void ResetInfoForNextDocument();
 
+  virtual const mojom::ActivationState
+  GetInheritedActivationStateForNewDocument();
+
   void ConstructFilter(const mojom::ActivationState activation_state,
                        const GURL& url);
 

components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc

@@ -82,10 +82,10 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
     return std::move(last_injected_filter_);
   }
 
-  void SetInheritedActivationState(mojom::ActivationLevel level) {
+  void SetInheritedActivationStateForNewDocument(mojom::ActivationLevel level) {
     mojom::ActivationState state;
     state.activation_level = level;
-    inherited_activation_state_ = state;
+    inherited_activation_state_for_new_document_ = state;
   }
 
   void SimulateNonInitialLoad() { SetFirstDocument(false); }
@@ -93,15 +93,15 @@ class SubresourceFilterAgentUnderTest : public SubresourceFilterAgent {
   using SubresourceFilterAgent::ActivateForNextCommittedLoad;
 
  private:
-  mojom::ActivationState GetInheritedActivationState(
-      content::RenderFrame*) override {
-    return inherited_activation_state_;
+  const mojom::ActivationState GetInheritedActivationStateForNewDocument()
+      override {
+    return inherited_activation_state_for_new_document_;
   }
 
   std::unique_ptr<blink::WebDocumentSubresourceFilter> last_injected_filter_;
   bool is_ad_subframe_ = false;
   bool is_main_frame_ = true;
-  mojom::ActivationState inherited_activation_state_;
+  mojom::ActivationState inherited_activation_state_for_new_document_;
 
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterAgentUnderTest);
 };
@@ -568,7 +568,8 @@ TEST_F(SubresourceFilterAgentTest,
       SetTestRulesetToDisallowURLsWithPathSuffix("somethingNotMatched"));
 
   agent()->SetIsMainFrame(false);
-  agent()->SetInheritedActivationState(mojom::ActivationLevel::kEnabled);
+  agent()->SetInheritedActivationStateForNewDocument(
+      mojom::ActivationLevel::kEnabled);
 
   EXPECT_CALL(*agent(), GetDocumentURL())
       .WillOnce(::testing::Return(GURL("about:blank")));
@@ -585,7 +586,8 @@ TEST_F(SubresourceFilterAgentTest,
       SetTestRulesetToDisallowURLsWithPathSuffix("somethingNotMatched"));
 
   agent()->SetIsMainFrame(true);
-  agent()->SetInheritedActivationState(mojom::ActivationLevel::kEnabled);
+  agent()->SetInheritedActivationStateForNewDocument(
+      mojom::ActivationLevel::kEnabled);
 
   EXPECT_CALL(*agent(), GetDocumentURL())
       .WillOnce(::testing::Return(GURL("about:blank")));