Remove ConsoleLogger in favor of augmenting TestRenderFrameHost

This class was used to mock out console log requests flowing through
RenderFrameHost. Now all unit tests can get the same functionality from
TestRenderFrameHost/RenderFrameHostTester, without injecting a mock.

Bug: None
Change-Id: I76a2603e48c9e7afd97a9b85da12ae2d4fe5714f
Reviewed-on: https://chromium-review.googlesource.com/748770
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513219}

chrome/browser/ui/BUILD.gn

@@ -74,8 +74,6 @@ split_static_library("ui") {
     "autofill/popup_view_common.h",
     "blocked_content/blocked_window_params.cc",
     "blocked_content/blocked_window_params.h",
-    "blocked_content/console_logger.cc",
-    "blocked_content/console_logger.h",
     "blocked_content/popup_blocker_tab_helper.cc",
     "blocked_content/popup_blocker_tab_helper.h",
     "blocked_content/popup_opener_tab_helper.cc",

chrome/browser/ui/blocked_content/console_logger.cc

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/ui/blocked_content/console_logger.h"
-
-#include "base/logging.h"
-#include "content/public/browser/render_frame_host.h"
-
-ConsoleLogger::ConsoleLogger() = default;
-ConsoleLogger::~ConsoleLogger() = default;
-
-void ConsoleLogger::LogInFrame(content::RenderFrameHost* render_frame_host,
-                               content::ConsoleMessageLevel level,
-                               const std::string& message) {
-  DCHECK(render_frame_host);
-  render_frame_host->AddMessageToConsole(level, message);
-}

chrome/browser/ui/blocked_content/console_logger.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_UI_BLOCKED_CONTENT_CONSOLE_LOGGER_H_
-#define CHROME_BROWSER_UI_BLOCKED_CONTENT_CONSOLE_LOGGER_H_
-
-#include <string>
-
-#include "base/macros.h"
-#include "content/public/common/console_message_level.h"
-
-namespace content {
-class RenderFrameHost;
-}
-
-// This simple class just forwards console logging to the associated
-// RenderFrameHost, to send down to the renderer. It exists for unit tests to
-// mock out, allowing unit tests to expect console messages without having to
-// write a full browser test.
-class ConsoleLogger {
- public:
-  ConsoleLogger();
-  virtual ~ConsoleLogger();
-
-  virtual void LogInFrame(content::RenderFrameHost* render_frame_host,
-                          content::ConsoleMessageLevel level,
-                          const std::string& message);
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ConsoleLogger);
-};
-
-#endif  // CHROME_BROWSER_UI_BLOCKED_CONTENT_CONSOLE_LOGGER_H_

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc

@@ -14,7 +14,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
 #include "chrome/browser/ui/blocked_content/blocked_window_params.h"
-#include "chrome/browser/ui/blocked_content/console_logger.h"
 #include "chrome/browser/ui/blocked_content/popup_tracker.h"
 #include "chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.h"
 #include "chrome/browser/ui/browser_navigator.h"
@@ -55,9 +54,7 @@ struct PopupBlockerTabHelper::BlockedRequest {
 PopupBlockerTabHelper::PopupBlockerTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       safe_browsing_triggered_popup_blocker_(
-          SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
-              web_contents,
-              base::MakeUnique<ConsoleLogger>())) {}
+          SafeBrowsingTriggeredPopupBlocker::MaybeCreate(web_contents)) {}
 
 PopupBlockerTabHelper::~PopupBlockerTabHelper() {
 }

chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.cc

@@ -9,11 +9,11 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
-#include "chrome/browser/ui/blocked_content/console_logger.h"
 #include "components/safe_browsing/db/util.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/page_navigator.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/console_message_level.h"
 #include "third_party/WebKit/public/web/WebTriggeringEventInfo.h"
@@ -47,8 +47,7 @@ SafeBrowsingTriggeredPopupBlocker::PageData::~PageData() {
 // static
 std::unique_ptr<SafeBrowsingTriggeredPopupBlocker>
 SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
-    content::WebContents* web_contents,
-    std::unique_ptr<ConsoleLogger> logger) {
+    content::WebContents* web_contents) {
   if (!base::FeatureList::IsEnabled(kAbusiveExperienceEnforce))
     return nullptr;
 
@@ -57,8 +56,8 @@ SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
           web_contents);
   if (!observer_manager)
     return nullptr;
