Remove need for CaptivePortalBlockingPage custom subclasses

To componentize CaptivePortalBlockingPage, we are going to move the
Chrome-specific elements of its creation into
ChromeSecurityBlockingPageFactory. However, that move is currently
hindered by the fact that there are two subclasses of
CaptivePortalBlockingPage that supply the Wifi info for
testing/debugging purposes.

This CL eliminates the need for those custom subclasses by instead
adding a method to CaptivePortalBlockingPage to override the wifi info
for testing. This change will enable the transfer of all //chrome-level
creation sites of CaptivePortalBlockingPage to do so via
ChromeSecurityBlockingPageFactory in a followup CL.

Bug: 1025059
Change-Id: I9b6feaddcb52ee994ecae8dc3ba73c51878d48fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949469
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721926}

chrome/browser/ssl/captive_portal_blocking_page.cc

@@ -100,7 +100,18 @@ const void* CaptivePortalBlockingPage::GetTypeForTesting() {
   return CaptivePortalBlockingPage::kTypeForTesting;
 }
 
+void CaptivePortalBlockingPage::OverrideWifiInfoForTesting(
+    bool is_wifi_connection,
+    const std::string& wifi_ssid) {
+  is_wifi_info_overridden_for_testing_ = true;
+  is_wifi_connection_for_testing_ = is_wifi_connection;
+  wifi_ssid_for_testing_ = wifi_ssid;
+}
+
 bool CaptivePortalBlockingPage::IsWifiConnection() const {
+  if (is_wifi_info_overridden_for_testing_)
+    return is_wifi_connection_for_testing_;
+
   // |net::NetworkChangeNotifier::GetConnectionType| isn't accurate on Linux
   // and Windows. See https://crbug.com/160537 for details.
   // TODO(meacer): Add heuristics to get a more accurate connection type on
@@ -110,6 +121,9 @@ bool CaptivePortalBlockingPage::IsWifiConnection() const {
 }
 
 std::string CaptivePortalBlockingPage::GetWiFiSSID() const {
+  if (is_wifi_info_overridden_for_testing_)
+    return wifi_ssid_for_testing_;
+
   // On Windows and Mac, |WiFiService| provides an easy to use API to get the
   // currently associated WiFi access point. |WiFiService| isn't available on
   // Linux so |net::GetWifiSSID| is used instead.

chrome/browser/ssl/captive_portal_blocking_page.h

@@ -49,11 +49,14 @@ class CaptivePortalBlockingPage : public SSLBlockingPageBase {
   // InterstitialPageDelegate method:
   const void* GetTypeForTesting() override;
 
- protected:
-  // Returns true if the connection is a Wi-Fi connection. Virtual for tests.
-  virtual bool IsWifiConnection() const;
-  // Returns the SSID of the connected Wi-Fi network, if any. Virtual for tests.
-  virtual std::string GetWiFiSSID() const;
+  void OverrideWifiInfoForTesting(bool is_wifi_connection,
+                                  const std::string& wifi_ssid);
+
+ private:
+  // Returns true if the connection is a Wi-Fi connection.
+  bool IsWifiConnection() const;
+  // Returns the SSID of the connected Wi-Fi network, if any.
+  std::string GetWiFiSSID() const;
 
   // SecurityInterstitialPage methods:
   bool ShouldCreateNewNavigation() const override;
@@ -64,13 +67,16 @@ class CaptivePortalBlockingPage : public SSLBlockingPageBase {
   void CommandReceived(const std::string& command) override;
   void OverrideEntry(content::NavigationEntry* entry) override;
 
- private:
   // URL of the login page, opened when the user clicks the "Connect" button.
   // If empty, the default captive portal detection URL for the platform will be
   // used.
   const GURL login_url_;
   const net::SSLInfo ssl_info_;
 
+  bool is_wifi_info_overridden_for_testing_ = false;
+  bool is_wifi_connection_for_testing_ = false;
+  std::string wifi_ssid_for_testing_;
+
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPage);
 };
 

chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc

@@ -73,32 +73,6 @@ enum ExpectLoginURL {
   EXPECT_LOGIN_URL_YES
 };
 
-class CaptivePortalBlockingPageForTesting : public CaptivePortalBlockingPage {
- public:
-  CaptivePortalBlockingPageForTesting(
-      content::WebContents* web_contents,
-      const GURL& request_url,
-      const GURL& login_url,
-      std::unique_ptr<SSLCertReporter> ssl_cert_reporter,
-      const net::SSLInfo& ssl_info,
-      bool is_wifi,
-      const std::string& wifi_ssid)
-      : CaptivePortalBlockingPage(web_contents,
-                                  request_url,
-                                  login_url,
-                                  std::move(ssl_cert_reporter),
-                                  ssl_info,
-                                  net::ERR_CERT_COMMON_NAME_INVALID),
-        is_wifi_(is_wifi),
-        wifi_ssid_(wifi_ssid) {}
-
- private:
-  bool IsWifiConnection() const override { return is_wifi_; }
-  std::string GetWiFiSSID() const override { return wifi_ssid_; }
-  const bool is_wifi_;
-  const std::string wifi_ssid_;
-};
-
 // A NavigationThrottle that observes failed requests and shows a captive portal
 // interstitial.
 class CaptivePortalTestingNavigationThrottle
@@ -145,11 +119,11 @@ CaptivePortalTestingNavigationThrottle::WillFailRequest() {
   ssl_info.cert =
       net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
   ssl_info.cert_status = net::CERT_STATUS_COMMON_NAME_INVALID;
-  CaptivePortalBlockingPage* blocking_page =
-      new CaptivePortalBlockingPageForTesting(
-          navigation_handle()->GetWebContents(), GURL(kBrokenSSL), login_url_,
-          std::move(ssl_cert_reporter_), ssl_info,
-          is_wifi_connection_, wifi_ssid_);
+  CaptivePortalBlockingPage* blocking_page = new CaptivePortalBlockingPage(
+      navigation_handle()->GetWebContents(), GURL(kBrokenSSL), login_url_,
+      std::move(ssl_cert_reporter_), ssl_info,
+      net::ERR_CERT_COMMON_NAME_INVALID);
+  blocking_page->OverrideWifiInfoForTesting(is_wifi_connection_, wifi_ssid_);
 
   std::string html = blocking_page->GetHTMLContents();
   // Hand the blocking page back to the WebContents's
@@ -447,10 +421,10 @@ class CaptivePortalBlockingPageIDNTest : public SecurityInterstitialIDNTest {
       const GURL& request_url) const override {
     net::SSLInfo empty_ssl_info;
     // Blocking page is owned by the interstitial.
-    CaptivePortalBlockingPage* blocking_page =
-        new CaptivePortalBlockingPageForTesting(
-            contents, GURL(kBrokenSSL), request_url, nullptr, empty_ssl_info,
-            false, "");
+    CaptivePortalBlockingPage* blocking_page = new CaptivePortalBlockingPage(
+        contents, GURL(kBrokenSSL), request_url, nullptr, empty_ssl_info,
+        net::ERR_CERT_COMMON_NAME_INVALID);
+    blocking_page->OverrideWifiInfoForTesting(false, "");
     return blocking_page;
   }
 };

chrome/browser/ui/webui/interstitials/interstitial_ui.cc

@@ -107,38 +107,6 @@ class InterstitialHTMLSource : public content::URLDataSource {
   DISALLOW_COPY_AND_ASSIGN(InterstitialHTMLSource);
 };
 
-#if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
-// Provides fake connection information to the captive portal blocking page so
-// that both Wi-Fi and non Wi-Fi blocking pages can be displayed.
-class CaptivePortalBlockingPageWithNetInfo : public CaptivePortalBlockingPage {
- public:
-  CaptivePortalBlockingPageWithNetInfo(content::WebContents* web_contents,
-                                       const GURL& request_url,
-                                       const GURL& login_url,
-                                       const net::SSLInfo& ssl_info,
-                                       bool is_wifi,
-                                       const std::string& wifi_ssid)
-      : CaptivePortalBlockingPage(web_contents,
-                                  request_url,
-                                  login_url,
-                                  nullptr,
-                                  ssl_info,
-                                  net::ERR_CERT_COMMON_NAME_INVALID),
-        is_wifi_(is_wifi),
-        wifi_ssid_(wifi_ssid) {}
-
- private:
-  // CaptivePortalBlockingPage methods:
-  bool IsWifiConnection() const override { return is_wifi_; }
-  std::string GetWiFiSSID() const override { return wifi_ssid_; }
-
-  const bool is_wifi_;
-  const std::string wifi_ssid_;
-
-  DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPageWithNetInfo);
-};
-#endif
-
 SSLBlockingPage* CreateSslBlockingPage(content::WebContents* web_contents) {
   // Random parameters for SSL blocking page.
   int cert_error = net::ERR_CERT_CONTAINS_ERRORS;
@@ -427,10 +395,10 @@ CaptivePortalBlockingPage* CreateCaptivePortalBlockingPage(
   }
   net::SSLInfo ssl_info;
   ssl_info.cert = ssl_info.unverified_cert = CreateFakeCert();
-  CaptivePortalBlockingPage* blocking_page =
-      new CaptivePortalBlockingPageWithNetInfo(
-          web_contents, request_url, landing_url, ssl_info,
-          is_wifi_connection, wifi_ssid);
+  CaptivePortalBlockingPage* blocking_page = new CaptivePortalBlockingPage(
+      web_contents, request_url, landing_url, nullptr, ssl_info,
+      net::ERR_CERT_COMMON_NAME_INVALID);
+  blocking_page->OverrideWifiInfoForTesting(is_wifi_connection, wifi_ssid);
   return blocking_page;
 }
 #endif