Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
861953002:
Add login url to captive portal interstitial. (Closed)
DescriptionAdd login url to captive portal interstitial.
BUG=450428
Committed: https://crrev.com/ca64d98190fba90e62bfbed00fe5e1b0f09d9be4
Cr-Commit-Position: refs/heads/master@{#312934}
Patch Set 1 #Patch Set 2 : Add DCHECK #Patch Set 3 : Add a separate message for when the login url is the same as ping url #
Total comments: 7
Patch Set 4 : Add tests #Patch Set 5 : Minor style fix #
Total comments: 25
Patch Set 6 : mmenke comments #
Total comments: 19
Messages
Total messages: 32 (2 generated)
meacer@chromium.org changed reviewers: + felt@chromium.org, mmenke@chromium.org
mmenke, felt: Can you please take a look? Thanks.
On 2015/01/21 20:42:47, Mustafa Emre Acer wrote: > mmenke, felt: Can you please take a look? Thanks. Problem: If a captive portal intercepts all TCP requests on port 80 and returns its landing page (Instead of a redirect to one), we'd display the probe URL there. Same if it intercepts DNS requests and returns its landing page without a redirect. More generally, captive portals can put any URL they want there...though I suppose captive portals can already do that anyways, so not really a new concern.
On 2015/01/21 20:50:48, mmenke wrote: > On 2015/01/21 20:42:47, Mustafa Emre Acer wrote: > > mmenke, felt: Can you please take a look? Thanks. > > Problem: If a captive portal intercepts all TCP requests on port 80 and returns > its landing page (Instead of a redirect to one), we'd display the probe URL > there. Same if it intercepts DNS requests and returns its landing page without > a redirect. > > More generally, captive portals can put any URL they want there...though I > suppose captive portals can already do that anyways, so not really a new > concern. I added a check to captive_portal_blocking_page.cc for this case. If the landing url is the same as probe url, we just "login page" instead.
I'm extremely paranoid: Maybe tests showing we display the hostname only when we should? And make sure we decode an IDN hostname as well. The errorpage_browsertests have one that you can grab, if you're feeling lazy. https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:90: // login url in that case. nit: Doesn't have to be at the DNS level to run into issues. Maybe "Captive Portal may intercept requests without using HTTP redirects, in which case..." https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 login_host(base::UTF8ToUTF16(login_url_.host())); net::IDNToUnicode https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 login_host(base::UTF8ToUTF16(login_url_.host())); Should we display hostnames at RTL for LTR languages or not? I know for URLs we should, but for hostnames, I have no clue. Looks like localized_error.cc does not, but should consult someone with a clue.
mmenke: Added browser tests for CaptivePortalBlockingPage. Thanks for the IDN pointer in errorpage tests. palmer: In interstitials we've always shown host names LTR, regardless of them being RTL or not. Since you recently had a meeting about RTL URLs, wanted to check with you if this sounds reasonable. https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:90: // login url in that case. On 2015/01/21 21:07:20, mmenke wrote: > nit: Doesn't have to be at the DNS level to run into issues. > > Maybe "Captive Portal may intercept requests without using HTTP redirects, in > which case..." Done. https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 login_host(base::UTF8ToUTF16(login_url_.host())); On 2015/01/21 21:07:20, mmenke wrote: > net::IDNToUnicode Done. https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 login_host(base::UTF8ToUTF16(login_url_.host())); On 2015/01/21 21:07:20, mmenke wrote: > Should we display hostnames at RTL for LTR languages or not? I know for URLs we > should, but for hostnames, I have no clue. Looks like localized_error.cc does > not, but should consult someone with a clue. SecurityInterstitialPage::GetFormattedHostName wraps the host name with LTR formatting, so I'm using the same mechanism here. But CC'ing palmer just in case, since he recently met with people who are trying to solve RTL urls in the omnibox.
https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 login_host(base::UTF8ToUTF16(login_url_.host())); On 2015/01/22 02:47:24, Mustafa Emre Acer wrote: > On 2015/01/21 21:07:20, mmenke wrote: > > Should we display hostnames at RTL for LTR languages or not? I know for URLs > we > > should, but for hostnames, I have no clue. Looks like localized_error.cc does > > not, but should consult someone with a clue. > > SecurityInterstitialPage::GetFormattedHostName wraps the host name with LTR > formatting, so I'm using the same mechanism here. But CC'ing palmer just in > case, since he recently met with people who are trying to solve RTL urls in the > omnibox. You're misunderstanding: This is using whatever formatting the error page itself is using, not LTR. So if it's an RTL error page, I believe we'll display www.google.com backwards, which seems incorrect.
On 2015/01/22 03:18:05, mmenke wrote: > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > File chrome/browser/ssl/captive_portal_blocking_page.cc (right): > > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 > login_host(base::UTF8ToUTF16(login_url_.host())); > On 2015/01/22 02:47:24, Mustafa Emre Acer wrote: > > On 2015/01/21 21:07:20, mmenke wrote: > > > Should we display hostnames at RTL for LTR languages or not? I know for > URLs > > we > > > should, but for hostnames, I have no clue. Looks like localized_error.cc > does > > > not, but should consult someone with a clue. > > > > SecurityInterstitialPage::GetFormattedHostName wraps the host name with LTR > > formatting, so I'm using the same mechanism here. But CC'ing palmer just in > > case, since he recently met with people who are trying to solve RTL urls in > the > > omnibox. > > You're misunderstanding: This is using whatever formatting the error page > itself is using, not LTR. So if it's an RTL error page, I believe we'll display > http://www.google.com backwards, which seems incorrect. I'm not sure that's correct, the code adds LTR marks if the page is RTL. You can test this at chrome://interstitials/ssl?url=https://google.com (launch chrome with LANGUAGE=ar). The host name is displayed as google.com even though the page is RTL. Previously one of the interstitials had a bug similar to what you mentioned, but that issue was fixed after combining SSL and SB under SecurityInterstitialPage. But then I just filed this bug from your comment about IDN: crbug.com/450883
On 2015/01/22 03:44:20, Mustafa Emre Acer wrote: > On 2015/01/22 03:18:05, mmenke wrote: > > > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > > File chrome/browser/ssl/captive_portal_blocking_page.cc (right): > > > > > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > > chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 > > login_host(base::UTF8ToUTF16(login_url_.host())); > > On 2015/01/22 02:47:24, Mustafa Emre Acer wrote: > > > On 2015/01/21 21:07:20, mmenke wrote: > > > > Should we display hostnames at RTL for LTR languages or not? I know for > > URLs > > > we > > > > should, but for hostnames, I have no clue. Looks like localized_error.cc > > does > > > > not, but should consult someone with a clue. > > > > > > SecurityInterstitialPage::GetFormattedHostName wraps the host name with LTR > > > formatting, so I'm using the same mechanism here. But CC'ing palmer just in > > > case, since he recently met with people who are trying to solve RTL urls in > > the > > > omnibox. > > > > You're misunderstanding: This is using whatever formatting the error page > > itself is using, not LTR. So if it's an RTL error page, I believe we'll > display > > http://www.google.com backwards, which seems incorrect. > > > I'm not sure that's correct, the code adds LTR marks if the page is RTL. You can > test this at http://chrome://interstitials/ssl?url=https://google.com (launch chrome > with LANGUAGE=ar). The host name is displayed as http://google.com even though the page > is RTL. > > Previously one of the interstitials had a bug similar to what you mentioned, but > that issue was fixed after combining SSL and SB under SecurityInterstitialPage. > But then I just filed this bug from your comment about IDN: crbug.com/450883 You may well be right. I'm far from an expert on the topic. I don't suppose you know where in the code, where we're adding the LTR order marker?
On 2015/01/22 03:49:41, mmenke wrote: > On 2015/01/22 03:44:20, Mustafa Emre Acer wrote: > > On 2015/01/22 03:18:05, mmenke wrote: > > > > > > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > > > File chrome/browser/ssl/captive_portal_blocking_page.cc (right): > > > > > > > > > https://codereview.chromium.org/861953002/diff/40001/chrome/browser/ssl/capti... > > > chrome/browser/ssl/captive_portal_blocking_page.cc:96: base::string16 > > > login_host(base::UTF8ToUTF16(login_url_.host())); > > > On 2015/01/22 02:47:24, Mustafa Emre Acer wrote: > > > > On 2015/01/21 21:07:20, mmenke wrote: > > > > > Should we display hostnames at RTL for LTR languages or not? I know for > > > URLs > > > > we > > > > > should, but for hostnames, I have no clue. Looks like > localized_error.cc > > > does > > > > > not, but should consult someone with a clue. > > > > > > > > SecurityInterstitialPage::GetFormattedHostName wraps the host name with > LTR > > > > formatting, so I'm using the same mechanism here. But CC'ing palmer just > in > > > > case, since he recently met with people who are trying to solve RTL urls > in > > > the > > > > omnibox. > > > > > > You're misunderstanding: This is using whatever formatting the error page > > > itself is using, not LTR. So if it's an RTL error page, I believe we'll > > display > > > http://www.google.com backwards, which seems incorrect. > > > > > > I'm not sure that's correct, the code adds LTR marks if the page is RTL. You > can > > test this at http://chrome://interstitials/ssl?url=https://google.com (launch > chrome > > with LANGUAGE=ar). The host name is displayed as http://google.com even though > the page > > is RTL. > > > > Previously one of the interstitials had a bug similar to what you mentioned, > but > > that issue was fixed after combining SSL and SB under > SecurityInterstitialPage. > > But then I just filed this bug from your comment about IDN: crbug.com/450883 > > You may well be right. I'm far from an expert on the topic. I don't suppose > you know where in the code, where we're adding the LTR order marker? And to answer my own question, in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/int... (And you added the LTR wrapping code here - sorry, I hadn't looked at the code, and what you said indicated to me that you hadn't added the LTR stuff)
LGTM, just the usual nits. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:108: base::i18n::WrapStringWithLTRFormatting(&login_host); Use 2-space indent https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.h:14: class GURL; This is no longer enough - should include gurl.h instead. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015? Same goes for other files. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:6: include <string> https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:26: const std::string& text) { optional: Maybe DCHECK that text doesn't contain '? (It could contain \', which would work, but checking for that correctly is a pain...Anyhow, if you think it isn't worth doing, feel free to leave it as-is) https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:32: EXPECT_TRUE(content::ExecuteScriptAndExtractBool( Should probably include gtest.h explicitly. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:37: class CaptivePortalBlockingPageTest : public InProcessBrowserTest { I guess there's no IN_PROC_BROWSER_TEST macro (Which wouldn't need this class), just IN_PROC_BROWSER_TEST_F? https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:45: GURL landing_url("http://captive.portal/landing_url"); optional: At least in net, for test constants like this, we generally use (Or try to use, when we think about it) "const GURL kLandingUrl(...);". https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:46: base::Callback<void(bool)> callback; Optional: Could just inline base::Callback<void(bool)>(). https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:46: base::Callback<void(bool)> callback; Should probably also include callback.h https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:50: DCHECK(contents); Include logging.h for DCHECK https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:53: contents, GURL(kBrokenSSL), landing_url, callback); Include gurl.h
https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:108: base::i18n::WrapStringWithLTRFormatting(&login_host); On 2015/01/22 15:21:27, mmenke wrote: > Use 2-space indent Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.h:14: class GURL; On 2015/01/22 15:21:27, mmenke wrote: > This is no longer enough - should include gurl.h instead. Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/22 15:21:27, mmenke wrote: > 2015? Same goes for other files. This is the only new file. I'm not sure I should modify the existing files as well? https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:6: On 2015/01/22 15:21:27, mmenke wrote: > include <string> Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:26: const std::string& text) { On 2015/01/22 15:21:27, mmenke wrote: > optional: Maybe DCHECK that text doesn't contain '? (It could contain \', > which would work, but checking for that correctly is a pain...Anyhow, if you > think it isn't worth doing, feel free to leave it as-is) Just checking for ' for now. Added a comment saying this is mainly used to check for hostnames and a predefined string. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:32: EXPECT_TRUE(content::ExecuteScriptAndExtractBool( On 2015/01/22 15:21:28, mmenke wrote: > Should probably include gtest.h explicitly. Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:37: class CaptivePortalBlockingPageTest : public InProcessBrowserTest { On 2015/01/22 15:21:27, mmenke wrote: > I guess there's no IN_PROC_BROWSER_TEST macro (Which wouldn't need this class), > just IN_PROC_BROWSER_TEST_F? Right, there isn't a IN_PROC_BROWSER_TEST macro. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:45: GURL landing_url("http://captive.portal/landing_url"); On 2015/01/22 15:21:27, mmenke wrote: > optional: At least in net, for test constants like this, we generally use (Or > try to use, when we think about it) "const GURL kLandingUrl(...);". Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:46: base::Callback<void(bool)> callback; On 2015/01/22 15:21:27, mmenke wrote: > Should probably also include callback.h Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:46: base::Callback<void(bool)> callback; On 2015/01/22 15:21:28, mmenke wrote: > Optional: Could just inline base::Callback<void(bool)>(). Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:50: DCHECK(contents); On 2015/01/22 15:21:27, mmenke wrote: > Include logging.h for DCHECK Done. https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:53: contents, GURL(kBrokenSSL), landing_url, callback); On 2015/01/22 15:21:28, mmenke wrote: > Include gurl.h Done.
And still LGTM (Another signoff wasn't needed, but thought there were changes that I'd double-check for mistakes). https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right): https://codereview.chromium.org/861953002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/22 19:07:41, Mustafa Emre Acer wrote: > On 2015/01/22 15:21:27, mmenke wrote: > > 2015? Same goes for other files. > > This is the only new file. I'm not sure I should modify the existing files as > well? You shouldn't. I was just too lazy to check for others when I made the comment. :)
You can see the RTL/LTR discussion here: https://code.google.com/p/chromium/issues/detail?id=351639 Bascially, we think that if the TLD of the domain name is RTL, then we should show the hostname as RTL. Otherwise, we should should show it as LTR. In the case of this error interstitial, I think we should make that decision purely on the endianness of the TLD, regardless of the surrounding context. If I have Chrome set to English but I get an SSL error on an Arabic web page, I'd like to see We found a terrible cyber problem when browsing to MOC.ETIS-LOOC-REPUS.WWW. Bummer! And similarly, !REMMUB. .www.google.com OT GNISWORB NEHW MELBORP... (using all caps as a stand-in for Arabic text since I don't know any Arabic)
On 2015/01/22 19:43:02, Chromium Palmer wrote: > You can see the RTL/LTR discussion here: > > https://code.google.com/p/chromium/issues/detail?id=351639 > > Bascially, we think that if the TLD of the domain name is RTL, then we should > show the hostname as RTL. Otherwise, we should should show it as LTR. > > In the case of this error interstitial, I think we should make that decision > purely on the endianness of the TLD, regardless of the surrounding context. If I > have Chrome set to English but I get an SSL error on an Arabic web page, I'd > like to see > > We found a terrible cyber problem when browsing to MOC.ETIS-LOOC-REPUS.WWW. > Bummer! > > And similarly, > > !REMMUB. .www.google.com OT GNISWORB NEHW MELBORP... > > (using all caps as a stand-in for Arabic text since I don't know any Arabic) That seems completely reasonable to me. I'm not sure we're doing that currently on the error pages, when we display just the hostname (We do for the full URL). I'll check when I have a chance.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:22: #error This file must be built with ENABLE_CAPTIVE_PORTAL_DETECTION flag. presumably this is already true in the build files? (i don't see a change to a build file in this CL?) https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); 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. https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:106: base::string16 login_host = net::IDNToUnicode(login_url_.host(), languages); is it ok that |languages| might never be assigned a value (if there is no profile)?
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:05:25, felt wrote: > 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. 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.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:10:48, mmenke wrote: > On 2015/01/22 20:05:25, felt wrote: > > 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. > > 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/%22, I'd > be fine with that...but that would require server-side changes. Could we link to it even if we don't display the hostname? https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:111: l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH, It looks like this just sets the login page as text. Should it be linked? (Or will the "login" button link to it already?)
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:13:24, felt wrote: > On 2015/01/22 20:10:48, mmenke wrote: > > On 2015/01/22 20:05:25, felt wrote: > > > 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. > > > > 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/%22, > I'd > > be fine with that...but that would require server-side changes. > > Could we link to it even if we don't display the hostname? 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)
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:22: #error This file must be built with ENABLE_CAPTIVE_PORTAL_DETECTION flag. On 2015/01/22 20:05:25, felt wrote: > presumably this is already true in the build files? (i don't see a change to a > build file in this CL?) 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/. https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:16:25, mmenke wrote: > On 2015/01/22 20:13:24, felt wrote: > > On 2015/01/22 20:10:48, mmenke wrote: > > > On 2015/01/22 20:05:25, felt wrote: > > > > 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. > > > > > > 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/%22, > > I'd > > > be fine with that...but that would require server-side changes. > > > > Could we link to it even if we don't display the hostname? > > 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) 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. https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:106: base::string16 login_host = net::IDNToUnicode(login_url_.host(), languages); On 2015/01/22 20:05:25, felt wrote: > is it ok that |languages| might never be assigned a value (if there is no > profile)? If |languages| is empty, net::IDNToUnicode will decode punycode, ending up with the unicode hostname. I think this is fine. https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:111: l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH, On 2015/01/22 20:13:24, felt wrote: > It looks like this just sets the login page as text. Should it be linked? (Or > will the "login" button link to it already?) As I mentioned above, the mocks didn't have the links but if you think it's useful, I can check with Alex.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:111: l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH, On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > On 2015/01/22 20:13:24, felt wrote: > > It looks like this just sets the login page as text. Should it be linked? (Or > > will the "login" button link to it already?) > > As I mentioned above, the mocks didn't have the links but if you think it's > useful, I can check with Alex. And yes, the login button does that already.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > On 2015/01/22 20:16:25, mmenke wrote: > > On 2015/01/22 20:13:24, felt wrote: > > > On 2015/01/22 20:10:48, mmenke wrote: > > > > On 2015/01/22 20:05:25, felt wrote: > > > > > 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. > > > > > > > > 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/%22, > > > I'd > > > > be fine with that...but that would require server-side changes. > > > > > > Could we link to it even if we don't display the hostname? > > > > 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) > > 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. 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.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:26:57, mmenke wrote: > On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > > On 2015/01/22 20:16:25, mmenke wrote: > > > On 2015/01/22 20:13:24, felt wrote: > > > > On 2015/01/22 20:10:48, mmenke wrote: > > > > > On 2015/01/22 20:05:25, felt wrote: > > > > > > 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. > > > > > > > > > > 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/%22, > > > > I'd > > > > > be fine with that...but that would require server-side changes. > > > > > > > > Could we link to it even if we don't display the hostname? > > > > > > 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) > > > > 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. > > 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. 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.
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:54:15, Mustafa Emre Acer wrote: > On 2015/01/22 20:26:57, mmenke wrote: > > On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > > > On 2015/01/22 20:16:25, mmenke wrote: > > > > On 2015/01/22 20:13:24, felt wrote: > > > > > On 2015/01/22 20:10:48, mmenke wrote: > > > > > > On 2015/01/22 20:05:25, felt wrote: > > > > > > > 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. > > > > > > > > > > > > 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/%22, > > > > > I'd > > > > > > be fine with that...but that would require server-side changes. > > > > > > > > > > Could we link to it even if we don't display the hostname? > > > > > > > > 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) > > > > > > 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. > > > > 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. > > 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. (or navigating to an http page for that matter)
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 20:55:12, Mustafa Emre Acer wrote: > On 2015/01/22 20:54:15, Mustafa Emre Acer wrote: > > On 2015/01/22 20:26:57, mmenke wrote: > > > 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. > > > > 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. > > (or navigating to an http page for that matter) 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).
https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:98: IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH_NO_LOGIN_URL)); On 2015/01/22 21:00:50, mmenke wrote: > On 2015/01/22 20:55:12, Mustafa Emre Acer wrote: > > On 2015/01/22 20:54:15, Mustafa Emre Acer wrote: > > > On 2015/01/22 20:26:57, mmenke wrote: > > > > 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. > > > > > > 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. > > > > (or navigating to an http page for that matter) > > 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). 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.
lgtm % the RTL discussion by the way ignore what i said about linkifying: i misunderstood and thought the generic string was being used when there was no CONNECT button, which is wrong, so i did not mean to kick off a big discussion there https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:106: base::string16 login_host = net::IDNToUnicode(login_url_.host(), languages); On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > On 2015/01/22 20:05:25, felt wrote: > > is it ok that |languages| might never be assigned a value (if there is no > > profile)? > > If |languages| is empty, net::IDNToUnicode will decode punycode, ending up with > the unicode hostname. I think this is fine. k https://codereview.chromium.org/861953002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:111: l10n_util::GetStringFUTF16(IDS_CAPTIVE_PORTAL_PRIMARY_PARAGRAPH, On 2015/01/22 20:23:13, Mustafa Emre Acer wrote: > On 2015/01/22 20:22:19, Mustafa Emre Acer wrote: > > On 2015/01/22 20:13:24, felt wrote: > > > It looks like this just sets the login page as text. Should it be linked? > (Or > > > will the "login" button link to it already?) > > > > As I mentioned above, the mocks didn't have the links but if you think it's > > useful, I can check with Alex. > > And yes, the login button does that already. 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
Thanks all! I filed http://crbug.com/451213 to investigate the display of RTL hostnames. I *think* this patch is OK for the time being, since it uses the same mechanism interstitials use to display hostnames. If 451213 finds any problems, it's better to do a sweeping fix.
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/861953002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ca64d98190fba90e62bfbed00fe5e1b0f09d9be4 Cr-Commit-Position: refs/heads/master@{#312934} |
