Captive Portal Interstitial: Use SSID even if connection type isn't Wi-Fi.

|IsWiFiConnection| isn't accurate on some platforms. Whenever SSID is available,
use it and ignore |IsWiFiConnection|. This CL also adds a delegate to
|CaptivePortalBlockingPage| for better testing.

BUG=451272

Review URL: https://codereview.chromium.org/894153003

Cr-Commit-Position: refs/heads/master@{#314391}

chrome/browser/ssl/captive_portal_blocking_page.cc

@@ -34,22 +34,29 @@ enum CaptivePortalBlockingPageEvent {
   CAPTIVE_PORTAL_BLOCKING_PAGE_EVENT_COUNT
 };
 
+const char kOpenLoginPageCommand[] = "openLoginPage";
+
 void RecordUMA(CaptivePortalBlockingPageEvent event) {
   UMA_HISTOGRAM_ENUMERATION("interstitial.captive_portal",
                             event,
                             CAPTIVE_PORTAL_BLOCKING_PAGE_EVENT_COUNT);
 }
 
-bool IsWifiConnection() {
-  // |net::NetworkChangeNotifier::GetConnectionType| isn't accurate on Linux and
-  // Windows. See https://crbug.com/160537 for details.
+class ConnectionInfoDelegate : public CaptivePortalBlockingPage::Delegate {
+ public:
+  ConnectionInfoDelegate() {}
+  ~ConnectionInfoDelegate() override {}
+
+  bool IsWifiConnection() const override {
+    // |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
     //               these platforms.
     return net::NetworkChangeNotifier::GetConnectionType() ==
            net::NetworkChangeNotifier::CONNECTION_WIFI;
-}
+  }
 
-std::string GetWiFiName() {
+  std::string GetWiFiSSID() const override {
     std::string ssid;
 #if defined(OS_WIN) || defined(OS_MACOSX)
     scoped_ptr<wifi::WiFiService> wifi_service(wifi::WiFiService::Create());
@@ -63,9 +70,8 @@ std::string GetWiFiName() {
     if (!base::IsStringUTF8(ssid))
       return "";
     return ssid;
-}
-
-const char kOpenLoginPageCommand[] = "openLoginPage";
+  }
+};
 
 } // namespace
 
@@ -80,7 +86,7 @@ CaptivePortalBlockingPage::CaptivePortalBlockingPage(
     const base::Callback<void(bool)>& callback)
     : SecurityInterstitialPage(web_contents, request_url),
       login_url_(login_url),
-      is_wifi_connection_(IsWifiConnection()),
+      delegate_(new ConnectionInfoDelegate),
       callback_(callback) {
   DCHECK(login_url_.is_valid());
   RecordUMA(SHOW_ALL);
@@ -109,81 +115,67 @@ void CaptivePortalBlockingPage::PopulateInterstitialStrings(
   load_time_data->SetString("type", "CAPTIVE_PORTAL");
   load_time_data->SetBoolean("overridable", false);
 
+  // |IsWifiConnection| isn't accurate on some platforms, so always try to get
+  // the Wi-Fi SSID even if |IsWifiConnection| is false.
+  std::string wifi_ssid = delegate_.get()->GetWiFiSSID();
+  bool is_wifi_connection = !wifi_ssid.empty() ||
+                            delegate_.get()->IsWifiConnection();
+
   load_time_data->SetString(
       "primaryButtonText",
       l10n_util::GetStringUTF16(IDS_CAPTIVE_PORTAL_BUTTON_OPEN_LOGIN_PAGE));
-  load_time_data->SetString("tabTitle",
-      l10n_util::GetStringUTF16(
-          is_wifi_connection_ ?
-          IDS_CAPTIVE_PORTAL_HEADING_WIFI :
-          IDS_CAPTIVE_PORTAL_HEADING_WIRED));
-  load_time_data->SetString("heading",
-      l10n_util::GetStringUTF16(
-          is_wifi_connection_ ?
-          IDS_CAPTIVE_PORTAL_HEADING_WIFI :
-          IDS_CAPTIVE_PORTAL_HEADING_WIRED));
+  load_time_data->SetString(
+      "tabTitle", l10n_util::GetStringUTF16(
+                      is_wifi_connection ? IDS_CAPTIVE_PORTAL_HEADING_WIFI
+                                         : IDS_CAPTIVE_PORTAL_HEADING_WIRED));
+  load_time_data->SetString(
+      "heading", l10n_util::GetStringUTF16(
+                     is_wifi_connection ? IDS_CAPTIVE_PORTAL_HEADING_WIFI
+                                        : IDS_CAPTIVE_PORTAL_HEADING_WIRED));
 
   if (login_url_.spec() == captive_portal::CaptivePortalDetector::kDefaultURL) {
     // Captive portal may intercept requests without HTTP redirects, in which
     // case the login url would be the same as the captive portal detection url.
     // Don't show the login url in that case.
-    if (is_wifi_connection_) {
-      if (wifi_ssid_.empty())
-        wifi_ssid_ = GetWiFiName();
-      if (wifi_ssid_.empty()) {
+    if (wifi_ssid.empty()) {
       load_time_data->SetString(
           "primaryParagraph",
           l10n_util::GetStringUTF16(
-                IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL_WIFI));
+              is_wifi_connection
+                  ? IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL_WIFI
+                  : IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL_WIRED));
     } else {
       load_time_data->SetString(
           "primaryParagraph",
           l10n_util::GetStringFUTF16(
               IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL_WIFI_SSID,
-                net::EscapeForHTML(base::UTF8ToUTF16(wifi_ssid_))));
+              net::EscapeForHTML(base::UTF8ToUTF16(wifi_ssid))));
     }
   } else {
-      // Non-WiFi connection:
-      load_time_data->SetString(
-          "primaryParagraph",
-          l10n_util::GetStringUTF16(
-              IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL_WIRED));
-    }
-  } else {
-    // Portal redirection was done with HTTP redirects, show the login URL.
+    // Portal redirection was done with HTTP redirects, so show the login URL.
+    // If |languages| is empty, punycode in |login_host| will always be decoded.
     std::string languages;
     Profile* profile = Profile::FromBrowserContext(
         web_contents()->GetBrowserContext());
     if (profile)
       languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
-
     base::string16 login_host = net::IDNToUnicode(login_url_.host(), languages);
     if (base::i18n::IsRTL())
       base::i18n::WrapStringWithLTRFormatting(&login_host);
 
-    if (is_wifi_connection_) {
-      if (wifi_ssid_.empty())
-        wifi_ssid_ = GetWiFiName();
-      if (wifi_ssid_.empty()) {
+    if (wifi_ssid.empty()) {
       load_time_data->SetString(
           "primaryParagraph",
           l10n_util::GetStringFUTF16(
-                IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_WIFI,
+              is_wifi_connection ? IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_WIFI
+                                 : IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_WIRED,
               login_host));
     } else {
       load_time_data->SetString(
           "primaryParagraph",
           l10n_util::GetStringFUTF16(
               IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_WIFI_SSID,
-                net::EscapeForHTML(base::UTF8ToUTF16(wifi_ssid_)),
-                login_host));
-      }
-    } else {
-      // Non-WiFi connection:
-      load_time_data->SetString(
-          "primaryParagraph",
-          l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_WIRED,
-                                     login_host));
+              net::EscapeForHTML(base::UTF8ToUTF16(wifi_ssid)), login_host));
     }
   }
 