-  return base::WrapUnique(new SafeBrowsingTriggeredPopupBlocker(
-      web_contents, observer_manager, std::move(logger)));
+  return base::WrapUnique(
+      new SafeBrowsingTriggeredPopupBlocker(web_contents, observer_manager));
 }
 
 SafeBrowsingTriggeredPopupBlocker::~SafeBrowsingTriggeredPopupBlocker() =
@@ -81,20 +80,17 @@ bool SafeBrowsingTriggeredPopupBlocker::ShouldApplyStrongPopupBlocker(
   if (should_block) {
     LogAction(Action::kBlocked);
     current_page_data_->inc_num_popups_blocked();
-    logger_->LogInFrame(web_contents()->GetMainFrame(),
-                        content::CONSOLE_MESSAGE_LEVEL_ERROR,
-                        kAbusiveEnforceMessage);
+    web_contents()->GetMainFrame()->AddMessageToConsole(
+        content::CONSOLE_MESSAGE_LEVEL_ERROR, kAbusiveEnforceMessage);
   }
   return should_block;
 }
 
 SafeBrowsingTriggeredPopupBlocker::SafeBrowsingTriggeredPopupBlocker(
     content::WebContents* web_contents,
-    subresource_filter::SubresourceFilterObserverManager* observer_manager,
-    std::unique_ptr<ConsoleLogger> logger)
+    subresource_filter::SubresourceFilterObserverManager* observer_manager)
     : content::WebContentsObserver(web_contents),
       scoped_observer_(this),
-      logger_(std::move(logger)),
       current_page_data_(base::MakeUnique<PageData>()),
       ignore_sublists_(
           base::GetFieldTrialParamByFeatureAsBool(kAbusiveExperienceEnforce,
@@ -128,9 +124,8 @@ void SafeBrowsingTriggeredPopupBlocker::DidFinishNavigation(
     current_page_data_->set_is_triggered(true);
     LogAction(Action::kEnforcedSite);
   } else if (level == SubresourceFilterLevel::WARN) {
-    logger_->LogInFrame(web_contents()->GetMainFrame(),
-                        content::CONSOLE_MESSAGE_LEVEL_WARNING,
-                        kAbusiveWarnMessage);
+    web_contents()->GetMainFrame()->AddMessageToConsole(
+        content::CONSOLE_MESSAGE_LEVEL_WARNING, kAbusiveWarnMessage);
     LogAction(Action::kWarningSite);
   }
   LogAction(Action::kNavigation);

chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.h

@@ -21,7 +21,6 @@ struct OpenURLParams;
 class WebContents;
 }  // namespace content
 
-class ConsoleLogger;
 
 extern const base::Feature kAbusiveExperienceEnforce;
 
@@ -68,20 +67,18 @@ class SafeBrowsingTriggeredPopupBlocker
   };
 
   static std::unique_ptr<SafeBrowsingTriggeredPopupBlocker> MaybeCreate(
-      content::WebContents* web_contents,
-      std::unique_ptr<ConsoleLogger> logger);
+      content::WebContents* web_contents);
   ~SafeBrowsingTriggeredPopupBlocker() override;
 
   bool ShouldApplyStrongPopupBlocker(
       const content::OpenURLParams* open_url_params);
 
  private:
