Make ChromeSubresourceFilterClient a WebContentsUserData

This also modifies the ownership of the subresource filter client, which now must
outlive the throttle manager / driver factory.

BUG=717590

Review-Url: https://codereview.chromium.org/2850373002
Cr-Commit-Position: refs/heads/master@{#469331}

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

@@ -17,14 +17,20 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "components/subresource_filter/content/browser/content_ruleset_service.h"
+#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 #include "content/public/browser/navigation_handle.h"
 
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSubresourceFilterClient);
+
 ChromeSubresourceFilterClient::ChromeSubresourceFilterClient(
     content::WebContents* web_contents)
     : web_contents_(web_contents), did_show_ui_for_navigation_(false) {
   DCHECK(web_contents);
   SubresourceFilterProfileContextFactory::EnsureForProfile(
       Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+
+  subresource_filter::ContentSubresourceFilterDriverFactory::
+      CreateForWebContents(web_contents, this);
 }
 
 ChromeSubresourceFilterClient::~ChromeSubresourceFilterClient() {}

chrome/browser/subresource_filter/chrome_subresource_filter_client.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/subresource_filter/content/browser/subresource_filter_client.h"
+#include "content/public/browser/web_contents_user_data.h"
 
 class GURL;
 
@@ -61,8 +62,11 @@ enum SubresourceFilterAction {
 };
 
 // Chrome implementation of SubresourceFilterClient.
+// TODO(csharrison): Make this a WebContentsObserver and own the throttle
+// manager directly.
 class ChromeSubresourceFilterClient
-    : public subresource_filter::SubresourceFilterClient {
+    : public content::WebContentsUserData<ChromeSubresourceFilterClient>,
+      public subresource_filter::SubresourceFilterClient {
  public:
   explicit ChromeSubresourceFilterClient(content::WebContents* web_contents);
   ~ChromeSubresourceFilterClient() override;

chrome/browser/subresource_filter/subresource_filter_unittest.cc

@@ -23,7 +23,6 @@
 #include "components/prefs/testing_pref_service.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
 #include "components/subresource_filter/content/browser/content_ruleset_service.h"
-#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 #include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
 #include "components/subresource_filter/core/browser/ruleset_service.h"
 #include "components/subresource_filter/core/browser/subresource_filter_features.h"
@@ -102,13 +101,8 @@ class SubresourceFilterTest : public ChromeRenderViewHostTestHarness {
     // Set up the tab helpers.
     InfoBarService::CreateForWebContents(web_contents());
     TabSpecificContentSettings::CreateForWebContents(web_contents());
+    ChromeSubresourceFilterClient::CreateForWebContents(web_contents());
 
-    std::unique_ptr<ChromeSubresourceFilterClient> subresource_filter_client(
-        new ChromeSubresourceFilterClient(web_contents()));
-    client_ = subresource_filter_client.get();
-    subresource_filter::ContentSubresourceFilterDriverFactory::
-        CreateForWebContents(web_contents(),
-                             std::move(subresource_filter_client));
     base::RunLoop().RunUntilIdle();
   }
 
@@ -153,7 +147,9 @@ class SubresourceFilterTest : public ChromeRenderViewHostTestHarness {
         url, safe_browsing::SB_THREAT_TYPE_SUBRESOURCE_FILTER);
   }
 
-  ChromeSubresourceFilterClient* client() { return client_; }
+  ChromeSubresourceFilterClient* client() {
+    return ChromeSubresourceFilterClient::FromWebContents(web_contents());
+  }
 
  private:
   base::ScopedTempDir ruleset_service_dir_;
@@ -163,8 +159,6 @@ class SubresourceFilterTest : public ChromeRenderViewHostTestHarness {
 
   scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
 
-  ChromeSubresourceFilterClient* client_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(SubresourceFilterTest);
 };
 

chrome/browser/ui/tab_helpers.cc

@@ -174,6 +174,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
       web_contents,
       autofill::ChromeAutofillClient::FromWebContents(web_contents));
+  ChromeSubresourceFilterClient::CreateForWebContents(web_contents);
   ChromeTranslateClient::CreateForWebContents(web_contents);
   CoreTabHelper::CreateForWebContents(web_contents);
   data_use_measurement::DataUseWebContentsObserver::CreateForWebContents(
@@ -204,10 +205,6 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
   SecurityStateTabHelper::CreateForWebContents(web_contents);
   if (SiteEngagementService::IsEnabled())
     SiteEngagementService::Helper::CreateForWebContents(web_contents);
-  std::unique_ptr<ChromeSubresourceFilterClient> subresource_filter_client(
-      new ChromeSubresourceFilterClient(web_contents));
-  subresource_filter::ContentSubresourceFilterDriverFactory::
-      CreateForWebContents(web_contents, std::move(subresource_filter_client));
   sync_sessions::SyncSessionsRouterTabHelper::CreateForWebContents(
       web_contents,
       sync_sessions::SyncSessionsWebContentsRouterFactory::GetForProfile(

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc

@@ -20,13 +20,13 @@
 #include "net/base/net_errors.h"
 #include "url/gurl.h"
 
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(
+    subresource_filter::ContentSubresourceFilterDriverFactory);
+
 namespace subresource_filter {
 
 namespace {
 
-const char kWebContentsUserDataKey[] =
-    "web_contents_subresource_filter_driver_factory";
-
 std::string DistillURLToHostAndPath(const GURL& url) {
   return url.host() + url.path();
 }
@@ -57,21 +57,12 @@ bool ShouldMeasurePerformanceForPageLoad(double performance_measurement_rate) {
 // static
 void ContentSubresourceFilterDriverFactory::CreateForWebContents(
     content::WebContents* web_contents,
-    std::unique_ptr<SubresourceFilterClient> client) {
+    SubresourceFilterClient* client) {
   if (FromWebContents(web_contents))
     return;
   web_contents->SetUserData(
-      kWebContentsUserDataKey,
-      base::MakeUnique<ContentSubresourceFilterDriverFactory>(
-          web_contents, std::move(client)));
-}
-
-// static
-ContentSubresourceFilterDriverFactory*
-ContentSubresourceFilterDriverFactory::FromWebContents(
-    content::WebContents* web_contents) {
-  return static_cast<ContentSubresourceFilterDriverFactory*>(
-      web_contents->GetUserData(kWebContentsUserDataKey));
+      UserDataKey(), base::MakeUnique<ContentSubresourceFilterDriverFactory>(
+                         web_contents, client));
 }
 
 // static
@@ -86,9 +77,9 @@ bool ContentSubresourceFilterDriverFactory::NavigationIsPageReload(
 
 ContentSubresourceFilterDriverFactory::ContentSubresourceFilterDriverFactory(
     content::WebContents* web_contents,
-    std::unique_ptr<SubresourceFilterClient> client)
+    SubresourceFilterClient* client)
     : content::WebContentsObserver(web_contents),
-      client_(std::move(client)),
+      client_(client),
       throttle_manager_(
           base::MakeUnique<ContentSubresourceFilterThrottleManager>(
               this,

components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h

@@ -13,11 +13,11 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/supports_user_data.h"
 #include "base/time/time.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
 #include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 
@@ -42,7 +42,8 @@ using URLToActivationListsMap =
 // WebContents and is responsible for sending the activation signal to all the
 // per-frame SubresourceFilterAgents on the renderer side.
 class ContentSubresourceFilterDriverFactory
-    : public base::SupportsUserData::Data,
+    : public content::WebContentsUserData<
+          ContentSubresourceFilterDriverFactory>,
       public content::WebContentsObserver,
       public ContentSubresourceFilterThrottleManager::Delegate {
  public:
@@ -72,11 +73,8 @@ class ContentSubresourceFilterDriverFactory
     ACTIVATION_DECISION_MAX
   };
 
-  static void CreateForWebContents(
-      content::WebContents* web_contents,
-      std::unique_ptr<SubresourceFilterClient> client);
-  static ContentSubresourceFilterDriverFactory* FromWebContents(
-      content::WebContents* web_contents);
+  static void CreateForWebContents(content::WebContents* web_contents,
+                                   SubresourceFilterClient* client);
 
   // Whether the |url|, |referrer|, and |transition| are considered to be
   // associated with a page reload.
@@ -86,7 +84,7 @@ class ContentSubresourceFilterDriverFactory
 
   explicit ContentSubresourceFilterDriverFactory(
       content::WebContents* web_contents,
-      std::unique_ptr<SubresourceFilterClient> client);
+      SubresourceFilterClient* client);
   ~ContentSubresourceFilterDriverFactory() override;
 
   // Called when Safe Browsing detects that the |url| corresponding to the load
@@ -119,7 +117,7 @@ class ContentSubresourceFilterDriverFactory
     return throttle_manager_.get();
   }
 
-  SubresourceFilterClient* client() { return client_.get(); }
+  SubresourceFilterClient* client() { return client_; }
 
  private:
   friend class ContentSubresourceFilterDriverFactoryTest;
@@ -149,7 +147,8 @@ class ContentSubresourceFilterDriverFactory
   void RecordRedirectChainMatchPatternForList(
       ActivationList activation_list) const;
 
-  std::unique_ptr<SubresourceFilterClient> client_;
+  // Must outlive this class.
+  SubresourceFilterClient* client_;
 
   std::unique_ptr<ContentSubresourceFilterThrottleManager> throttle_manager_;
 

components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc

@@ -236,9 +236,10 @@ class ContentSubresourceFilterDriverFactoryTest
         base::MessageLoop::current()->task_runner());
     ruleset_dealer_->SetRulesetFile(
         testing::TestRuleset::Open(test_ruleset_pair_.indexed));
-    client_ = new MockSubresourceFilterClient(ruleset_dealer_.get());
+    client_ =
+        base::MakeUnique<MockSubresourceFilterClient>(ruleset_dealer_.get());
     ContentSubresourceFilterDriverFactory::CreateForWebContents(
-        RenderViewHostTestHarness::web_contents(), base::WrapUnique(client()));
+        RenderViewHostTestHarness::web_contents(), client());
     ResetConfigurationToEnableFilteringOnSocialEngineeringSites();
 
     // Add a subframe.
@@ -276,7 +277,7 @@ class ContentSubresourceFilterDriverFactoryTest
         RenderViewHostTestHarness::web_contents());
   }
 
-  MockSubresourceFilterClient* client() { return client_; }
+  MockSubresourceFilterClient* client() { return client_.get(); }
 
   content::RenderFrameHost* GetSubframeRFH() {
     for (content::RenderFrameHost* rfh :
@@ -493,9 +494,7 @@ class ContentSubresourceFilterDriverFactoryTest
   testing::TestRulesetCreator test_ruleset_creator_;
   testing::TestRulesetPair test_ruleset_pair_;
 
-  // Owned by the factory.
-  MockSubresourceFilterClient* client_;
-
+  std::unique_ptr<MockSubresourceFilterClient> client_;
   std::unique_ptr<VerifiedRulesetDealer::Handle> ruleset_dealer_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSubresourceFilterDriverFactoryTest);

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -130,10 +130,10 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
         ActivationList::SUBRESOURCE_FILTER));
     test_io_task_runner_ = new base::TestMockTimeTaskRunner();
     // Note: Using NiceMock to allow uninteresting calls and suppress warnings.
-    auto client =
+    client_ =
         base::MakeUnique<::testing::NiceMock<MockSubresourceFilterClient>>();
     ContentSubresourceFilterDriverFactory::CreateForWebContents(
-        RenderViewHostTestHarness::web_contents(), std::move(client));
+        RenderViewHostTestHarness::web_contents(), client_.get());
     fake_safe_browsing_database_ = new FakeSafeBrowsingDatabaseManager();
     NavigateAndCommit(GURL("https://test.com"));
     Observe(RenderViewHostTestHarness::web_contents());
@@ -235,6 +235,7 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
   testing::ScopedSubresourceFilterConfigurator scoped_configuration_;
   scoped_refptr<base::TestMockTimeTaskRunner> test_io_task_runner_;
   std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
+  std::unique_ptr<SubresourceFilterClient> client_;
   scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
   base::HistogramTester tester_;