chrome/browser/ssl/captive_portal_blocking_page.h

@@ -9,6 +9,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/interstitials/security_interstitial_page.h"
 #include "url/gurl.h"
 
@@ -28,6 +29,16 @@ class CaptivePortalBlockingPage : public SecurityInterstitialPage {
   // Interstitial type, for testing.
   static const void* kTypeForTesting;
 
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Returns true if the connection is a Wi-Fi connection.
+    virtual bool IsWifiConnection() const = 0;
+    // Returns the SSID of the connected Wi-Fi network, if any.
+    virtual std::string GetWiFiSSID() const = 0;
+  };
+
   CaptivePortalBlockingPage(content::WebContents* web_contents,
                             const GURL& request_url,
                             const GURL& login_url,
@@ -37,13 +48,7 @@ class CaptivePortalBlockingPage : public SecurityInterstitialPage {
   // SecurityInterstitialPage method:
   const void* GetTypeForTesting() const override;
 
-  void SetWiFiConnectionForTesting(bool is_wifi_connection) {
-    is_wifi_connection_ = is_wifi_connection;
-  }
-
-  void SetWiFiSSIDForTesting(const std::string& wifi_ssid) {
-    wifi_ssid_ = wifi_ssid;
-  }
+  void SetDelegateForTesting(Delegate* delegate) { delegate_.reset(delegate); }
 
  protected:
   // SecurityInterstitialPage methods:
@@ -57,11 +62,7 @@ class CaptivePortalBlockingPage : public SecurityInterstitialPage {
  private:
   // URL of the login page, opened when the user clicks the "Connect" button.
   GURL login_url_;
-  // True if on a Wi-Fi connection.
-  bool is_wifi_connection_;
-  // SSID of the connected network if the connection is a Wi-Fi connection.
-  std::string wifi_ssid_;
-
+  scoped_ptr<Delegate> delegate_;
   base::Callback<void(bool)> callback_;
 
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPage);

chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc

@@ -64,6 +64,22 @@ enum ExpectLoginURL {
 
 }  // namespace
 
+class FakeConnectionInfoDelegate : public CaptivePortalBlockingPage::Delegate {
+ public:
+  FakeConnectionInfoDelegate(bool is_wifi_connection, std::string wifi_ssid)
+      : is_wifi_connection_(is_wifi_connection), wifi_ssid_(wifi_ssid) {}
+  ~FakeConnectionInfoDelegate() override {}
+
+  bool IsWifiConnection() const override { return is_wifi_connection_; }
+  std::string GetWiFiSSID() const override { return wifi_ssid_; }
+
+ private:
+  const bool is_wifi_connection_;
+  const std::string wifi_ssid_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeConnectionInfoDelegate);
+};
+
 class CaptivePortalBlockingPageTest : public InProcessBrowserTest {
  public:
   CaptivePortalBlockingPageTest() {}
@@ -98,11 +114,13 @@ void CaptivePortalBlockingPageTest::TestInterstitial(
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   DCHECK(contents);
+  // Delegate is owned by the blocking page.
+  FakeConnectionInfoDelegate* delegate =
+      new FakeConnectionInfoDelegate(is_wifi_connection, wifi_ssid);
   // Blocking page is owned by the interstitial.
   CaptivePortalBlockingPage* blocking_page = new CaptivePortalBlockingPage(
       contents, GURL(kBrokenSSL), login_url, base::Callback<void(bool)>());
-  blocking_page->SetWiFiConnectionForTesting(is_wifi_connection);
-  blocking_page->SetWiFiSSIDForTesting(wifi_ssid);
+  blocking_page->SetDelegateForTesting(delegate);
   blocking_page->Show();
 
   WaitForInterstitialAttach(contents);
@@ -140,9 +158,16 @@ void CaptivePortalBlockingPageTest::TestInterstitial(
 // captive portal interstitial should be displayed.
 IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest,
                        WiredNetwork_LoginURL) {
-  TestInterstitial(false, kWiFiSSID, GURL("http://captive.portal/landing_url"),
+  TestInterstitial(false, "", GURL("http://captive.portal/landing_url"),
                    EXPECT_WIFI_NO, EXPECT_WIFI_SSID_NO, EXPECT_LOGIN_URL_YES);
+}
 
+// Same as above, but SSID is available, so the connection should be assumed to
+// be Wi-Fi.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest,
+                       WiredNetwork_LoginURL_With_SSID) {
+  TestInterstitial(false, kWiFiSSID, GURL("http://captive.portal/landing_url"),
+                   EXPECT_WIFI_YES, EXPECT_WIFI_SSID_YES, EXPECT_LOGIN_URL_YES);
 }
 
 // Same as above, expect the login URL is the same as the captive portal ping
@@ -151,8 +176,17 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest,
 IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest,
                        WiredNetwork_NoLoginURL) {
   const GURL kLandingUrl(captive_portal::CaptivePortalDetector::kDefaultURL);
-  TestInterstitial(false, kWiFiSSID, kLandingUrl,
-                   EXPECT_WIFI_NO, EXPECT_WIFI_SSID_NO, EXPECT_LOGIN_URL_NO);
+  TestInterstitial(false, "", kLandingUrl, EXPECT_WIFI_NO, EXPECT_WIFI_SSID_NO,
+                   EXPECT_LOGIN_URL_NO);
+}
+
+// Same as above, but SSID is available, so the connection should be assumed to
+// be Wi-Fi.
+IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageTest,
+                       WiredNetwork_NoLoginURL_With_SSID) {
+  const GURL kLandingUrl(captive_portal::CaptivePortalDetector::kDefaultURL);
+  TestInterstitial(false, kWiFiSSID, kLandingUrl, EXPECT_WIFI_YES,
+                   EXPECT_WIFI_SSID_YES, EXPECT_LOGIN_URL_NO);
 }
 
 // If the connection is a Wi-Fi connection, the Wi-Fi version of the captive
@@ -209,7 +243,6 @@ IN_PROC_BROWSER_TEST_F(CaptivePortalBlockingPageIDNTest,
       base::StringPrintf("http://%s/landing_url", kHostname);
   GURL landing_url(landing_url_spec);
 
-  TestInterstitial(false, kWiFiSSID, landing_url,
-                   EXPECT_WIFI_NO, EXPECT_WIFI_SSID_NO, EXPECT_LOGIN_URL_YES,
-                   kHostnameJSUnicode);
+  TestInterstitial(false, "", landing_url, EXPECT_WIFI_NO, EXPECT_WIFI_SSID_NO,
+                   EXPECT_LOGIN_URL_YES, kHostnameJSUnicode);
 }