-  // The |web_contents|, |observer_manager|, and |logger| are expected to be
+  // The |web_contents| and |observer_manager| are expected to be
   // non-nullptr.
   SafeBrowsingTriggeredPopupBlocker(
       content::WebContents* web_contents,
-      subresource_filter::SubresourceFilterObserverManager* observer_manager,
-      std::unique_ptr<ConsoleLogger> logger);
+      subresource_filter::SubresourceFilterObserverManager* observer_manager);
 
   // content::WebContentsObserver:
   void DidFinishNavigation(
@@ -127,9 +124,6 @@ class SafeBrowsingTriggeredPopupBlocker
   base::Optional<safe_browsing::SubresourceFilterLevel>
       level_for_next_committed_navigation_;
 
-  // Should never be nullptr.
-  std::unique_ptr<ConsoleLogger> logger_;
-
   // Should never be nullptr.
   std::unique_ptr<PageData> current_page_data_;
 

chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc

@@ -16,12 +16,12 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
 #include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
-#include "chrome/browser/ui/blocked_content/console_logger.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "components/subresource_filter/content/browser/fake_safe_browsing_database_manager.h"
 #include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/web/WebTriggeringEventInfo.h"
 #include "ui/base/page_transition_types.h"
@@ -31,24 +31,6 @@
 const char kNumBlockedHistogram[] =
     "ContentSettings.Popups.StrongBlocker.NumBlocked";
 
-class TestConsoleLogger : public ConsoleLogger {
- public:
-  TestConsoleLogger() {}
-  ~TestConsoleLogger() override {}
-
-  void LogInFrame(content::RenderFrameHost* render_frame_host,
-                  content::ConsoleMessageLevel level,
-                  const std::string& message) override {
-    messages_.push_back(message);
-  }
-
-  const std::vector<std::string>& messages() { return messages_; }
-
- private:
-  std::vector<std::string> messages_;
-  DISALLOW_COPY_AND_ASSIGN(TestConsoleLogger);
-};
-
 class SafeBrowsingTriggeredPopupBlockerTest
     : public ChromeRenderViewHostTestHarness {
  public:
@@ -79,11 +61,8 @@ class SafeBrowsingTriggeredPopupBlockerTest
     ChromeSubresourceFilterClient::CreateForWebContents(web_contents());
 
     scoped_feature_list_ = DefaultFeatureList();
-
-    auto console_logger = base::MakeUnique<TestConsoleLogger>();
-    console_logger_ = console_logger.get();
-    popup_blocker_ = SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
-        web_contents(), std::move(console_logger));
+    popup_blocker_ =
+        SafeBrowsingTriggeredPopupBlocker::MaybeCreate(web_contents());
   }
 
   void TearDown() override {
@@ -140,16 +119,17 @@ class SafeBrowsingTriggeredPopupBlockerTest
     MarkUrlAsAbusiveWithLevel(url, safe_browsing::SubresourceFilterLevel::WARN);
   }
 
-  TestConsoleLogger* console_logger() { return console_logger_; }
+  const std::vector<std::string>& GetMainFrameConsoleMessages() {
+    content::RenderFrameHostTester* rfh_tester =
+        content::RenderFrameHostTester::For(main_rfh());
+    return rfh_tester->GetConsoleMessages();
+  }
 
  private:
   std::unique_ptr<base::test::ScopedFeatureList> scoped_feature_list_;
   scoped_refptr<FakeSafeBrowsingDatabaseManager> fake_safe_browsing_database_;
   std::unique_ptr<SafeBrowsingTriggeredPopupBlocker> popup_blocker_;
 
-  // Owned by the popup blocker.
-  TestConsoleLogger* console_logger_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingTriggeredPopupBlockerTest);
 };
 
@@ -185,11 +165,11 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerTest, MatchingURL_BlocksPopupAndLogs) {
   const GURL url("https://example.test/");
   MarkUrlAsAbusiveEnforce(url);
   NavigateAndCommit(url);
-  EXPECT_TRUE(console_logger()->messages().empty());
+  EXPECT_TRUE(GetMainFrameConsoleMessages().empty());
 
   EXPECT_TRUE(popup_blocker()->ShouldApplyStrongPopupBlocker(nullptr));
-  EXPECT_EQ(1u, console_logger()->messages().size());
-  EXPECT_EQ(console_logger()->messages().front(), kAbusiveEnforceMessage);
+  EXPECT_EQ(1u, GetMainFrameConsoleMessages().size());
+  EXPECT_EQ(GetMainFrameConsoleMessages().front(), kAbusiveEnforceMessage);
 }
 
 TEST_F(SafeBrowsingTriggeredPopupBlockerTest,
@@ -216,15 +196,15 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerTest, NoMatch_NoBlocking) {
   const GURL url("https://example.test/");
   NavigateAndCommit(url);
   EXPECT_FALSE(popup_blocker()->ShouldApplyStrongPopupBlocker(nullptr));
-  EXPECT_TRUE(console_logger()->messages().empty());
+  EXPECT_TRUE(GetMainFrameConsoleMessages().empty());
 }
 
 TEST_F(SafeBrowsingTriggeredPopupBlockerTest, NoFeature_NoCreating) {
-  EXPECT_NE(nullptr, SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
-                         web_contents(), base::MakeUnique<ConsoleLogger>()));
+  EXPECT_NE(nullptr,
+            SafeBrowsingTriggeredPopupBlocker::MaybeCreate(web_contents()));
   ResetFeatureAndGet();
