Add login url to captive portal interstitial.

chrome/browser/ssl/captive_portal_blocking_page.cc

Index: chrome/browser/ssl/captive_portal_blocking_page.cc
diff --git a/chrome/browser/ssl/captive_portal_blocking_page.cc b/chrome/browser/ssl/captive_portal_blocking_page.cc
index 97d6f88c93022db59de6f9e07451e2276bfbc15c..216e6baf6c549a8ff59f9b458809994108df62a1 100644
--- a/chrome/browser/ssl/captive_portal_blocking_page.cc
+++ b/chrome/browser/ssl/captive_portal_blocking_page.cc
@@ -4,16 +4,22 @@
 
 #include "chrome/browser/ssl/captive_portal_blocking_page.h"
 
+#include "base/i18n/rtl.h"
 #include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/captive_portal/captive_portal_detector.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/generated_resources.h"
+#include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
-#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
-#include "chrome/browser/captive_portal/captive_portal_tab_helper.h"
+#if !defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+#error This file must be built with ENABLE_CAPTIVE_PORTAL_DETECTION flag.

[felt, 2015/01/22 20:05:25]

presumably this is already true in the build files? (i don't see a change to a
build file in this CL?)

[meacer, 2015/01/22 20:22:19]

Yes it's already true. Trying to compile without the flag would fail (at worst
in link stage), but I think it's worth explicitly saying why since this file is
under chrome/browser/ssl/.

 #endif
 
 namespace {
@@ -42,12 +48,12 @@ const void* CaptivePortalBlockingPage::kTypeForTesting =
 CaptivePortalBlockingPage::CaptivePortalBlockingPage(
     content::WebContents* web_contents,
     const GURL& request_url,
+    const GURL& login_url,
     const base::Callback<void(bool)>& callback)
     : SecurityInterstitialPage(web_contents, request_url),
+      login_url_(login_url),
       callback_(callback) {
-#if !defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
-  NOTREACHED();
-#endif
+  DCHECK(login_url_.is_valid());
   RecordUMA(SHOW_ALL);
 }
 
@@ -79,12 +85,33 @@ void CaptivePortalBlockingPage::PopulateInterstitialStrings(
       l10n_util::GetStringUTF16(IDS_CAPTIVE_PORTAL_BUTTON_OPEN_LOGIN_PAGE));
   load_time_data->SetString("tabTitle",
       l10n_util::GetStringUTF16(IDS_CAPTIVE_PORTAL_TITLE));
-  load_time_data->SetString(
-      "primaryParagraph",
-      l10n_util::GetStringUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH));
   load_time_data->SetString("heading",
       l10n_util::GetStringUTF16(IDS_CAPTIVE_PORTAL_HEADING));
 
+  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.
+    load_time_data->SetString(
+        "primaryParagraph",
+        l10n_util::GetStringUTF16(
+            IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL));

[felt, 2015/01/22 20:05:25]

In the future, could we handle this case by providing a link to a known HTTP url
(with nothing actually on it, no cookies etc)? That should theoretically trigger
the login page.

[mmenke, 2015/01/22 20:10:48]

We already have one (Namely captive_portal::CaptivePortalDetector::kDefaultURL).
 It seems weird displaying http://www.gstatic.com/generate_204 to people,
though, particularly people who may know that's a Google-owned domain.  If we
had something more interesting, like http://www.captiveportaldomain.com/", I'd
be fine with that...but that would require server-side changes.

[felt, 2015/01/22 20:13:24]

Could we link to it even if we don't display the hostname?

[mmenke, 2015/01/22 20:16:25]

There's a login button elsewhere on the page.  Not exactly a link, it'll open
the login page in a new tab (If we already have a tab there, it will re-focus
that tab...in theory.  Could probably improve the logic there)

[meacer, 2015/01/22 20:22:19]

Well the button works in practive too :) We can linkify the text of course, but
we'll need to check with Alex since his mocks didn't have the link. He might
object on the basis that there would be more than one action on the page.

[mmenke, 2015/01/22 20:26:57]

What I was thinking of is that if the user navigates the login tab manually away
from the login page, I can't remember if we decide it's no longer a login tab or
not, which could be a problem.

Anyhow, that's a bit removed from this CL.

[meacer, 2015/01/22 20:54:15]

Just checked it, we still keep the tab as login tab if the user navigates away
so clicking connect will focus the navigated tab. I'm not sure if this is ideal:
once they navigate away, there is no way to view the actual login page short of
typing the gstatic url.

[meacer, 2015/01/22 20:55:12]

(or navigating to an http page for that matter)

[mmenke, 2015/01/22 21:00:50]

Thanks for investigating!  Yea, if the user navigates away though any means but
a link in the page, we should probably decide it's no longer a login tab.  Even
with that, there's still the issue of other random links on the page, or content
scripts, which could navigate elsewhere.

May be simpler just to decide it's no longer a login tab if it's navigated again
any time after the initial page commits.  Open to other ideas (Note:  I have no
time to work on this myself in the immediate future).

[meacer, 2015/01/22 21:26:57]

Sure, I'm happy to look at this. I realized I tested with google.com which
obviously redirects to the https version. But keeping the tab as the login tab
if it's navigated to an http site actually sounds fine.
Anyways, I'll file a bug where we can discuss ideas.

+  } else {
+    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);

[felt, 2015/01/22 20:05:25]

is it ok that |languages| might never be assigned a value (if there is no
profile)?

[meacer, 2015/01/22 20:22:19]

If |languages| is empty, net::IDNToUnicode will decode punycode, ending up with
the unicode hostname. I think this is fine.

[felt, 2015/01/22 22:51:34]

k

+    if (base::i18n::IsRTL())
+      base::i18n::WrapStringWithLTRFormatting(&login_host);
+    load_time_data->SetString(
+        "primaryParagraph",
+        l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH,

[felt, 2015/01/22 20:13:24]

It looks like this just sets the login page as text. Should it be linked? (Or
will the "login" button link to it already?)

[meacer, 2015/01/22 20:22:19]

As I mentioned above, the mocks didn't have the links but if you think it's
useful, I can check with Alex.

[meacer, 2015/01/22 20:23:13]

And yes, the login button does that already.

[felt, 2015/01/22 22:51:34]

ok then nevermind me i was confused, i thought the generic login string would be
needed in the case where the connect button wasn't doing anything

+                                   login_host));
+  }
+
   // Fill the empty strings to avoid getting debug warnings.
   load_time_data->SetString("openDetails", base::string16());
   load_time_data->SetString("closeDetails", base::string16());
@@ -96,8 +123,6 @@ void CaptivePortalBlockingPage::CommandReceived(const std::string& command) {
   // The response has quotes around it.
   if (command == std::string("\"") + kOpenLoginPageCommand + "\"") {
     RecordUMA(OPEN_LOGIN_PAGE);
-#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
     CaptivePortalTabHelper::OpenLoginTabForWebContents(web_contents(), true);
-#endif
   }
 }