-  EXPECT_EQ(nullptr, SafeBrowsingTriggeredPopupBlocker::MaybeCreate(
-                         web_contents(), base::MakeUnique<ConsoleLogger>()));
+  EXPECT_EQ(nullptr,
+            SafeBrowsingTriggeredPopupBlocker::MaybeCreate(web_contents()));
 }
 
 TEST_F(SafeBrowsingTriggeredPopupBlockerTest, OnlyBlockOnMatchingUrls) {
@@ -368,8 +348,8 @@ TEST_F(SafeBrowsingTriggeredPopupBlockerTest, WarningMatch_OnlyLogs) {
   NavigateAndCommit(url);
 
   // Warning should come at navigation commit time, not at popup time.
-  EXPECT_EQ(1u, console_logger()->messages().size());
-  EXPECT_EQ(console_logger()->messages().front(), kAbusiveWarnMessage);
+  EXPECT_EQ(1u, GetMainFrameConsoleMessages().size());
+  EXPECT_EQ(GetMainFrameConsoleMessages().front(), kAbusiveWarnMessage);
 
   EXPECT_FALSE(popup_blocker()->ShouldApplyStrongPopupBlocker(nullptr));
 }

content/public/test/test_renderer_host.h

@@ -8,6 +8,8 @@
 #include <stdint.h>
 
 #include <memory>
+#include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
@@ -121,6 +123,10 @@ class RenderFrameHostTester {
   virtual void SimulateFeaturePolicyHeader(
       blink::WebFeaturePolicyFeature feature,
       const std::vector<url::Origin>& whitelist) = 0;
+
+  // Gets all the console messages requested via
+  // RenderFrameHost::AddMessageToConsole in this frame.
+  virtual const std::vector<std::string>& GetConsoleMessages() = 0;
 };
 
 // An interface and utility for driving tests of RenderViewHost.

content/test/test_render_frame_host.cc

@@ -111,6 +111,12 @@ MockRenderProcessHost* TestRenderFrameHost::GetProcess() {
   return static_cast<MockRenderProcessHost*>(RenderFrameHostImpl::GetProcess());
 }
 
+void TestRenderFrameHost::AddMessageToConsole(ConsoleMessageLevel level,
+                                              const std::string& message) {
+  console_messages_.push_back(message);
+  RenderFrameHostImpl::AddMessageToConsole(level, message);
+}
+
 void TestRenderFrameHost::InitializeRenderFrameIfNeeded() {
   if (!render_view_host()->IsRenderViewLive()) {
     render_view_host()->GetProcess()->Init();
@@ -305,6 +311,10 @@ void TestRenderFrameHost::SimulateFeaturePolicyHeader(
   OnDidSetFeaturePolicyHeader(header);
 }
 
+const std::vector<std::string>& TestRenderFrameHost::GetConsoleMessages() {
+  return console_messages_;
+}
+
 void TestRenderFrameHost::SendNavigate(int nav_entry_id,
                                        bool did_create_new_entry,
                                        const GURL& url) {

content/test/test_render_frame_host.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <string>
 #include <vector>
 
 #include "base/macros.h"
@@ -52,6 +53,8 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
   // RenderFrameHostImpl overrides (same values, but in Test*/Mock* types)
   TestRenderViewHost* GetRenderViewHost() override;
   MockRenderProcessHost* GetProcess() override;
+  void AddMessageToConsole(ConsoleMessageLevel level,
+                           const std::string& message) override;
 
   // RenderFrameHostTester implementation.
   void InitializeRenderFrameIfNeeded() override;
@@ -71,6 +74,7 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
   void SimulateFeaturePolicyHeader(
       blink::WebFeaturePolicyFeature feature,
       const std::vector<url::Origin>& whitelist) override;
+  const std::vector<std::string>& GetConsoleMessages() override;
 
   void SendNavigateWithReplacement(int nav_entry_id,
                                    bool did_create_new_entry,
@@ -186,6 +190,9 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
 
   mojom::FrameNavigationControl* GetInternalNavigationControl();
 
+  // Keeps a running vector of messages sent to AddMessageToConsole.
+  std::vector<std::string> console_messages_;
+
   TestRenderFrameHostCreationObserver child_creation_observer_;
 
   std::string contents_mime_type_;