Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
318213002:
Add custom interstitial for captive portals. (Closed)
|
Created:
6 years, 6 months ago by meacer Modified:
5 years, 11 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Target Ref:
refs/pending/heads/master Visibility:
Public. |
DescriptionAdd custom interstitial for captive portals.
This CL adds a new type of interstitial for SSL pages
broken by captive portals. When captive portal detection
is enabled, the new SSLErrorHandler class triggers a captive
portal check and fires a short lived (2 second) one shot timer.
If a captive portal is detected in this window, a custom
captive portal interstitial with "Connect" button is shown. If
a captive portal isn't detected, an SSL interstitial is shown.
Any captive portal result that arrives after the timer expires
is ignored.
Screenshot of the new interstitial: https://goo.gl/cnLIXQ
BUG=384667
TEST=ssl_error_handler_unittest.cc, CaptivePortalBrowserTest.*
Committed: https://crrev.com/4ef065e92f5822c7b0e73ed01433978c0566954f
Cr-Commit-Position: refs/heads/master@{#310697}
Patch Set 1 #Patch Set 2 : Add unit test #Patch Set 3 : WebContentsObserver #Patch Set 4 : Fix build #Patch Set 5 : Fix build #
Total comments: 9
Patch Set 6 : Fix android tests, add more uma, address mmenke's comments #
Total comments: 34
Patch Set 7 : mmenke comments #Patch Set 8 : Const all the things #
Total comments: 40
Patch Set 9 : Address mmenke comments #Patch Set 10 : Remove unnecessary change #
Total comments: 73
Patch Set 11 : mmenke and rsleevi comments #
Total comments: 17
Patch Set 12 : mmenke comments, Simplify error handler #Patch Set 13 : Stop the timer at the right place #
Total comments: 34
Patch Set 14 : mmenke comments, add login scenario to browser tests and fix race. #
Total comments: 30
Patch Set 15 : Expand browser tests #
Total comments: 22
Patch Set 16 : mmenke comments #
Total comments: 54
Patch Set 17 : mmenke comments - add scenario to close the login tab and reopen. #
Total comments: 2
Patch Set 18 : Remove notification, add observer for SSLErrorHandler timer. #
Total comments: 36
Patch Set 19 : bauerb and mmenke comments - make SSLErrorHandler a WebContentsUserData #
Total comments: 10
Patch Set 20 : bauerb comments #
Total comments: 1
Patch Set 21 : Fix the unit test #
Total comments: 6
Patch Set 22 : bauerb comments and rebase #Patch Set 23 : Fix trybot errors (memory leak + unused code) #
Total comments: 25
Patch Set 24 : felt comments #Patch Set 25 : Fix Android builds #Messages
Total messages: 71 (3 generated)
FYI, though not ready for review yet.
mmenke, rsleevi: Could you please take a look? What do you think about the general idea? https://codereview.chromium.org/318213002/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/318213002/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:9336: + <ph name="URL">$1<ex>http://www.google.com/foo/bar</ex></ph> is not secure Strings are subject to change: https://crbug.com/379944
I'll take a look at this tomorrow.
A couple nits.... I'm fine with the idea, but have yet to dig into the implementation details (And doesn't look like I'll have time to do so today, unfortunately). https://codereview.chromium.org/318213002/diff/80001/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_tab_helper.cc:277: } I don't believe this is needed - "AddSelectedTabWithURL" uses "NEW_FOREGROUND_TAB", so no need to activate an already active tab, no? https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:123: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) I don't think we can ever get here if ENABLE_CAPTIVE_PORTAL_DETECTION is false. https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_handler.cc:83: return; Why is this needed? Doesn't look like we're recording histograms, so we're just do nothing, anyways, in that case. https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_handler.cc:90: OnCaptivePortalResult(); Think it's a little weird that we just always wait for the timer if no captive portal is enabled.
https://codereview.chromium.org/318213002/diff/80001/chrome/browser/captive_p... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/captive_p... chrome/browser/captive_portal/captive_portal_tab_helper.cc:277: } On 2014/06/17 19:17:03, mmenke wrote: > I don't believe this is needed - "AddSelectedTabWithURL" uses > "NEW_FOREGROUND_TAB", so no need to activate an already active tab, no? Correct. https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/capti... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/capti... chrome/browser/ssl/captive_portal_blocking_page.cc:123: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) On 2014/06/17 19:17:03, mmenke wrote: > I don't think we can ever get here if ENABLE_CAPTIVE_PORTAL_DETECTION is false. Correct, but the compilation will fail when ENABLE_CAPTIVE_PORTAL_DETECTION is false. https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_handler.cc:83: return; On 2014/06/17 19:17:03, mmenke wrote: > Why is this needed? Doesn't look like we're recording histograms, so we're just > do nothing, anyways, in that case. Done. https://codereview.chromium.org/318213002/diff/80001/chrome/browser/ssl/ssl_e... chrome/browser/ssl/ssl_error_handler.cc:90: OnCaptivePortalResult(); On 2014/06/17 19:17:03, mmenke wrote: > Think it's a little weird that we just always wait for the timer if no captive > portal is enabled. If portal detection isn't enabled, we immediately show the ssl interstitial without firing a timer, and Observe wouldn't get called at all since we won't listen for any notifications.
Should probably have some browser tests for this. Also, should make sure this doesn't affect any of the SSL tests - I believe we currently disable captive portal detection for tests, and let individual tests enable it, so I think we're fine, but should double-check that. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.h:112: void OpenLoginTab(bool focus); optional: Could make this something like: static void OpenLoginTabForWebContents(WebContents* contents, bool focus); Then consumers don't have to muck with the factory. This function doesn't even need the CaptivePortalTabHelper for the requesting tab. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... File chrome/browser/resources/ssl/captive_portal.js (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.js:16: sendCommand(CMD_OPEN_LOGIN_PAGE); nit: Believe this should use a 2-space indent. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.js:26: sendCommand(CMD_MORE); nit: Should be consistent - use more everywhere, or use details everywhere. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:91: : web_contents_(web_contents), nit: +2 indent. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:123: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) Think it makes more sense just to exclude this file if ENABLE_CAPTIVE_PORTAL_DETECTION is false, unless you have some other plan for that case. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:8: #include <string> Blank line after C++ headers. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:8: #include <string> include macros.h for OVERRIDE and DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:12: namespace content{ nit: Space before "{" https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:23: // UI thread. More fundamentally, it's not safe to muck with a WebContents on any other thread. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:125: handled_ = true; We just keep the handler around until some random load stops, at this point? https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:150: void SSLErrorHandler::DidStopLoading( Should also destroy the handler on a new load. Not sure if we can distinguish between user-initiated ones and content-initiated ones, but cancelling on both may be reasonable. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:150: void SSLErrorHandler::DidStopLoading( What if the user presses the stop button? https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:152: timer_.Stop(); Not need to stop the timer. Destroying ourselves will destroy the timer, which will stop it. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:8: #include <string> nit: Line break after standard includes. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:10: #include "base/memory/weak_ptr.h" include macros.h https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:51: } nit: bool handled() const { return handled_; } https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:79: const GURL& request_url_; Who owns these? Seems like we should keep out own copies.
Oh, and I'm sorry about being so slow to get to this.
Thanks Matt. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.h:112: void OpenLoginTab(bool focus); On 2014/06/20 16:07:08, mmenke wrote: > optional: Could make this something like: > > static void OpenLoginTabForWebContents(WebContents* contents, bool focus); > > Then consumers don't have to muck with the factory. This function doesn't even > need the CaptivePortalTabHelper for the requesting tab. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... File chrome/browser/resources/ssl/captive_portal.js (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.js:16: sendCommand(CMD_OPEN_LOGIN_PAGE); On 2014/06/20 16:07:08, mmenke wrote: > nit: Believe this should use a 2-space indent. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.js:26: sendCommand(CMD_MORE); On 2014/06/20 16:07:08, mmenke wrote: > nit: Should be consistent - use more everywhere, or use details everywhere. Renamed to SHOW_DETAILS. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:91: : web_contents_(web_contents), On 2014/06/20 16:07:08, mmenke wrote: > nit: +2 indent. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:123: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) On 2014/06/20 16:07:08, mmenke wrote: > Think it makes more sense just to exclude this file if > ENABLE_CAPTIVE_PORTAL_DETECTION is false, unless you have some other plan for > that case. Hmm, I was thinking if we enable portal pings on Windows 8 and on Android/iOS, we'd still need this page to show a non-security warning. So in that case ENABLE_CAPTIVE_PORTAL_DETECTION will always be true, while we'll defer to the OS for login handling. Also if the whole file is excluded, it makes sense to exclude SSLErrorHandler as well, but that file records some UMA when ENABLE_CAPTIVE_PORTAL_DETECTION is false and we'll lose that (or I'll have to push it to chrome_content_browser_client). We also won't need unittests for the disabled case. I have a patch for this if you prefer doing this. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:8: #include <string> On 2014/06/20 16:07:09, mmenke wrote: > include macros.h for OVERRIDE and DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:12: namespace content{ On 2014/06/20 16:07:08, mmenke wrote: > nit: Space before "{" Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:23: // UI thread. On 2014/06/20 16:07:08, mmenke wrote: > More fundamentally, it's not safe to muck with a WebContents on any other > thread. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:125: handled_ = true; On 2014/06/20 16:07:09, mmenke wrote: > We just keep the handler around until some random load stops, at this point? Once we show the captive portal interstitial, the handler will be deleted. handled_=true is mostly for the tests where ShowCaptivePortalInterstitial and ShowSSLInterstitial are mocked, so that we can check that we handled once any of those methods are called. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:150: void SSLErrorHandler::DidStopLoading( On 2014/06/20 16:07:09, mmenke wrote: > Should also destroy the handler on a new load. Not sure if we can distinguish > between user-initiated ones and content-initiated ones, but cancelling on both > may be reasonable. In which case do we get a new load notification on this page? If a new navigation navigation starts while we are waiting for a result, we already get a DidStopLoading. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:150: void SSLErrorHandler::DidStopLoading( On 2014/06/20 16:07:09, mmenke wrote: > What if the user presses the stop button? In that case we'll get a DidStopLoading and delete the handler. The captive portal result will be ignored (ie. no login tab will be opened). https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:152: timer_.Stop(); On 2014/06/20 16:07:09, mmenke wrote: > Not need to stop the timer. Destroying ourselves will destroy the timer, which > will stop it. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:8: #include <string> On 2014/06/20 16:07:09, mmenke wrote: > nit: Line break after standard includes. Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:10: #include "base/memory/weak_ptr.h" On 2014/06/20 16:07:09, mmenke wrote: > include macros.h Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:51: } On 2014/06/20 16:07:09, mmenke wrote: > nit: bool handled() const { return handled_; } Done. https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:79: const GURL& request_url_; On 2014/06/20 16:07:09, mmenke wrote: > Who owns these? Seems like we should keep out own copies. ssl_info is owned by net layer. We can certainly keep a copy of request_url.
https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:79: const GURL& request_url_; On 2014/06/20 23:28:15, Mustafa Emre Acer wrote: > On 2014/06/20 16:07:09, mmenke wrote: > > Who owns these? Seems like we should keep out own copies. > > ssl_info is owned by net layer. We can certainly keep a copy of request_url. The net layer lives on another thread... It looks like it's actually owned by the SSLCertErrorHandler. It looks to me like these are fairly short lived as well.
On 2014/06/20 23:41:03, mmenke wrote: > https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler.h (right): > > https://codereview.chromium.org/318213002/diff/100001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler.h:79: const GURL& request_url_; > On 2014/06/20 23:28:15, Mustafa Emre Acer wrote: > > On 2014/06/20 16:07:09, mmenke wrote: > > > Who owns these? Seems like we should keep out own copies. > > > > ssl_info is owned by net layer. We can certainly keep a copy of request_url. > > The net layer lives on another thread... It looks like it's actually owned by > the SSLCertErrorHandler. It looks to me like these are fairly short lived as > well. Oh...It's actually owned by our callback. Relying on that just seems like something bound to regress at some point.
> Oh...It's actually owned by our callback. Relying on that just seems like > something bound to regress at some point Right, we are copying now.
Think we should have a couple browser tests for this. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:270: content::WebContents* contents = chrome::AddSelectedTabWithURL( nit: new_contents or login_web_contents or login_tab, maybe? https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... File chrome/browser/resources/ssl/captive_portal.html (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.html:3: <head> I wonder if we can just reuse interstitial_v2.html, hiding the elements we don't use? May even be able to share the C++ files, just use different strings and adding different functions. Or could make them share a parent class or something. My concern is just minimizing redundant HTML, and making updating neterror.css less of a pain. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:17: #include "grit/browser_resources.h" I don't think this is used. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:19: #include "net/base/net_errors.h" I don't think this is used. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:120: int cmd = atoi(command.c_str()); Need the header for atoi. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: New files shouldn't use the (c) https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:21: // It deletes itself when the interstitial page is closed.. nit: Remove extra period https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:31: protected: optional: These can all be private instead. Or public. Think it's cleanest to only have a protected section when we need it. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:40: // The interstitial page owns us. nit: Don't use "us" in comments, as it can be ambiguous. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:120: if (!timer_.IsRunning()) { Could theoretically get results from captive portal probes initiated by other objects. Not likely, due to exponential backoff, but it's still theoretically possible. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:121: RecordUMA(CAPTIVE_PORTAL_RESULT_BEFORE_TIMER); Shouldn't this be CAPTIVE_PORTAL_RESULT_AFTER_TIMER? I think it makes more sense to record the UMA in OnTimerExpired than here. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:135: RecordUMA(SSL_WARNING_ALREADY_HANDLED); Is this UMA useful? Seems like we don't care if the handler was destroyed before the time expires or not, when we've already handled the result. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: --(c) https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:47: virtual void OnTimerExpired(); Suggest moving these down to the protected section, and having the test fixture expose them. Also, I don't believe they need to be virtual. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:55: base::OneShotTimer<SSLErrorHandler> timer_; Rather than making timer_ protected (Which Google style guide technically forbids), suggest making it private, and making a protected method IsTimerRunning. Then just have the test fixture expose that. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: Remove "(c)" https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:14: nit: Remove extre blank line. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:32: void set_ssl_interstitial_display_delay(base::TimeDelta delay) { Is this method needed? Seems like you can just call it in the constructor with a delay of 0 seconds. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:44: private: nit: blank line before private section. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:71: scoped_ptr<net::SSLInfo> ssl_info_; Does this need to be a scoped_ptr? https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:78: ShouldShowSSLInterstitialOnTimerExpired) { I suggest not using GMOCK. I think just adding bools and using EXPECT_EQ is much clearer, and makes for tighter tests. Can also add EXPECT_FALSE calls when setting the bools to true, to protect against all unexpected calls. "EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);" means that it expects a call from any point after that call until the next ShowSSLInterstitial expect call, which is non-obvious, and can lead to overlooked bugs. Also, some people are opposed to using GMOCK in general, since it increases the amount of knowledge needed to hack on Chrome. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:118: EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0); These should be before the OnCaptivePortalResult call. Otherwise, the test passes if ShowCaptivePortalInterstitial() is only called as a result of the CaptivePortalResult (unlikely, but possible).
Hi Matt, Sorry for getting back to this so late. The UX has been changed a couple of times after reviews but this part remains the same. I've addressed most of your comments. Can you please take a look? I'll be sending followups CLs as well. Thanks! https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:270: content::WebContents* contents = chrome::AddSelectedTabWithURL( On 2014/06/24 18:09:26, mmenke wrote: > nit: new_contents or login_web_contents or login_tab, maybe? Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... File chrome/browser/resources/ssl/captive_portal.html (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... chrome/browser/resources/ssl/captive_portal.html:3: <head> On 2014/06/24 18:09:26, mmenke wrote: > I wonder if we can just reuse interstitial_v2.html, hiding the elements we don't > use? May even be able to share the C++ files, just use different strings and > adding different functions. Or could make them share a parent class or > something. > > My concern is just minimizing redundant HTML, and making updating neterror.css > less of a pain. Done. I refactored security interstitials to share some code in https://codereview.chromium.org/622683006/. This CL depends on that, and all interstitials use the same template now. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:17: #include "grit/browser_resources.h" On 2014/06/24 18:09:26, mmenke wrote: > I don't think this is used. Not referred from this class anymore. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:19: #include "net/base/net_errors.h" On 2014/06/24 18:09:26, mmenke wrote: > I don't think this is used. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:120: int cmd = atoi(command.c_str()); On 2014/06/24 18:09:26, mmenke wrote: > Need the header for atoi. Code not used anymore. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/24 18:09:26, mmenke wrote: > nit: New files shouldn't use the (c) Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:21: // It deletes itself when the interstitial page is closed.. On 2014/06/24 18:09:26, mmenke wrote: > nit: Remove extra period Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:31: protected: On 2014/06/24 18:09:26, mmenke wrote: > optional: These can all be private instead. Or public. Think it's cleanest to > only have a protected section when we need it. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:40: // The interstitial page owns us. On 2014/06/24 18:09:26, mmenke wrote: > nit: Don't use "us" in comments, as it can be ambiguous. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:135: RecordUMA(SSL_WARNING_ALREADY_HANDLED); On 2014/06/24 18:09:27, mmenke wrote: > Is this UMA useful? Seems like we don't care if the handler was destroyed > before the time expires or not, when we've already handled the result. Removed. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:55: base::OneShotTimer<SSLErrorHandler> timer_; On 2014/06/24 18:09:27, mmenke wrote: > Rather than making timer_ protected (Which Google style guide technically > forbids), suggest making it private, and making a protected method > IsTimerRunning. Then just have the test fixture expose that. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/24 18:09:27, mmenke wrote: > nit: Remove "(c)" Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:14: On 2014/06/24 18:09:27, mmenke wrote: > nit: Remove extre blank line. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:32: void set_ssl_interstitial_display_delay(base::TimeDelta delay) { On 2014/06/24 18:09:27, mmenke wrote: > Is this method needed? Seems like you can just call it in the constructor with > a delay of 0 seconds. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:44: private: On 2014/06/24 18:09:27, mmenke wrote: > nit: blank line before private section. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:71: scoped_ptr<net::SSLInfo> ssl_info_; On 2014/06/24 18:09:27, mmenke wrote: > Does this need to be a scoped_ptr? Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:78: ShouldShowSSLInterstitialOnTimerExpired) { On 2014/06/24 18:09:27, mmenke wrote: > I suggest not using GMOCK. I think just adding bools and using EXPECT_EQ is > much clearer, and makes for tighter tests. Can also add EXPECT_FALSE calls when > setting the bools to true, to protect against all unexpected calls. > > "EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);" means that it > expects a call from any point after that call until the next ShowSSLInterstitial > expect call, which is non-obvious, and can lead to overlooked bugs. > > Also, some people are opposed to using GMOCK in general, since it increases the > amount of knowledge needed to hack on Chrome. Done. https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:118: EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0); On 2014/06/24 18:09:27, mmenke wrote: > These should be before the OnCaptivePortalResult call. Otherwise, the test > passes if ShowCaptivePortalInterstitial() is only called as a result of the > CaptivePortalResult (unlikely, but possible). Done.
Sorry, forgot about this one - I'll get back to it on Monday. On 2014/10/22 23:04:30, Mustafa Emre Acer wrote: > Hi Matt, > > Sorry for getting back to this so late. The UX has been changed a couple of > times after reviews but this part remains the same. I've addressed most of your > comments. Can you please take a look? > > I'll be sending followups CLs as well. > > Thanks! > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... > File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/captive_... > chrome/browser/captive_portal/captive_portal_tab_helper.cc:270: > content::WebContents* contents = chrome::AddSelectedTabWithURL( > On 2014/06/24 18:09:26, mmenke wrote: > > nit: new_contents or login_web_contents or login_tab, maybe? > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... > File chrome/browser/resources/ssl/captive_portal.html (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/resource... > chrome/browser/resources/ssl/captive_portal.html:3: <head> > On 2014/06/24 18:09:26, mmenke wrote: > > I wonder if we can just reuse interstitial_v2.html, hiding the elements we > don't > > use? May even be able to share the C++ files, just use different strings and > > adding different functions. Or could make them share a parent class or > > something. > > > > My concern is just minimizing redundant HTML, and making updating neterror.css > > less of a pain. > > Done. I refactored security interstitials to share some code in > https://codereview.chromium.org/622683006/. This CL depends on that, and all > interstitials use the same template now. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > File chrome/browser/ssl/captive_portal_blocking_page.cc (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.cc:17: #include > "grit/browser_resources.h" > On 2014/06/24 18:09:26, mmenke wrote: > > I don't think this is used. > > Not referred from this class anymore. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.cc:19: #include > "net/base/net_errors.h" > On 2014/06/24 18:09:26, mmenke wrote: > > I don't think this is used. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.cc:120: int cmd = > atoi(command.c_str()); > On 2014/06/24 18:09:26, mmenke wrote: > > Need the header for atoi. > > Code not used anymore. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > File chrome/browser/ssl/captive_portal_blocking_page.h (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:1: // Copyright (c) 2014 The > Chromium Authors. All rights reserved. > On 2014/06/24 18:09:26, mmenke wrote: > > nit: New files shouldn't use the (c) > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:21: // It deletes itself when > the interstitial page is closed.. > On 2014/06/24 18:09:26, mmenke wrote: > > nit: Remove extra period > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:31: protected: > On 2014/06/24 18:09:26, mmenke wrote: > > optional: These can all be private instead. Or public. Think it's cleanest > to > > only have a protected section when we need it. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:40: // The interstitial page > owns us. > On 2014/06/24 18:09:26, mmenke wrote: > > nit: Don't use "us" in comments, as it can be ambiguous. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler.cc (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler.cc:135: > RecordUMA(SSL_WARNING_ALREADY_HANDLED); > On 2014/06/24 18:09:27, mmenke wrote: > > Is this UMA useful? Seems like we don't care if the handler was destroyed > > before the time expires or not, when we've already handled the result. > > Removed. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler.h (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler.h:55: base::OneShotTimer<SSLErrorHandler> > timer_; > On 2014/06/24 18:09:27, mmenke wrote: > > Rather than making timer_ protected (Which Google style guide technically > > forbids), suggest making it private, and making a protected method > > IsTimerRunning. Then just have the test fixture expose that. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:1: // Copyright (c) 2014 The > Chromium Authors. All rights reserved. > On 2014/06/24 18:09:27, mmenke wrote: > > nit: Remove "(c)" > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:14: > On 2014/06/24 18:09:27, mmenke wrote: > > nit: Remove extre blank line. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:32: void > set_ssl_interstitial_display_delay(base::TimeDelta delay) { > On 2014/06/24 18:09:27, mmenke wrote: > > Is this method needed? Seems like you can just call it in the constructor > with > > a delay of 0 seconds. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:44: private: > On 2014/06/24 18:09:27, mmenke wrote: > > nit: blank line before private section. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:71: scoped_ptr<net::SSLInfo> > ssl_info_; > On 2014/06/24 18:09:27, mmenke wrote: > > Does this need to be a scoped_ptr? > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:78: > ShouldShowSSLInterstitialOnTimerExpired) { > On 2014/06/24 18:09:27, mmenke wrote: > > I suggest not using GMOCK. I think just adding bools and using EXPECT_EQ is > > much clearer, and makes for tighter tests. Can also add EXPECT_FALSE calls > when > > setting the bools to true, to protect against all unexpected calls. > > > > "EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);" means that it > > expects a call from any point after that call until the next > ShowSSLInterstitial > > expect call, which is non-obvious, and can lead to overlooked bugs. > > > > Also, some people are opposed to using GMOCK in general, since it increases > the > > amount of knowledge needed to hack on Chrome. > > Done. > > https://codereview.chromium.org/318213002/diff/140001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:118: > EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0); > On 2014/06/24 18:09:27, mmenke wrote: > > These should be before the OnCaptivePortalResult call. Otherwise, the test > > passes if ShowCaptivePortalInterstitial() is only called as a result of the > > CaptivePortalResult (unlikely, but possible). > > Done.
And forgot about it again, because it had pending comments. Sorry about that (again).
On 2014/10/29 22:19:33, mmenke wrote: > And forgot about it again, because it had pending comments. Sorry about that > (again). No worries :)
Deferring this to matt, as this seems mostly his ken. If there's anything specific I should peep, let me know. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:11: #include "url/gurl.h" You can forward declare GURL here. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:18: // uses captive_portal::CaptivePortalService which can only be accessed on the "CaptivePortalService which " -> "CaptivePortalService, which" https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: nit: no newline. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: bool handled() const { was_handled_for_testing then? Are all of these for tests? If so, does it make more sense to private + friend? https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:52: } These aren't virtual methods you're overriding, so you're instead shadowing (which is not ideal). Your order of declarations also doesn't match that in the SSLErrorHandler https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:56: DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandler); no newline https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: #ifdef ENABLE_CAPTIVE_PORTAL_DETECTION #if defined(...)
https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2256: // tab. The user then logs in and the page with the error is reloaded. The last sentence is false. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2256: // tab. The user then logs in and the page with the error is reloaded. I suggest putting more basic tests before ones that depend on them - that way, when running tests in order, the simplest, most useful failure is generally the first one. So suggest putting this just above the SSLCertErrorLogin test. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:57: base::TimeDelta::FromSeconds(kDefaultSSLInterstitialDisplayDelay)), I don't think this value makes any sense here - the class doesn't even use it for anything. Does the browser test have issues if it's non-zero? https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:256: void CaptivePortalTabHelper::OpenLoginTabForWebContents( Definition order should match declaration order. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1746: #endif I think this code makes much more sense in SSLErrorHandler, than in a 2600-line catch-all class. If this is because you don't want tests to depend on CaptivePortalTabHelper, there are other options. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1748: SSLErrorHandler* ssl_error_handler = new SSLErrorHandler( Suggest making this a static function, since nothing else event needs to touch the handler. SSLErrorHandler::HandleSSLError(blah); or SSLErrorHandler::ShowInterstatial(blah); https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1752: callback); nit: Should either fit args in as few lines as possible, or put one on each line, unless you have some particular reason for not doing so. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:47: const { nit: const should go on same line as close paren, so I suggest: SecurityInterstitialPage::Type CaptivePortalBlockingPage::GetTypeForTesting() const { https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:83: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) Can we just not include this file when it's not enabled? I seem to recall this being discussed before, but I could be misremembering. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:24: virtual ~CaptivePortalBlockingPage(); -virtual, +override https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:36: virtual void CommandReceived(const std::string& command) override; -virtual(x4) https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:40: DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPage); include base/macros.h for DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:22: namespace { nit: blank line after namespace starts https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:29: CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE, nit: TRIGGERED / EXPIRED https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:79: bool SSLErrorHandler::overridable() const { nit: Should either change naming style, or inline this method. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:79: bool SSLErrorHandler::overridable() const { Definition order should match declaration order https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:81: !(options_mask_ & SSLBlockingPage::STRICT_ENFORCEMENT); I have no idea what these enum values mean, and they're not documented. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:103: SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK); What does this get us? ENABLE_CAPTIVE_PORTAL_DETECTION is defined on a per-platform basis, and IsCaptivePortalInterstitialEnabled varies based on build-time flags. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:137: OnCaptivePortalResult(); If we don't see a captive portal, and handled_ is false, shouldn't we just call ShowSSLInterstitial()? https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:142: void SSLErrorHandler::OnCaptivePortalResult() { "CaptivePortalResult" is a very confusing name here, and for the histogram enum. It actually means "I have a CaptivePortalService::Results, and that indicated a captive portal". I suggest inlining this function, since it's clearer when inlined (For tests, send a bogus notification instead - that'll exercise the logic in SSLErrorHandler::Observe as well), and renaming CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE to CAPTIVE_PORTAL_DETECTED_AFTER_TIMER_EXPIRE. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:148: if (handled_) { Is there any case where handled_ != timer_.IsRunning() here? https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:167: captive_portal_interstitial_show_count_++; If you want to go this route for testing, I think it makes more sense to make ShowCaptivePortalInterstitial / ShowSSLInterstitial virtual, and have the derived class deal with the counts. Then you can get rid of create_interstitials_, too. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:28: SSLErrorHandler::DontCreateInterstitials(); Any reason to prefer this over InterstitialPage::DontCreateViewForTesting();? https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:61: virtual void SetUp() override { The new coolness is to use override without virtual (Goes for all files) https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:165: #else #else // if !defined(...) https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:176: #endif #endif // defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:86: RecordUMA(SSL_WARNING_ALREADY_HANDLED); Also, this doesn't get anything. Should just DCHECK(!handled_) and get rid of the histogram value. In general, we shouldn't have histograms checking for every obscure, unlikely regression.
https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2256: // tab. The user then logs in and the page with the error is reloaded. On 2014/10/30 19:28:01, mmenke wrote: > The last sentence is false. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:57: base::TimeDelta::FromSeconds(kDefaultSSLInterstitialDisplayDelay)), On 2014/10/30 19:28:01, mmenke wrote: > I don't think this value makes any sense here - the class doesn't even use it > for anything. Does the browser test have issues if it's non-zero? Yes, for example SSLCertErrorLogin will fail if it's non zero. I agree it doesn't look like it belongs here, but then the only alternative I can think of is to introduce global state somewhere in SSLErrorHandler which doesn't sound better. I'm happy to consider any other suggestions. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:256: void CaptivePortalTabHelper::OpenLoginTabForWebContents( On 2014/10/30 19:28:01, mmenke wrote: > Definition order should match declaration order. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1746: #endif On 2014/10/30 19:28:01, mmenke wrote: > I think this code makes much more sense in SSLErrorHandler, than in a 2600-line > catch-all class. > > If this is because you don't want tests to depend on CaptivePortalTabHelper, > there are other options. Moved to SSLErrorHandler. The tests depend on CaptivePortalTabHelper to be able to set the delay value. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1748: SSLErrorHandler* ssl_error_handler = new SSLErrorHandler( On 2014/10/30 19:28:01, mmenke wrote: > Suggest making this a static function, since nothing else event needs to touch > the handler. > > SSLErrorHandler::HandleSSLError(blah); or > SSLErrorHandler::ShowInterstatial(blah); Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:1752: callback); On 2014/10/30 19:28:01, mmenke wrote: > nit: Should either fit args in as few lines as possible, or put one on each > line, unless you have some particular reason for not doing so. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:47: const { On 2014/10/30 19:28:01, mmenke wrote: > nit: const should go on same line as close paren, so I suggest: > > SecurityInterstitialPage::Type > CaptivePortalBlockingPage::GetTypeForTesting() const { Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:11: #include "url/gurl.h" On 2014/10/29 23:17:32, Ryan Sleevi wrote: > You can forward declare GURL here. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:18: // uses captive_portal::CaptivePortalService which can only be accessed on the On 2014/10/29 23:17:32, Ryan Sleevi wrote: > "CaptivePortalService which " -> "CaptivePortalService, which" Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:24: virtual ~CaptivePortalBlockingPage(); On 2014/10/30 19:28:02, mmenke wrote: > -virtual, +override Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:36: virtual void CommandReceived(const std::string& command) override; On 2014/10/30 19:28:02, mmenke wrote: > -virtual(x4) Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: On 2014/10/29 23:17:32, Ryan Sleevi wrote: > nit: no newline. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:40: DISALLOW_COPY_AND_ASSIGN(CaptivePortalBlockingPage); On 2014/10/30 19:28:02, mmenke wrote: > include base/macros.h for DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:22: namespace { On 2014/10/30 19:28:02, mmenke wrote: > nit: blank line after namespace starts Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:29: CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE, On 2014/10/30 19:28:02, mmenke wrote: > nit: TRIGGERED / EXPIRED Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:79: bool SSLErrorHandler::overridable() const { On 2014/10/30 19:28:02, mmenke wrote: > nit: Should either change naming style, or inline this method. Moved to SSLBlockingPage. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:81: !(options_mask_ & SSLBlockingPage::STRICT_ENFORCEMENT); On 2014/10/30 19:28:02, mmenke wrote: > I have no idea what these enum values mean, and they're not documented. Added descriptions in ssl_blocking_page.h https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:86: RecordUMA(SSL_WARNING_ALREADY_HANDLED); On 2014/10/30 19:33:44, mmenke wrote: > Also, this doesn't get anything. > > Should just DCHECK(!handled_) and get rid of the histogram value. In general, > we shouldn't have histograms checking for every obscure, unlikely regression. OnCaptivePortalResult might get calledbefore Handle() is called (e.g. the portal check was triggered by another SSL error), so it's possible that handled_ is sometimes true, although the window is very small. I removed the histogram entry though. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:103: SHOW_SSL_INTERSTITIAL_WITHOUT_CAPTIVE_PORTAL_CHECK); On 2014/10/30 19:28:02, mmenke wrote: > What does this get us? ENABLE_CAPTIVE_PORTAL_DETECTION is defined on a > per-platform basis, and IsCaptivePortalInterstitialEnabled varies based on > build-time flags. Removed. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:137: OnCaptivePortalResult(); On 2014/10/30 19:28:02, mmenke wrote: > If we don't see a captive portal, and handled_ is false, shouldn't we just call > ShowSSLInterstitial()? Hmm, isn't it possible that we could see a captive portal in a second notification before the timer expires? I guess it's a very edge case given the exponential backoff behavior though. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:142: void SSLErrorHandler::OnCaptivePortalResult() { On 2014/10/30 19:28:02, mmenke wrote: > "CaptivePortalResult" is a very confusing name here, and for the histogram enum. > It actually means "I have a CaptivePortalService::Results, and that indicated a > captive portal". > > I suggest inlining this function, since it's clearer when inlined (For tests, > send a bogus notification instead - that'll exercise the logic in > SSLErrorHandler::Observe as well), and renaming > CAPTIVE_PORTAL_RESULT_AFTER_TIMER_EXPIRE to > CAPTIVE_PORTAL_DETECTED_AFTER_TIMER_EXPIRE. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:148: if (handled_) { On 2014/10/30 19:28:02, mmenke wrote: > Is there any case where handled_ != timer_.IsRunning() here? That would be the general case where timer is running and a captive portal is detected before timer expires, right? 1. Handle() -> start timer. IsRunning = true, handled_ = false 2. OnCaptivePortalResult -> IsRunning = true, handled_ = false https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:167: captive_portal_interstitial_show_count_++; On 2014/10/30 19:28:02, mmenke wrote: > If you want to go this route for testing, I think it makes more sense to make > ShowCaptivePortalInterstitial / ShowSSLInterstitial virtual, and have the > derived class deal with the counts. Then you can get rid of > create_interstitials_, too. Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: bool handled() const { On 2014/10/29 23:17:32, Ryan Sleevi wrote: > was_handled_for_testing then? > > Are all of these for tests? If so, does it make more sense to private + friend? Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:28: SSLErrorHandler::DontCreateInterstitials(); On 2014/10/30 19:28:02, mmenke wrote: > Any reason to prefer this over InterstitialPage::DontCreateViewForTesting();? Not needed anymore, removed. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:52: } On 2014/10/29 23:17:32, Ryan Sleevi (ooo until 11-4) wrote: > These aren't virtual methods you're overriding, so you're instead shadowing > (which is not ideal). > > Your order of declarations also doesn't match that in the SSLErrorHandler They are not overrides anymore since I moved the counts into this class from SSLErrorHandler. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:56: DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandler); On 2014/10/29 23:17:32, Ryan Sleevi wrote: > no newline Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:61: virtual void SetUp() override { On 2014/10/30 19:28:02, mmenke wrote: > The new coolness is to use override without virtual (Goes for all files) Done. That change happened long after I wrote this test so I missed it. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: #ifdef ENABLE_CAPTIVE_PORTAL_DETECTION On 2014/10/29 23:17:32, Ryan Sleevi wrote: > #if defined(...) Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:165: #else On 2014/10/30 19:28:02, mmenke wrote: > #else // if !defined(...) Done. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:176: #endif On 2014/10/30 19:28:02, mmenke wrote: > #endif // defined(ENABLE_CAPTIVE_PORTAL_DETECTION) Done.
Quick responses, and a new comment. Hope to do another pass, and think about tests, tomorrow, but may not get back to it until Monday. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.cc:57: base::TimeDelta::FromSeconds(kDefaultSSLInterstitialDisplayDelay)), On 2014/11/06 21:21:55, Mustafa Emre Acer wrote: > On 2014/10/30 19:28:01, mmenke wrote: > > I don't think this value makes any sense here - the class doesn't even use it > > for anything. Does the browser test have issues if it's non-zero? > > Yes, for example SSLCertErrorLogin will fail if it's non zero. > I agree it doesn't look like it belongs here, but then the only alternative I > can think of is to introduce global state somewhere in SSLErrorHandler which > doesn't sound better. I'm happy to consider any other suggestions. I want to play around with this locally to better understand the tests a bit better (I wrote them ages ago), and see if I can come up with another option. May not have time until tomorrow. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:86: RecordUMA(SSL_WARNING_ALREADY_HANDLED); On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > On 2014/10/30 19:33:44, mmenke wrote: > > Also, this doesn't get anything. > > > > Should just DCHECK(!handled_) and get rid of the histogram value. In general, > > we shouldn't have histograms checking for every obscure, unlikely regression. > > OnCaptivePortalResult might get calledbefore Handle() is called (e.g. the portal > check was triggered by another SSL error), so it's possible that handled_ is > sometimes true, although the window is very small. I removed the histogram entry > though. That can't happen, because we don't spin the message loop between creating the SSLErrorHandler and calling Handle(), and I don't think that's likely to change. I actually think it makes more sense to do a DCHECK(!handled_) than have an if statement here - generally, if something can't possibly happen, the Chrome philosophy is not to handle it. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:137: OnCaptivePortalResult(); On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > On 2014/10/30 19:28:02, mmenke wrote: > > If we don't see a captive portal, and handled_ is false, shouldn't we just > call > > ShowSSLInterstitial()? > > Hmm, isn't it possible that we could see a captive portal in a second > notification before the timer expires? I guess it's a very edge case given the > exponential backoff behavior though. Yes, it's possible, theoretically, but due to exponential backoff, in practice it won't. I think it makes more sense to trust the result we get, rather than wait for another one that won't actually happen, and only show an error page after a timeout. Better responsiveness in the average case is better than better handling of a corner case that probably never happens, in my opinion. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:148: if (handled_) { On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > On 2014/10/30 19:28:02, mmenke wrote: > > Is there any case where handled_ != timer_.IsRunning() here? > > That would be the general case where timer is running and a captive portal is > detected before timer expires, right? > > 1. Handle() -> start timer. IsRunning = true, handled_ = false > 2. OnCaptivePortalResult -> IsRunning = true, handled_ = false Sorry, I meant handled_ != !timer_.IsRunning(). i.e., isn't this the same as the above timer_.IsRunning() check? Both cases where we set handled_ to true, we also stop the timer, so the check is redundant. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:173: RecordUMA(CAPTIVE_PORTAL_DETECTED_ALREADY_HANDLED); This histogram seems in need of improvement. If an error handler sees a captive portal twice in a row, we could call ShowCaptivePortalInterstitial() the first time, then log this the second. Also, we could record it 15 times for a single SSL ErrorHandler, if the user leaves the error tab up, right?
Thanks for taking a quick look. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:86: RecordUMA(SSL_WARNING_ALREADY_HANDLED); On 2014/11/06 22:12:40, mmenke wrote: > On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > > On 2014/10/30 19:33:44, mmenke wrote: > > > Also, this doesn't get anything. > > > > > > Should just DCHECK(!handled_) and get rid of the histogram value. In > general, > > > we shouldn't have histograms checking for every obscure, unlikely > regression. > > > > OnCaptivePortalResult might get calledbefore Handle() is called (e.g. the > portal > > check was triggered by another SSL error), so it's possible that handled_ is > > sometimes true, although the window is very small. I removed the histogram > entry > > though. > > That can't happen, because we don't spin the message loop between creating the > SSLErrorHandler and calling Handle(), and I don't think that's likely to change. > I actually think it makes more sense to do a DCHECK(!handled_) than have an if > statement here - generally, if something can't possibly happen, the Chrome > philosophy is not to handle it. Okay, I was assuming that could happen so I had added a test for it earlier (ShouldShowCaptivePortalInterstitialOnTooEarlyCaptivePortalResult). In the test as soon as I send a captive portal notification, it's received in SSLErrorHandler. I'm not sure why, since I'm not running the message loop there. If it can't happen, then there is no point in testing that scenario. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:137: OnCaptivePortalResult(); On 2014/11/06 22:12:40, mmenke wrote: > On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > > On 2014/10/30 19:28:02, mmenke wrote: > > > If we don't see a captive portal, and handled_ is false, shouldn't we just > > call > > > ShowSSLInterstitial()? > > > > Hmm, isn't it possible that we could see a captive portal in a second > > notification before the timer expires? I guess it's a very edge case given the > > exponential backoff behavior though. > > Yes, it's possible, theoretically, but due to exponential backoff, in practice > it won't. I think it makes more sense to trust the result we get, rather than > wait for another one that won't actually happen, and only show an error page > after a timeout. > > Better responsiveness in the average case is better than better handling of a > corner case that probably never happens, in my opinion. Sure, will do. https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:148: if (handled_) { On 2014/11/06 22:12:40, mmenke wrote: > On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > > On 2014/10/30 19:28:02, mmenke wrote: > > > Is there any case where handled_ != timer_.IsRunning() here? > > > > That would be the general case where timer is running and a captive portal is > > detected before timer expires, right? > > > > 1. Handle() -> start timer. IsRunning = true, handled_ = false > > 2. OnCaptivePortalResult -> IsRunning = true, handled_ = false > > Sorry, I meant handled_ != !timer_.IsRunning(). i.e., isn't this the same as > the above timer_.IsRunning() check? Both cases where we set handled_ to true, > we also stop the timer, so the check is redundant. Good point, looks like CAPTIVE_PORTAL_DETECTED_AFTER_TIMER_EXPIRED and BEFORE_TIMER_TRIGGERED aren't needed since we'd show the interstitial when that happens and the SSLErrorHandler will go away anyways. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:173: RecordUMA(CAPTIVE_PORTAL_DETECTED_ALREADY_HANDLED); On 2014/11/06 22:12:40, mmenke wrote: > This histogram seems in need of improvement. > > If an error handler sees a captive portal twice in a row, we could call > ShowCaptivePortalInterstitial() the first time, then log this the second. > > Also, we could record it 15 times for a single SSL ErrorHandler, if the user > leaves the error tab up, right? I think I'm going to remove that histogram too. As soon as we handle captive portal or ssl error, the SSLErrorHandler should go away anyways, by showing either interstitial.
https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/180001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:86: RecordUMA(SSL_WARNING_ALREADY_HANDLED); On 2014/11/06 23:23:18, Mustafa Emre Acer wrote: > On 2014/11/06 22:12:40, mmenke wrote: > > On 2014/11/06 21:21:56, Mustafa Emre Acer wrote: > > > On 2014/10/30 19:33:44, mmenke wrote: > > > > Also, this doesn't get anything. > > > > > > > > Should just DCHECK(!handled_) and get rid of the histogram value. In > > general, > > > > we shouldn't have histograms checking for every obscure, unlikely > > regression. > > > > > > OnCaptivePortalResult might get calledbefore Handle() is called (e.g. the > > portal > > > check was triggered by another SSL error), so it's possible that handled_ is > > > sometimes true, although the window is very small. I removed the histogram > > entry > > > though. > > > > That can't happen, because we don't spin the message loop between creating the > > SSLErrorHandler and calling Handle(), and I don't think that's likely to > change. > > I actually think it makes more sense to do a DCHECK(!handled_) than have an > if > > statement here - generally, if something can't possibly happen, the Chrome > > philosophy is not to handle it. > > Okay, I was assuming that could happen so I had added a test for it earlier > (ShouldShowCaptivePortalInterstitialOnTooEarlyCaptivePortalResult). > > In the test as soon as I send a captive portal notification, it's received in > SSLErrorHandler. I'm not sure why, since I'm not running the message loop there. > If it can't happen, then there is no point in testing that scenario. In the test it happens because you send the notification yourself between creation and calling Handle(). When running in Chrome, those are called synchronously on the same thread. Any notification about captive portal status will have to wait until the current task is complete before it can be run on the UI thread - the current task will include both calling the constructor and the call to Handle().
https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... chrome/app/generated_resources.grd:9631: + Connect to Network Has this gone through UI review? This feature should probably have a launch bug, with all the privacy/security/UX signoffs (I don't see any issues in terms of privacy/security, but best to be careful). While I completely defer to the UI folks, I think something like "[Network] Login Required" would be clearer. https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... chrome/app/generated_resources.grd:9640: + Connect Login? https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:107: return; Since this can't happen, we shouldn't check for it. Fine to have a DCHECK(!handled_) if you want. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:120: handled_ = true; I think it makes more sense to move this line into the ShowBlah functions, instead of having to set it after every call to them. If you're going to switch to deleting this instead of using handled_ (A simplification I completely support), can just delete |this| in those two methods instead. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:134: void SSLErrorHandler::ShowCaptivePortalInterstitial() { DCHECK(!handled_);? https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:144: void SSLErrorHandler::ShowSSLInterstitial() { DCHECK(!handled_);? https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:157: } nit: Don't use braces when the condition and body each take up one line.
https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... chrome/app/generated_resources.grd:9631: + Connect to Network On 2014/11/07 15:45:30, mmenke wrote: > Has this gone through UI review? This feature should probably have a launch > bug, with all the privacy/security/UX signoffs (I don't see any issues in terms > of privacy/security, but best to be careful). While I completely defer to the > UI folks, I think something like "[Network] Login Required" would be clearer. Yes, in fact it went through a couple of UI iterations. The strings are from the UI team. But I should note that the UI team wants two different strings, one set for wired and another set for Wifi connections. I'm going to do that in a separate CL since it looks like it'll be a significant change, so this CL only includes non-Wifi strings. https://codereview.chromium.org/318213002/diff/200001/chrome/app/generated_re... chrome/app/generated_resources.grd:9640: + Connect On 2014/11/07 15:45:30, mmenke wrote: > Login? While I agree with you, UI team prefers "connect" instead. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:107: return; On 2014/11/07 15:45:30, mmenke wrote: > Since this can't happen, we shouldn't check for it. Fine to have a > DCHECK(!handled_) if you want. Done. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:120: handled_ = true; On 2014/11/07 15:45:30, mmenke wrote: > I think it makes more sense to move this line into the ShowBlah functions, > instead of having to set it after every call to them. > > If you're going to switch to deleting this instead of using handled_ (A > simplification I completely support), can just delete |this| in those two > methods instead. Done. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:134: void SSLErrorHandler::ShowCaptivePortalInterstitial() { On 2014/11/07 15:45:30, mmenke wrote: > DCHECK(!handled_);? Done. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:144: void SSLErrorHandler::ShowSSLInterstitial() { On 2014/11/07 15:45:30, mmenke wrote: > DCHECK(!handled_);? Done. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:157: } On 2014/11/07 15:45:30, mmenke wrote: > nit: Don't use braces when the condition and body each take up one line. Done. https://codereview.chromium.org/318213002/diff/200001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:173: RecordUMA(CAPTIVE_PORTAL_DETECTED_ALREADY_HANDLED); On 2014/11/06 23:23:19, Mustafa Emre Acer wrote: > On 2014/11/06 22:12:40, mmenke wrote: > > This histogram seems in need of improvement. > > > > If an error handler sees a captive portal twice in a row, we could call > > ShowCaptivePortalInterstitial() the first time, then log this the second. > > > > Also, we could record it 15 times for a single SSL ErrorHandler, if the user > > leaves the error tab up, right? > > I think I'm going to remove that histogram too. As soon as we handle captive > portal or ssl error, the SSLErrorHandler should go away anyways, by showing > either interstitial. Removed CAPTIVE_PORTAL_DETECTED_ALREADY_HANDLED
Hope to get to your unit tests later today, but there's a chance I won't. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1763: GetInterstitialType(web_contents)); Should switch back to having web_contents active, and simulate a click on the button, which should us back to having the same login tab open as before. Should also then login, like the next test does (Could make that a different test, if you want, but I'd like to see us login with both SSL interstitials showing - shouldn't matter, but I'm paranoid). https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. This is racy. We depend on the captive portal check triggered by CaptivePortalTabReloader to make it to the IO thread and back to the CaptivePortalTabReloader UI thread only after a message posted from the UI thread to the UI thread completes. That second message is posted only after the tab reloader starts the portal check, so this is actually a race. So...How to fix it? I suggest waiting until the interstitial shows, and only then triggering the captive portal probe result. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2285: } Think we have a new error case that needs to be tested now. A slow SSL load starts. The reloader triggers a captive portal check, finds a captive portal. The SSL finally interstitial commits, triggers another captive portal check. The second captive portal check finds no captive portal. The reloader triggers a reload at the same time the error page handler tries to show an interstitial. We need to make sure the reload happens successfully. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:92: ssl_error_delay = captive_portal_tab_helper->GetSSLErrorDelay(); I need to spend some time thinking about how to move this - I think as-is, this is a layering violation. Moving this class into the captive_portal directory would fix that...But that also seems a little weird. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:95: (new SSLErrorHandler(web_contents, cert_error, ssl_info, request_url, nit: +1 indent https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:117: RecordUMA(CAPTIVE_PORTAL_CHECK); Do we get anything out of this histogram? Basically just tells us if we're on mobile or not. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:49: const base::Callback<void(bool)>& callback); Most non-override methods. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:55: virtual void ShowSSLInterstitial(); Suggest mentioning these are virtual for tests. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:56: void OnTimerExpired(); nit: Should generally have blank lines between non-override/non-inline method declarations.
https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:105: command_line.AppendSwitch(::switches::kTestType); What does this do? https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:120: TestSSLErrorHandler& error_handler() { return *error_handler_.get(); } Should either make this const or make it return a pointer. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:123: scoped_ptr<net::SSLInfo> ssl_info_; Any reason this has to be a scoped_ptr? Seems like we could just stuff it on the stack and be fine. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:139: error_handler().Reset(); optional: Could use counts instead of bools, and get rid of Reset - it's a bit more robust, as it also catches when things happen more than once before/after your reset calls, and I think it makes the tests a little clearer. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:160: captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL); These aren't really checking that the error handler is unhooking itself correctly (Because in these tests, it's not). Maybe a browser test where we make sure the interstitial isn't re-created on new captive portal checks? I suspect the captive portal tests would fail if that happened, anyways, but I'd be happier with tests explicitly checking for that, in absence of the whole login tab stuff. Could additionally/alternatively check that here, by having a shared object keep track of the counts, and calling into the real handler after updating the counters. I'm sure what trying to show an interstitial in these tests would do, though...And we rather depend on browser tests for this, anyways. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:197: error_handler().Handle(); EXPECT_FALSE(error_handler().IsTimerRunning());
https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. On 2014/11/10 17:32:08, mmenke wrote: > This is racy. We depend on the captive portal check triggered by > CaptivePortalTabReloader to make it to the IO thread and back to the > CaptivePortalTabReloader UI thread only after a message posted from the UI > thread to the UI thread completes. That second message is posted only after the > tab reloader starts the portal check, so this is actually a race. > > So...How to fix it? I suggest waiting until the interstitial shows, and only > then triggering the captive portal probe result. The probes are triggered automatically from SSLErrorHandler. Since I can't access SSLErrorHandler from here, it's not clear to me how I can trigger the probe result after the interstitial. WDYT about making SSLErrorHandler a tab helper, so that it can be accessed from here? That'll also fix the layering violation since I won't need to call into CaptivePortalTabHelper to set the ssl interstitial display delay. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:95: (new SSLErrorHandler(web_contents, cert_error, ssl_info, request_url, On 2014/11/10 17:32:08, mmenke wrote: > nit: +1 indent Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:117: RecordUMA(CAPTIVE_PORTAL_CHECK); On 2014/11/10 17:32:08, mmenke wrote: > Do we get anything out of this histogram? Basically just tells us if we're on > mobile or not. Mostly convenience. With this one doesn't need to cross correlate multiple histograms. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:139: error_handler().Reset(); On 2014/11/12 22:15:08, mmenke wrote: > optional: Could use counts instead of bools, and get rid of Reset - it's a bit > more robust, as it also catches when things happen more than once before/after > your reset calls, and I think it makes the tests a little clearer. I used counts before, but then switched to bool because the counters only went up to 1. I find bools easier to read than 1s and 0s in the counts. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:160: captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL); On 2014/11/12 22:15:08, mmenke wrote: > These aren't really checking that the error handler is unhooking itself > correctly (Because in these tests, it's not). > > Maybe a browser test where we make sure the interstitial isn't re-created on new > captive portal checks? I suspect the captive portal tests would fail if that > happened, anyways, but I'd be happier with tests explicitly checking for that, > in absence of the whole login tab stuff. > > Could additionally/alternatively check that here, by having a shared object keep > track of the counts, and calling into the real handler after updating the > counters. I'm sure what trying to show an interstitial in these tests would do, > though...And we rather depend on browser tests for this, anyways. Will try to add a browser test for this.
https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:117: RecordUMA(CAPTIVE_PORTAL_CHECK); On 2014/11/14 00:30:25, Mustafa Emre Acer wrote: > On 2014/11/10 17:32:08, mmenke wrote: > > Do we get anything out of this histogram? Basically just tells us if we're on > > mobile or not. > > Mostly convenience. With this one doesn't need to cross correlate multiple > histograms. Actually, I'll probably delete this histogram.
https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. On 2014/11/10 17:32:08, mmenke wrote: > That second message is posted only after the > tab reloader starts the portal check, so this is actually a race. I'm also not sure I understand the race here. What's the second message you are referring to?
https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. On 2014/11/14 00:30:25, Mustafa Emre Acer wrote: > On 2014/11/10 17:32:08, mmenke wrote: > > This is racy. We depend on the captive portal check triggered by > > CaptivePortalTabReloader to make it to the IO thread and back to the > > CaptivePortalTabReloader UI thread only after a message posted from the UI > > thread to the UI thread completes. That second message is posted only after > the > > tab reloader starts the portal check, so this is actually a race. > > > > So...How to fix it? I suggest waiting until the interstitial shows, and only > > then triggering the captive portal probe result. > > The probes are triggered automatically from SSLErrorHandler. Since I can't > access SSLErrorHandler from here, it's not clear to me how I can trigger the > probe result after the interstitial. > > WDYT about making SSLErrorHandler a tab helper, so that it can be accessed from > here? That'll also fix the layering violation since I won't need to call into > CaptivePortalTabHelper to set the ssl interstitial display delay. I'm not sure if that fixes the race. I had been thinking this test fixture allowed us to control when the captive portal result came in, but it looks like it doesn't - would need a little work. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. On 2014/11/14 01:51:24, Mustafa Emre Acer wrote: > On 2014/11/10 17:32:08, mmenke wrote: > > > That second message is posted only after the > > tab reloader starts the portal check, so this is actually a race. > > I'm also not sure I understand the race here. What's the second message you are > referring to? The timer - you tell it to trigger instantly, but at that point, you've already tried to trigger the captive portal probe, so it may have already completed and posted a result back to the UI thread before you start the 0-second timer. You could just flip the order - call Handle() before triggering the probe, but that seems extremely regression prone it me, and since it would be a flaky regression, I think we need something more robust.
mmenke@: Sorry for the late response, I was trying to figure out a problem with the login scenario in the browser test (particularly, the "connect" button on the interstitial wasn't always available to click). I think there is one more scenario I need to address (your last comment in browser tests). In the meanwhile, ptal. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1763: GetInterstitialType(web_contents)); On 2014/11/10 17:32:08, mmenke wrote: > Should switch back to having web_contents active, and simulate a click on the > button, which should us back to having the same login tab open as before. > > Should also then login, like the next test does (Could make that a different > test, if you want, but I'd like to see us login with both SSL interstitials > showing - shouldn't matter, but I'm paranoid). Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1782: // of a captive portal error page. On 2014/11/14 15:28:55, mmenke wrote: > On 2014/11/14 00:30:25, Mustafa Emre Acer wrote: > > On 2014/11/10 17:32:08, mmenke wrote: > > > This is racy. We depend on the captive portal check triggered by > > > CaptivePortalTabReloader to make it to the IO thread and back to the > > > CaptivePortalTabReloader UI thread only after a message posted from the UI > > > thread to the UI thread completes. That second message is posted only after > > the > > > tab reloader starts the portal check, so this is actually a race. > > > > > > So...How to fix it? I suggest waiting until the interstitial shows, and > only > > > then triggering the captive portal probe result. > > > > The probes are triggered automatically from SSLErrorHandler. Since I can't > > access SSLErrorHandler from here, it's not clear to me how I can trigger the > > probe result after the interstitial. > > > > WDYT about making SSLErrorHandler a tab helper, so that it can be accessed > from > > here? That'll also fix the layering violation since I won't need to call into > > CaptivePortalTabHelper to set the ssl interstitial display delay. > > I'm not sure if that fixes the race. > > I had been thinking this test fixture allowed us to control when the captive > portal result came in, but it looks like it doesn't - would need a little work. Added a flag to tab reloader to enable/disable portal checks on SSL errors. It now disables portal check until an interstitial is displayed and then fires one once there is an interstitial. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:49: const base::Callback<void(bool)>& callback); On 2014/11/10 17:32:08, mmenke wrote: > Most non-override methods. Sorry, not sure what you mean here. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:55: virtual void ShowSSLInterstitial(); On 2014/11/10 17:32:08, mmenke wrote: > Suggest mentioning these are virtual for tests. Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:56: void OnTimerExpired(); On 2014/11/10 17:32:08, mmenke wrote: > nit: Should generally have blank lines between non-override/non-inline method > declarations. Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:105: command_line.AppendSwitch(::switches::kTestType); On 2014/11/12 22:15:08, mmenke wrote: > What does this do? Good catch, it's an artifact from previous patches to check if ssl error handler should disable portal checks etc. Removed. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:120: TestSSLErrorHandler& error_handler() { return *error_handler_.get(); } On 2014/11/12 22:15:08, mmenke wrote: > Should either make this const or make it return a pointer. Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:123: scoped_ptr<net::SSLInfo> ssl_info_; On 2014/11/12 22:15:08, mmenke wrote: > Any reason this has to be a scoped_ptr? Seems like we could just stuff it on > the stack and be fine. Done. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:197: error_handler().Handle(); On 2014/11/12 22:15:08, mmenke wrote: > EXPECT_FALSE(error_handler().IsTimerRunning()); Done.
May not get to this until tomorrow. https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/240001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:49: const base::Callback<void(bool)>& callback); On 2014/11/25 01:39:52, Mustafa Emre Acer wrote: > On 2014/11/10 17:32:08, mmenke wrote: > > Most non-override methods. > > Sorry, not sure what you mean here. Erm...that makes two of us. I'm sure I had a very, very good point. Brilliant, even. Too bad I'm not smart enough to understand it.
https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:123: bool WaitForPageReady(content::RenderViewHost* rvh) { Hrm...Do you know what this is needed? We embed the Javascript in the HTML, so it should run before the load stops/completes, right? https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1056: DCHECK(contents->ShowingInterstitialPage()); Rather than a DCHECK, seems like we should just asset on these. How about returning nullptr here? No so concerned about the other case https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2376: } One test I thought I asked about, but can't seem to find: The same captive portal check triggers both reloading a tab, because we're no longer behind a captive portal, and the SSLErrorHandler showing an SSL interstitial, because it now knows it's not behind a captive portal. This may be a bit tricky to set up - you need a hanging SSL load to trigger a captive portal check, find a captive portal, and then an SSL error, creating the SSLErrorHandler, and then a probe that doesn't file a captive portal...Ideally, the reload would win the fight, but I'm happy with just not crashing. It may be a little difficult to set up, but it's the case I think is most likely to result in a crash. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:62: nit: Remove blank line https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:82: load_time_data->SetString("finalParagraph", base::string16()); If you us jscontent=blah rather than i18n-values=".innerHTML: blah", this shouldn't be necessary (Or if you need HTML support, i18n-values=".innerHTML: blah ? blah : ''"). If you don't need HTML, though, suggest not allowing it). Think requiring these all be kept in sync to avoid Javascript warnings doesn't work that well, unless you have browser tests for that. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:27: CaptivePortalBlockingPage(content::WebContents* web_contents, Should forward declare WebContents https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:454: load_time_data->SetString("primaryParagraph", Mind having someone else review the js / HTML related changes for the interstitial? I don't think there's much reason for me to learn them well enough to do a good review of these changes. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:44: return channel <= chrome::VersionInfo::CHANNEL_DEV; Suggest using Finch instead - I think should avoid channel checks like this in the code, so all channels by default run the same code. Alternatively, just enable it on all channels. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:125: #endif optional: Should this have an #else NOTREACHED, just like ShowCaptivePortalInterstitial? https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:180: delete this; Should think about tests for these deletes. Just exercise the cases, and make sure nothing seems to completely fail, and we don't show an interstitial when the captive portal result comes back (Can't test that for the destruction case, of course, but can for the others). https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:26: // captive portal error page. Should give details on how it does this...The asynchronous decision making is pretty important, and not obvious without digging into the class. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:27: // It deletes itself when the interstitial page is closed. nit: Somewhat weird to use a double break between paragraphs below, but not here. Suggest either adding a blank comment line separating it from the one above, or merging it into the previous paragraph. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: void Handle(); "Handle" is a rather unclear... Suggest just HandleError or OnError or somesuch. "On" may be a little better than "Handle" since it may not actually "handle" the error, bur rather wait for for the captive portal check to complete. Or maybe "StartHandlingError" or somesuch. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:53: void OnTimerExpired(); Should document these methods. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:74: content::WebContents* web_contents_; Not needed. See WebContentsObserver::web_contents()
Sorry for the delay. Took me way too long to figure out how to interstitial-commit a slow loading SSL URLRequestJob for the browser tests. Can you PTAL? https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:123: bool WaitForPageReady(content::RenderViewHost* rvh) { On 2014/11/26 18:57:48, mmenke wrote: > Hrm...Do you know what this is needed? We embed the Javascript in the HTML, so > it should run before the load stops/completes, right? C++ side only waits for the navigation to complete and the interstitial to attach, it doesn't wait for onload event from the page. I think a better fix is just to set a callback that the interstitial code can call once it loads, but I'd like to do that in a separate CL. Some of the interstitial tests (both SSL and SB) are in fact flaky and I'll go over them later on. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1056: DCHECK(contents->ShowingInterstitialPage()); On 2014/11/26 18:57:48, mmenke wrote: > Rather than a DCHECK, seems like we should just asset on these. How about > returning nullptr here? No so concerned about the other case Done. Using NULL instead of nullptr to be consistent with the rest of the file. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2376: } On 2014/11/26 18:57:48, mmenke wrote: > One test I thought I asked about, but can't seem to find: The same captive > portal check triggers both reloading a tab, because we're no longer behind a > captive portal, and the SSLErrorHandler showing an SSL interstitial, because it > now knows it's not behind a captive portal. > > This may be a bit tricky to set up - you need a hanging SSL load to trigger a > captive portal check, find a captive portal, and then an SSL error, creating the > SSLErrorHandler, and then a probe that doesn't file a captive portal...Ideally, > the reload would win the fight, but I'm happy with just not crashing. > > It may be a little difficult to set up, but it's the case I think is most likely > to result in a crash. Added InterstitialTimerCertErrorAfterSlowLoad. This is what happens in this test in chronological order: - Cert error is committed. - SSLErrorHandler triggers a portal check. - Portal result immediately arrives. - Between tab reloader and SSLErrorHandler, tab reloader wins. It goes from BROKEN_BY_PORTAL to NEEDS_RELOAD. - Tab reloader then tries to reload but the logic in CaptivePortalTabReloader::ReloadTabIfNeeded ignores the reload since the interstitial hasn't attached yet. - SSLErrorHandler attaches the interstitial. tl; dr: Reloader wins but since there is no interstitial, it doesn't reload. SSLErrorHandler then attaches the interstitial. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:62: On 2014/11/26 18:57:48, mmenke wrote: > nit: Remove blank line Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:82: load_time_data->SetString("finalParagraph", base::string16()); On 2014/11/26 18:57:48, mmenke wrote: > If you us jscontent=blah rather than i18n-values=".innerHTML: blah", this > shouldn't be necessary (Or if you need HTML support, i18n-values=".innerHTML: > blah ? blah : ''"). If you don't need HTML, though, suggest not allowing it). > > Think requiring these all be kept in sync to avoid Javascript warnings doesn't > work that well, unless you have browser tests for that. We'll be refactoring the interstitials further in this quarter or the next, I'll put this in the todo list. For now both SSL and SB interstitials use this mechanism so I'd like to keep it consistent. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:27: CaptivePortalBlockingPage(content::WebContents* web_contents, On 2014/11/26 18:57:48, mmenke wrote: > Should forward declare WebContents Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:454: load_time_data->SetString("primaryParagraph", On 2014/11/26 18:57:48, mmenke wrote: > Mind having someone else review the js / HTML related changes for the > interstitial? I don't think there's much reason for me to learn them well > enough to do a good review of these changes. Sure, will do. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:44: return channel <= chrome::VersionInfo::CHANNEL_DEV; On 2014/11/26 18:57:49, mmenke wrote: > Suggest using Finch instead - I think should avoid channel checks like this in > the code, so all channels by default run the same code. > > Alternatively, just enable it on all channels. Enabled on all channels. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:125: #endif On 2014/11/26 18:57:49, mmenke wrote: > optional: Should this have an #else NOTREACHED, just like > ShowCaptivePortalInterstitial? Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:180: delete this; On 2014/11/26 18:57:49, mmenke wrote: > Should think about tests for these deletes. Just exercise the cases, and make > sure nothing seems to completely fail, and we don't show an interstitial when > the captive portal result comes back (Can't test that for the destruction case, > of course, but can for the others). I added a check at the end of CaptivePortalBrowserTest.ShowCaptivePortalInterstitialOnCertError to capture recreating the captive portal interstitial scenario. Also, CaptivePortalBrowserTest.InterstitialTimer* should sufficiently cover the cases with the error handler being destroyed. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:26: // captive portal error page. On 2014/11/26 18:57:49, mmenke wrote: > Should give details on how it does this...The asynchronous decision making is > pretty important, and not obvious without digging into the class. Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:27: // It deletes itself when the interstitial page is closed. On 2014/11/26 18:57:49, mmenke wrote: > nit: Somewhat weird to use a double break between paragraphs below, but not > here. Suggest either adding a blank comment line separating it from the one > above, or merging it into the previous paragraph. Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:52: void Handle(); On 2014/11/26 18:57:49, mmenke wrote: > "Handle" is a rather unclear... Suggest just HandleError or OnError or > somesuch. "On" may be a little better than "Handle" since it may not actually > "handle" the error, bur rather wait for for the captive portal check to > complete. > > Or maybe "StartHandlingError" or somesuch. I like "StartHandlingError". https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:53: void OnTimerExpired(); On 2014/11/26 18:57:49, mmenke wrote: > Should document these methods. Done. https://codereview.chromium.org/318213002/diff/260001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:74: content::WebContents* web_contents_; On 2014/11/26 18:57:49, mmenke wrote: > Not needed. See WebContentsObserver::web_contents() Done.
On 2014/12/08 22:29:51, Mustafa Emre Acer wrote: > Sorry for the delay. Took me way too long to figure out how to > interstitial-commit a slow loading SSL URLRequestJob for the browser tests. Can > you PTAL? Not a problem - I've been high latency on this one as well. I'll try and get to this tomorrow afternoon.
Not a full pass - haven't looked at tests again, in particular. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.h:54: void SetPortalDetectionEnabledForTest(bool enabled); Having both this and set_state_for_testing seems a bit redundant. Can we just add a new TestingState? IGNORE_REQUESTS_FOR_TESTING. (FOR_TESTING is redundant, but matches the others). Should probably make it act like SKIP_OS_CHECK_FOR_TESTING as well, for consistency, though you'll probably be setting it after the OS check, anyways. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.h:152: base::TimeDelta ssl_error_delay_; As much as I dislike statics variables, I think this would make more sense as a static member of SSLBlockingPage. Per earlier comments, I think using this as a settings object for SSLBlockingPage is weird. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/intersti... File chrome/browser/interstitials/security_interstitial_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/intersti... chrome/browser/interstitials/security_interstitial_page.h:10: #include "grit/browser_resources.h" I don't think you mean to make any changes in this file, and the corresponding cc file, in this CL? https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:45: : SecurityInterstitialPage(web_contents, request_url) { #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) NOTREACHED() #endif Maybe? https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:25: // UI thread. + "Only used when ENABLE_CAPTIVE_PORTAL_DETECTION is true."? https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: // SecurityInterstitialPage implementation: nit: Should be consistent. You use "method:" above, "implementation:" here, and "implementation." below. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:454: load_time_data->SetString("primaryParagraph", Does removing this clock logic belong in this CL? https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:85: static bool IsOverridable(const int options_mask) { -const https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:87: !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT); Should either be named is_overrideable, or not inlined. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:95: friend class TestSSLErrorHandler; friend classes should go first....But is this even necessary? if it needs access to private methods, can make them protected instead. I think it just needs StartHandlingError, and a method to check if timer is running? https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:43: } I believe this can just be: using SSLErrorHandler::StartHandlingError;
https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.h:54: void SetPortalDetectionEnabledForTest(bool enabled); On 2014/12/09 22:42:26, mmenke wrote: > Having both this and set_state_for_testing seems a bit redundant. Can we just > add a new TestingState? IGNORE_REQUESTS_FOR_TESTING. (FOR_TESTING is > redundant, but matches the others). > > Should probably make it act like SKIP_OS_CHECK_FOR_TESTING as well, for > consistency, though you'll probably be setting it after the OS check, anyways. I considered doing that previously but wasn't sure how it would work, given it's not possible to combine testing states. Added a new testing state now, with checks in the browser test to ensure that setting the new state only occurs after SKIP_OS_CHECK_FOR_TESTING is set. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_tab_helper.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_tab_helper.h:152: base::TimeDelta ssl_error_delay_; On 2014/12/09 22:42:26, mmenke wrote: > As much as I dislike statics variables, I think this would make more sense as a > static member of SSLBlockingPage. Per earlier comments, I think using this as a > settings object for SSLBlockingPage is weird. Moved to SSLErrorHandler as a static variable. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/intersti... File chrome/browser/interstitials/security_interstitial_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/intersti... chrome/browser/interstitials/security_interstitial_page.h:10: #include "grit/browser_resources.h" On 2014/12/09 22:42:26, mmenke wrote: > I don't think you mean to make any changes in this file, and the corresponding > cc file, in this CL? These were rebase artifacts (or "lack of rebase", to be more precise). They are gone now. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:45: : SecurityInterstitialPage(web_contents, request_url) { On 2014/12/09 22:42:26, mmenke wrote: > #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) > NOTREACHED() > #endif > > Maybe? Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:25: // UI thread. On 2014/12/09 22:42:26, mmenke wrote: > + "Only used when ENABLE_CAPTIVE_PORTAL_DETECTION is true."? Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: // SecurityInterstitialPage implementation: On 2014/12/09 22:42:26, mmenke wrote: > nit: Should be consistent. You use "method:" above, "implementation:" here, > and "implementation." below. Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.cc:454: load_time_data->SetString("primaryParagraph", On 2014/12/09 22:42:26, mmenke wrote: > Does removing this clock logic belong in this CL? Another rebase artifact. Gone. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:85: static bool IsOverridable(const int options_mask) { On 2014/12/09 22:42:26, mmenke wrote: > -const Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:87: !(options_mask & SSLBlockingPage::STRICT_ENFORCEMENT); On 2014/12/09 22:42:26, mmenke wrote: > Should either be named is_overrideable, or not inlined. Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:95: friend class TestSSLErrorHandler; On 2014/12/09 22:42:26, mmenke wrote: > friend classes should go first....But is this even necessary? if it needs > access to private methods, can make them protected instead. I think it just > needs StartHandlingError, and a method to check if timer is running? Done. https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/318213002/diff/280001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:43: } On 2014/12/09 22:42:26, mmenke wrote: > I believe this can just be: > > using SSLErrorHandler::StartHandlingError; Done.
Sorry for not getting back to this, this week. Been a hectic week (Aren't they all?)
Sorry for the delay - I've been giving this low priority because of its complexity, and you don't seem to be in any rush. I think we're (finally) just about there! https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:124: bool WaitForPageReady(content::RenderViewHost* rvh) { Suggest renaming this WaitForInterstitialReady, and checking that rvh is indeed showing an interstitial (In fact, probably simpler just to take an interstitial page). https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:127: std::string ready_state; Should include <string> https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:129: scoped_ptr<base::Value> value = content::ExecuteScriptAndGetValue( Should include the scoped_ptr and base/values headers. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:133: } while (ready_state != "complete"); Think you mentioned this option earlier, but I'd be happier injecting something into the page that called back into us, rather than polling. To do this: context::ExecuteStringAndExtractBool( rvh->GetMainFrame(), "function() {" // Begin anonymous namespace. " var done = false;" " function checkState() {" " if (!done && document.readyState == 'complete') {" " done = true;" " window.domAutomationController.send(true);" " }" " }" " checkState();" " document.onreadystatechange = checkState();" "}();"; // End anonymous namespace. ); And then checking the result as you currently do. It's a bit more code, but polling is an anti-pattern in tests. WDYT? document.addEventListener('readystatechange',...) is better than setting an onreadystatechange event, if it works, since it doesn't preclude anything else from listening to the event, unlike setting the onreadstatechange method on document. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:357: DCHECK(job->request()->url().SchemeIs(url::kHttpsScheme)); Currently this test fixture uses EXPECTs/ASSERTs for code run on the IO thread, so should be consistent. (I don't really care which we use, but I do feel we shouldn't mix the two unless we have a well-considered reason to do so) https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:859: // after captive portal service setup is done. Maybe "When disabled, probe requests are are silently ignored, never receiving a response." https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:860: void EnablePortalRequests(bool enabled); Maybe RespondToPortalRequests? Or RespondToProbeRequests https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:953: // result in an error rather than hanging. disable_portal_check_until_interstitial needs some documentation. Maybe "If |disable_portal_check_until_interstitial| is true, captive portal requests are ignored until the interstitial is shown, at which point a captive portal result is sent. This allows testing in conjunction with the certificate error interstitial." https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:958: bool disable_portal_check_until_interstitial); "disable_portal_check_until_interstitial" could be clearer, as this is different from normally disabling the checks at runtime, which makes the service claim we're always online. Maybe "delay_portal_response_until_interstital" https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:985: void LoginCertError(Browser* browser); Why wasn't this needed for the cert error test before? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1520: void CaptivePortalBrowserTest::LoginCertError(Browser* browser) { Could we order the code here more like CaptivePortalBrowserTest::Login, so it's easier to compare the two. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1870: base::TimeDelta::FromHours(1)); Suggest moving this into the constructor for the test fixture. Shouldn't affect any of the other tests, anyways, but I'm paranoid, and it's one less cause of variability between test runs. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1884: int login_tab_index = tab_strip_model->active_index(); Suggest moving this below the comment. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1888: Remove this line break (The comment above applies to more than just the line immediately below it. Removing the line break makes that clearer) https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1891: EXPECT_TRUE(WaitForPageReady(rvh)); Suggest a comment above this line, about having to wait for the interstitial to load so it has the button to click. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1896: EXPECT_EQ(login_tab_index, tab_strip_model->active_index()); Should we have a test where this opens another tab? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1928: void CaptivePortalBrowserTest::FastErrorWithInterstitialTimer( This should go up with the other CaptivePortalBrowserTest methods, in the same order it is declared. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1957: // - Portal checks are disabled. Should clarify this - The CaptivePortalService has its own notion of "disabled" which differs strongly from the concept here. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2058: EnablePortalRequests(true); Why don't we have a new SSLErrorHandler waiting on a probe result here? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2078: // page should load, and no interstitials shoudl be shown. nit: shoudl -> should https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2112: test_navigation_observer.WaitForNavigations(2); Why are there two navigations here? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2175: // like navigating to kMockHttpsQuickTimeoutUrl. Wonder about this comment - we actually trigger the check in FastErrorBehindCaptivePortal itself, rather than relying on the interstitial's check. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2654: SetSlowSSLLoadTime(tab_reloader, base::TimeDelta()); Shouldn't we be setting the 1 hour time here? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:362: if (testing_state_ != SKIP_OS_CHECK_FOR_TESTING && testing_state_ != SKIP_OS_CHECK_FOR_TESTING && testing_state_ != IGNORE_REQUESTS_FOR_TESTING && And a comment in the header that the latter implies the former. Tests pass without it, since we don't call UpdateEnabledState after setting the testing state, but it's more robust against refactoring. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: // SecurityInterstitialPage method: nit: methods? https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:64: const base::OneShotTimer<SSLErrorHandler>& get_timer_for_testing() const { optional nit: not sure the for testing is needed for protected methods. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:69: nit: No blank line after timer.
Thanks. This is actually top priority for me, I was just being unintentionally slow :) https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:124: bool WaitForPageReady(content::RenderViewHost* rvh) { On 2014/12/15 20:46:07, mmenke wrote: > Suggest renaming this WaitForInterstitialReady, and checking that rvh is indeed > showing an interstitial (In fact, probably simpler just to take an interstitial > page). Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:127: std::string ready_state; On 2014/12/15 20:46:08, mmenke wrote: > Should include <string> Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:129: scoped_ptr<base::Value> value = content::ExecuteScriptAndGetValue( On 2014/12/15 20:46:07, mmenke wrote: > Should include the scoped_ptr and base/values headers. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:133: } while (ready_state != "complete"); On 2014/12/15 20:46:07, mmenke wrote: > Think you mentioned this option earlier, but I'd be happier injecting something > into the page that called back into us, rather than polling. > > To do this: > > context::ExecuteStringAndExtractBool( > rvh->GetMainFrame(), > "function() {" // Begin anonymous namespace. > " var done = false;" > " function checkState() {" > " if (!done && document.readyState == 'complete') {" > " done = true;" > " window.domAutomationController.send(true);" > " }" > " }" > " checkState();" > " document.onreadystatechange = checkState();" > "}();"; // End anonymous namespace. > ); > > And then checking the result as you currently do. It's a bit more code, but > polling is an anti-pattern in tests. WDYT? > > document.addEventListener('readystatechange',...) is better than setting an > onreadystatechange event, if it works, since it doesn't preclude anything else > from listening to the event, unlike setting the onreadstatechange method on > document. Yes, as I mentioned earlier, I was planning to do this in a separate CL so that I could fix SafeBrowsing code as well. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:357: DCHECK(job->request()->url().SchemeIs(url::kHttpsScheme)); On 2014/12/15 20:46:07, mmenke wrote: > Currently this test fixture uses EXPECTs/ASSERTs for code run on the IO thread, > so should be consistent. (I don't really care which we use, but I do feel we > shouldn't mix the two unless we have a well-considered reason to do so) Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:859: // after captive portal service setup is done. On 2014/12/15 20:46:08, mmenke wrote: > Maybe "When disabled, probe requests are are silently ignored, never receiving a > response." Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:860: void EnablePortalRequests(bool enabled); On 2014/12/15 20:46:07, mmenke wrote: > Maybe RespondToPortalRequests? Or RespondToProbeRequests Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:953: // result in an error rather than hanging. On 2014/12/15 20:46:08, mmenke wrote: > disable_portal_check_until_interstitial needs some documentation. Maybe "If > |disable_portal_check_until_interstitial| is true, captive portal requests are > ignored until the interstitial is shown, at which point a captive portal result > is sent. This allows testing in conjunction with the certificate error > interstitial." Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:958: bool disable_portal_check_until_interstitial); On 2014/12/15 20:46:08, mmenke wrote: > "disable_portal_check_until_interstitial" could be clearer, as this is different > from normally disabling the checks at runtime, which makes the service claim > we're always online. > > Maybe "delay_portal_response_until_interstital" Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:985: void LoginCertError(Browser* browser); On 2014/12/15 20:46:07, mmenke wrote: > Why wasn't this needed for the cert error test before? It was part of the cert error test, I just pulled the common login code into this method so that it can be used by ShowCaptivePortalInterstitialOnCertError as well. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1520: void CaptivePortalBrowserTest::LoginCertError(Browser* browser) { On 2014/12/15 20:46:08, mmenke wrote: > Could we order the code here more like CaptivePortalBrowserTest::Login, so it's > easier to compare the two. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1870: base::TimeDelta::FromHours(1)); On 2014/12/15 20:46:08, mmenke wrote: > Suggest moving this into the constructor for the test fixture. Shouldn't affect > any of the other tests, anyways, but I'm paranoid, and it's one less cause of > variability between test runs. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1884: int login_tab_index = tab_strip_model->active_index(); On 2014/12/15 20:46:07, mmenke wrote: > Suggest moving this below the comment. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1888: On 2014/12/15 20:46:08, mmenke wrote: > Remove this line break (The comment above applies to more than just the line > immediately below it. Removing the line break makes that clearer) Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1891: EXPECT_TRUE(WaitForPageReady(rvh)); On 2014/12/15 20:46:08, mmenke wrote: > Suggest a comment above this line, about having to wait for the interstitial to > load so it has the button to click. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1896: EXPECT_EQ(login_tab_index, tab_strip_model->active_index()); On 2014/12/15 20:46:07, mmenke wrote: > Should we have a test where this opens another tab? Added that scenario here: After the first click on the "connect" button in the error page, this test now closes the login tab, clicks the "connect" button again and expects a new login tab to open. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1928: void CaptivePortalBrowserTest::FastErrorWithInterstitialTimer( On 2014/12/15 20:46:07, mmenke wrote: > This should go up with the other CaptivePortalBrowserTest methods, in the same > order it is declared. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1957: // - Portal checks are disabled. On 2014/12/15 20:46:08, mmenke wrote: > Should clarify this - The CaptivePortalService has its own notion of "disabled" > which differs strongly from the concept here. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2058: EnablePortalRequests(true); On 2014/12/15 20:46:08, mmenke wrote: > Why don't we have a new SSLErrorHandler waiting on a probe result here? Reloading effectively stops the navigation (this is the normal behavior with slow loading pages). Since the load is stopped, there is no cert error and no SSLErrorHandler. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2078: // page should load, and no interstitials shoudl be shown. On 2014/12/15 20:46:08, mmenke wrote: > nit: shoudl -> should Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2112: test_navigation_observer.WaitForNavigations(2); On 2014/12/15 20:46:08, mmenke wrote: > Why are there two navigations here? First one is for stopping the hanging navigation, second one is for completing the second page load. The second one is generated because of the FrameHostMsg_DidStopLoading IPC. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2175: // like navigating to kMockHttpsQuickTimeoutUrl. On 2014/12/15 20:46:07, mmenke wrote: > Wonder about this comment - we actually trigger the check in > FastErrorBehindCaptivePortal itself, rather than relying on the interstitial's > check. The comment is from the current SSLCertErrorLogin code (even though rietveld shows this as new code, it's not). I admit I don't quite understand what it referred to before, but tried to clarify. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2654: SetSlowSSLLoadTime(tab_reloader, base::TimeDelta()); On 2014/12/15 20:46:07, mmenke wrote: > Shouldn't we be setting the 1 hour time here? This is the timeout for the slow ssl timer, not the interstitial. Anyways, removed because it's not actually necessary here, as SlowLoadBehindCaptivePortal already sets it to zero. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.cc (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.cc:362: if (testing_state_ != SKIP_OS_CHECK_FOR_TESTING && On 2014/12/15 20:46:08, mmenke wrote: > testing_state_ != SKIP_OS_CHECK_FOR_TESTING && > testing_state_ != IGNORE_REQUESTS_FOR_TESTING && > > And a comment in the header that the latter implies the former. Tests pass > without it, since we don't call UpdateEnabledState after setting the testing > state, but it's more robust against refactoring. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:39: // SecurityInterstitialPage method: On 2014/12/15 20:46:08, mmenke wrote: > nit: methods? Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:64: const base::OneShotTimer<SSLErrorHandler>& get_timer_for_testing() const { On 2014/12/15 20:46:08, mmenke wrote: > optional nit: not sure the for testing is needed for protected methods. Done. https://codereview.chromium.org/318213002/diff/300001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:69: On 2014/12/15 20:46:08, mmenke wrote: > nit: No blank line after timer. Done.
LGTM (6 months later... :) https://codereview.chromium.org/318213002/diff/320001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/320001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2093: RespondToProbeRequests(true); On 2014/12/16 01:23:19, Mustafa Emre Acer wrote: > On 2014/12/15 20:46:08, mmenke wrote: > > Why don't we have a new SSLErrorHandler waiting on a probe result here? > > Reloading effectively stops the navigation (this is the normal behavior with > slow loading pages). Since the load is stopped, there is no cert error and no > SSLErrorHandler. Think that's worth a comment in the test.
meacer@chromium.org changed reviewers: + bauerb@chromium.org, felt@chromium.org, thestig@chromium.org
w00t :) Thanks! I'll address your last comment. felt: Can you PTAL at the following? chrome/browser/ssl/ssl_* chrome/browser/resources/security_warnings/interstitial_v2.(css|html|js) OWNERS: Can you PTAL? thestig: chrome/browser/chrome_notification_types.h? bauerb: chrome/browser/resources/security_warnings/*
We are actually trying to get rid of notifications, so can we not add any more? See some of the CLs on http://crbug.com/268984 for examples of how others converted existing notifications.
On 2014/12/17 20:36:12, Lei Zhang wrote: > We are actually trying to get rid of notifications, so can we not add any more? > See some of the CLs on http://crbug.com/268984 for examples of how others > converted existing notifications. You're right, thanks for pointing that out! I meant to suggest we get away from that, but forgot about it over the course of the review.
mmenke: I'll remove the notification and add a method to SSLErrorHandler that sets a static timer fired callback that's only used for testing. Does that sgty?
Removed the notification. mmenke, PTAL at the browsertest change. https://codereview.chromium.org/318213002/diff/320001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/320001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:2093: RespondToProbeRequests(true); On 2014/12/17 18:45:06, mmenke wrote: > On 2014/12/16 01:23:19, Mustafa Emre Acer wrote: > > On 2014/12/15 20:46:08, mmenke wrote: > > > Why don't we have a new SSLErrorHandler waiting on a probe result here? > > > > Reloading effectively stops the navigation (this is the normal behavior with > > slow loading pages). Since the load is stopped, there is no cert error and no > > SSLErrorHandler. > > Think that's worth a comment in the test. Done.
https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:63: } Can't the test just use content::WaitForInterstitialAttach? See content/public/test/browser_test_utils.h.
https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:848: void OnTimerFired(content::WebContents* web_contents); Could this be private? https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:866: SSLErrorHandler::SetInterstitialTimerFiredCallbackForTest(NULL); nullptr https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:876: base::MessageLoopForUI::current()->Quit(); Don't directly quit message loops. Instead, use a content::MessageLoopRunner (or in unit tests a RunLoop). https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1163: return NULL; nullptr? https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1167: DCHECK(blocking_page); This DCHECK is kind of unnecessary; If |blocking_page| is indeed null, the next line will crash anyway. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1624: render_frame_host->ExecuteJavaScript(base::ASCIIToUTF16("submitForm()")); Could you use content::ExecuteScript() here? https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.h (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.h:33: SKIP_OS_CHECK_FOR_TESTING, // The service can be enabled even if the OS has The previous alignment was okay (two spaces before //), but it might look even nicer if you aligned all the comments. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:69: var ssl = interstitialType === 'SSL'; The === isn't really necessary here if you know that |interstitialType| is going to be a string and not a weird object. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:91: sendCommand(CAPTIVEPORTAL_CMD_OPEN_LOGIN_PAGE); This should be indented two more spaces. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:28: const int kDefaultInterstitialDisplayDelay = 2; Add an "S" or "Seconds" suffix to clarify the unit? https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:49: base::TimeDelta SSLErrorHandler::interstitial_display_delay_ = You could probably put these into the anonymous namespace if they're private and static. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:54: NULL; nullptr https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:106: if (timer_fired_callback_ && !timer_fired_callback_->is_null()) Couldn't we simply make it an error to set |timer_fired_callback_| to a pointer to a null callback? :) https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:109: #endif You could put the ShowSSLInterstitial() call into an #else instead of having nonreachable code after the return. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:182: delete this; Another idea: If this class's lifetime is tied to the lifetime of the WebContents anyway, you could make it a WebContentsUserData. That would allow you to get rid of the self-deletions here (which at least make me a little queasy). https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:45: ~SSLErrorHandler() override; Please make this protected. If this class deletes itself, no one else should be able to do that. Similarly for the constructor: If the correct way to use this class is through HandleSSLError(), there is no need to make the constructor public. https://codereview.chromium.org/318213002/diff/340001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/318213002/diff/340001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2574: 'browser/ssl/ssl_error_handler.cc', Sort alphabetically please ("handler" before "info").
bauerb, mmenke: PTAL? https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:848: void OnTimerFired(content::WebContents* web_contents); On 2014/12/18 18:10:05, Bernhard Bauer wrote: > Could this be private? Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:866: SSLErrorHandler::SetInterstitialTimerFiredCallbackForTest(NULL); On 2014/12/18 18:10:05, Bernhard Bauer wrote: > nullptr Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:876: base::MessageLoopForUI::current()->Quit(); On 2014/12/18 18:10:05, Bernhard Bauer wrote: > Don't directly quit message loops. Instead, use a content::MessageLoopRunner (or > in unit tests a RunLoop). I was following the pattern in this file. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1163: return NULL; On 2014/12/18 18:10:05, Bernhard Bauer wrote: > nullptr? Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1167: DCHECK(blocking_page); On 2014/12/18 18:10:06, Bernhard Bauer wrote: > This DCHECK is kind of unnecessary; If |blocking_page| is indeed null, the next > line will crash anyway. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:1624: render_frame_host->ExecuteJavaScript(base::ASCIIToUTF16("submitForm()")); On 2014/12/18 18:10:05, Bernhard Bauer wrote: > Could you use content::ExecuteScript() here? Done here and two other places. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_service.h (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_service.h:33: SKIP_OS_CHECK_FOR_TESTING, // The service can be enabled even if the OS has On 2014/12/18 18:10:06, Bernhard Bauer wrote: > The previous alignment was okay (two spaces before //), but it might look even > nicer if you aligned all the comments. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:69: var ssl = interstitialType === 'SSL'; On 2014/12/18 18:10:06, Bernhard Bauer wrote: > The === isn't really necessary here if you know that |interstitialType| is going > to be a string and not a weird object. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:91: sendCommand(CAPTIVEPORTAL_CMD_OPEN_LOGIN_PAGE); On 2014/12/18 18:10:06, Bernhard Bauer wrote: > This should be indented two more spaces. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:28: const int kDefaultInterstitialDisplayDelay = 2; On 2014/12/18 18:10:06, Bernhard Bauer wrote: > Add an "S" or "Seconds" suffix to clarify the unit? Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:49: base::TimeDelta SSLErrorHandler::interstitial_display_delay_ = On 2014/12/18 18:10:06, Bernhard Bauer wrote: > You could probably put these into the anonymous namespace if they're private and > static. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:54: NULL; On 2014/12/18 18:10:06, Bernhard Bauer wrote: > nullptr Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:106: if (timer_fired_callback_ && !timer_fired_callback_->is_null()) On 2014/12/18 18:10:06, Bernhard Bauer wrote: > Couldn't we simply make it an error to set |timer_fired_callback_| to a pointer > to a null callback? :) Added the check to SetInterstitialTimerFiredCallbackForTest. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:109: #endif On 2014/12/18 18:10:06, Bernhard Bauer wrote: > You could put the ShowSSLInterstitial() call into an #else instead of having > nonreachable code after the return. Hmm, there was some other logic inside the #if and the fallthrough made sense back then. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:182: delete this; On 2014/12/18 18:10:06, Bernhard Bauer wrote: > Another idea: If this class's lifetime is tied to the lifetime of the > WebContents anyway, you could make it a WebContentsUserData. That would allow > you to get rid of the self-deletions here (which at least make me a little > queasy). I thought of this before but never did it. Done now. There is still some manual deletion here in ShowSSLInterstitial and ShowCaptivePortalInterstitial where I destroy the instance, but other than that it's now a WebContentsUserData and the tests pass. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:45: ~SSLErrorHandler() override; On 2014/12/18 18:10:06, Bernhard Bauer wrote: > Please make this protected. If this class deletes itself, no one else should be > able to do that. > > Similarly for the constructor: If the correct way to use this class is through > HandleSSLError(), there is no need to make the constructor public. Done. https://codereview.chromium.org/318213002/diff/340001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:63: } On 2014/12/18 15:44:03, mmenke (OOO 12-20 to 1-4) wrote: > Can't the test just use content::WaitForInterstitialAttach? See > content/public/test/browser_test_utils.h. This is used in FastErrorWithInterstitialTimer where we trigger an SSL error and then leave the page hanging, so there is no interstitial to wait for. The only interesting event there to wait for is the timer firing. https://codereview.chromium.org/318213002/diff/340001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/318213002/diff/340001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:2574: 'browser/ssl/ssl_error_handler.cc', On 2014/12/18 18:10:06, Bernhard Bauer wrote: > Sort alphabetically please ("handler" before "info"). Done.
Thanks! https://codereview.chromium.org/318213002/diff/360001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:872: message_loop_runner_ = new content::MessageLoopRunner; You may want to initialize the message loop runner earlier, e.g. in the constructor. Otherwise the OnTimerFired might happen too early, and then you won't ever quit the message loop. It could be that it's guaranteed that OnTimerFired will run later, but setting up the message loop runner early is still the safer pattern (for example in case someone else copies this code). https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:34: static base::TimeDelta interstitial_display_delay = Are we sure this won't create a static initializer? Also, I think the "static" isn't necessary if you're in an anonymous namespace anyway. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:34: static base::TimeDelta interstitial_display_delay = Could you name these with a g_ prefix to make it clear they're global variables? Otherwise they're easily confused with local variables. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:93: if (timer_fired_callback) { Uh, I don't think this does what you think it does... Shouldn't this check |callback| instead, and also leave out the return? https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:75: static void CreateForWebContents(content::WebContents* web_contents, You could move this to an anonymous namespace in the implementation file. Or just inline it, as it's only used once.
Bauer: Can you PTAL? https://codereview.chromium.org/318213002/diff/360001/chrome/browser/captive_... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/captive_... chrome/browser/captive_portal/captive_portal_browsertest.cc:872: message_loop_runner_ = new content::MessageLoopRunner; On 2014/12/19 10:14:25, Bernhard Bauer wrote: > You may want to initialize the message loop runner earlier, e.g. in the > constructor. Otherwise the OnTimerFired might happen too early, and then you > won't ever quit the message loop. > > It could be that it's guaranteed that OnTimerFired will run later, but setting > up the message loop runner early is still the safer pattern (for example in case > someone else copies this code). Done. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:34: static base::TimeDelta interstitial_display_delay = On 2014/12/19 10:14:25, Bernhard Bauer wrote: > Could you name these with a g_ prefix to make it clear they're global variables? > Otherwise they're easily confused with local variables. Done. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:34: static base::TimeDelta interstitial_display_delay = On 2014/12/19 10:14:25, Bernhard Bauer wrote: > Are we sure this won't create a static initializer? Also, I think the "static" > isn't necessary if you're in an anonymous namespace anyway. Since there are only three different values, I made this an enum to be on the safe side. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:93: if (timer_fired_callback) { On 2014/12/19 10:14:25, Bernhard Bauer wrote: > Uh, I don't think this does what you think it does... Shouldn't this check > |callback| instead, and also leave out the return? Oh wow, good catch. I don't know what I was thinking. https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/360001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:75: static void CreateForWebContents(content::WebContents* web_contents, On 2014/12/19 10:14:25, Bernhard Bauer wrote: > You could move this to an anonymous namespace in the implementation file. Or > just inline it, as it's only used once. Inlined.
I'll defer to Bernhard for the rest of the review. https://codereview.chromium.org/318213002/diff/380001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/380001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:108: g_timer_fired_callback = callback; I think this is misnamed. A timer is fired when the callback is invoked, not when it's started (Hence my earlier comment, without even looking at this code). Fired->Started?
On 2014/12/19 19:07:51, mmenke (OOO 12-20 to 1-4) wrote: > I'll defer to Bernhard for the rest of the review. > > https://codereview.chromium.org/318213002/diff/380001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler.cc (right): > > https://codereview.chromium.org/318213002/diff/380001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler.cc:108: g_timer_fired_callback = callback; > I think this is misnamed. A timer is fired when the callback is invoked, not > when it's started (Hence my earlier comment, without even looking at this code). > Fired->Started? I see what you mean. I'll address this with the rest of the comments.
LGTM with some small nits: https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:57: if (delay == SSLErrorHandler::LONG) You could make this a switch statement, so the compiler will check that you have covered all the cases. https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:107: DCHECK(!callback->is_null()); You could probably move the callback check into the DCHECK, i.e. DCHECK(!callback || callback->is_null()) https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:47: ~SSLErrorHandler() override; You could make the destructor protected as well, but then again, there is no way to actually get an instance of this class (except for a subclass), so I guess it doesn't really make a difference.
LGTM with some small nits:
meacer@chromium.org changed reviewers: + isherman@chromium.org
isherman: Can you PTAL at histograms/histograms.xml? https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:57: if (delay == SSLErrorHandler::LONG) On 2014/12/20 11:21:23, Bernhard Bauer wrote: > You could make this a switch statement, so the compiler will check that you have > covered all the cases. Done. https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:107: DCHECK(!callback->is_null()); On 2014/12/20 11:21:23, Bernhard Bauer wrote: > You could probably move the callback check into the DCHECK, i.e. > DCHECK(!callback || callback->is_null()) Done. https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/400001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:47: ~SSLErrorHandler() override; On 2014/12/20 11:21:23, Bernhard Bauer wrote: > You could make the destructor protected as well, but then again, there is no way > to actually get an instance of this class (except for a subclass), so I guess it > doesn't really make a difference. Done.
https://codereview.chromium.org/318213002/diff/440001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:69: var ssl = interstitialType == 'SSL'; is there a reason you changed this from === to ==? https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:97: if (command == std::string("\"") + kOpenLoginPageCommand + "\"") { Why is one of these std::string("\"") and the other simply "\""? https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:21: // shown when a captive portal and an SSL error is detected. nit: "...that is shown when a captive portal triggers an SSL error" (or similar) https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:30: static const void* kTypeForTesting; If this is only for testing, can it be protected or private w/ some friend classes? Same goes for GetTypeForTesting() below. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:85: static bool IsOverridable(int options_mask); At first I expected this to be a getter for overridable_, but now I see that it's static and used to calculate the value of overridable_. Can you either rename to CalculateIsOverridable or add a comment explaining? https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:36: SHOW_CAPTIVE_PORTAL_INTERSTITIAL, I can imagine this working two ways: (A) SHOW_CAPTIVE_PORTAL_INTERSTITIAL is a superset of SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE (B) SHOW_CAPTIVE_PORTAL_INTERSTITIAL + SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE = HANDLE_ALL Looking at the code, I see it's (B). To make this more clear, can you name them in a way to make them obviously disjoint? I.e., SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:38: SHOW_SSL_INTERSTITIAL, Same request for SHOW_SSL_INTERSTITIAL (turn it into SHOW_SSL_INTERSTITIAL_NONOVERRIDABLE) https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:58: switch (delay) { Suggestion: assign values in seconds to InterstitialDelayType enum (2 for normal, 0 for short/none, and 3600 for long). Then you don't also need kDefaultInterstitialDisplayDelayInSeconds, and you can just do base::TimeDelta::FromSeconds(delay). https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:151: g_timer_started_callback->Run(web_contents_); let's say g_timer_started_callback is null for some reason. What happens then? should there be a DCHECK instead? https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:43: SHORT, // Very short interstitial timer delay (ie. zero), used in tests. If this is zero, should it be named "NONE"? https://codereview.chromium.org/318213002/diff/440001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/318213002/diff/440001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56654: + <int value="0" label="HANDLE_ALL"/> Can the label make it clear that this is the total and everything else is a subset of it?
histograms lgm, thanks.
On 2015/01/06 00:23:56, Ilya Sherman wrote: > histograms lgm, thanks. Thanks Ilya, but did you mean to lgtm instead? :)
On 2015/01/06 09:34:37, Mustafa Acer (OOO until Jan13) wrote: > On 2015/01/06 00:23:56, Ilya Sherman wrote: > > histograms lgm, thanks. > > Thanks Ilya, but did you mean to lgtm instead? :) Whoops, I did indeed. LGTM.
felt: PTAL? https://codereview.chromium.org/318213002/diff/440001/chrome/browser/resource... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/resource... chrome/browser/resources/security_warnings/interstitial_v2.js:69: var ssl = interstitialType == 'SSL'; On 2014/12/28 15:40:52, felt wrote: > is there a reason you changed this from === to ==? Changed this upon bauerb's comment: https://codereview.chromium.org/318213002/diff/340001/chrome/browser/resource... https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:97: if (command == std::string("\"") + kOpenLoginPageCommand + "\"") { On 2014/12/28 15:40:52, felt wrote: > Why is one of these std::string("\"") and the other simply "\""? The first one needs to be a string so that the expression is string + char* + char*. The last one doesn't need to be string since string has a + operator that accepts char*. I don't mind making the second one string though, doesn't really change the meaning. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:21: // shown when a captive portal and an SSL error is detected. On 2014/12/28 15:40:52, felt wrote: > nit: "...that is shown when a captive portal triggers an SSL error" (or similar) Done. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:30: static const void* kTypeForTesting; On 2014/12/28 15:40:52, felt wrote: > If this is only for testing, can it be protected or private w/ some friend > classes? Same goes for GetTypeForTesting() below. If we do this, then we should do the same for SSLBlockingPage and SafeBrowsingBlockingPage as well. Do you mind if I do this in a later CL? As for GetTypeForTesting, we want it to be available in the SecurityInterstitialPage interface so that we can check the type without having to cast the interstitial page. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:85: static bool IsOverridable(int options_mask); On 2014/12/28 15:40:52, felt wrote: > At first I expected this to be a getter for overridable_, but now I see that > it's static and used to calculate the value of overridable_. Can you either > rename to CalculateIsOverridable or add a comment explaining? Calculate seemed to not imply that it returns bool, so renamed to IsOptionsOverridable and added a comment. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:36: SHOW_CAPTIVE_PORTAL_INTERSTITIAL, On 2014/12/28 15:40:52, felt wrote: > I can imagine this working two ways: > > (A) SHOW_CAPTIVE_PORTAL_INTERSTITIAL is a superset of > SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE > > (B) SHOW_CAPTIVE_PORTAL_INTERSTITIAL + > SHOW_CAPTIVE_PORTAL_INTERSTITIAL_OVERRIDABLE = HANDLE_ALL > > Looking at the code, I see it's (B). To make this more clear, can you name them > in a way to make them obviously disjoint? I.e., > SHOW_CAPTIVE_PORTAL_INTERSTITIAL_NONOVERRIDABLE. B is correct. Renamed to NONOVERRIDABLE. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:38: SHOW_SSL_INTERSTITIAL, On 2014/12/28 15:40:52, felt wrote: > Same request for SHOW_SSL_INTERSTITIAL (turn it into > SHOW_SSL_INTERSTITIAL_NONOVERRIDABLE) Done. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:58: switch (delay) { On 2014/12/28 15:40:52, felt wrote: > Suggestion: assign values in seconds to InterstitialDelayType enum (2 for > normal, 0 for short/none, and 3600 for long). Then you don't also need > kDefaultInterstitialDisplayDelayInSeconds, and you can just do > base::TimeDelta::FromSeconds(delay). Hmm, I feel like that'll be a bit confusing: enum InterstitialDelayType { NORMAL = 2, NONE = 0, LONG = 3600 } It doesn't say that the delay is in terms of seconds, so we'll need a comment or rename the interstitial delay. And if we want to rename it, we could just remove the enum and instead add a SetInterstitialDelayInSeconds which I wanted to avoid since it doesn't communicate whether the delay is short, long or normal. Also, the enum order will be a bit odd (I'd like to keep NORMAL at the beginning to imply that it's the default). I'm inclined to keep this as is. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:151: g_timer_started_callback->Run(web_contents_); On 2014/12/28 15:40:52, felt wrote: > let's say g_timer_started_callback is null for some reason. What happens then? > should there be a DCHECK instead? Can't be NULL here since SetInterstitialTimerStartedCallbackForTest checks for NULL (it's the only setter for g_timer_started_callback). I moved the NULL check from here to the setter. https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:43: SHORT, // Very short interstitial timer delay (ie. zero), used in tests. On 2014/12/28 15:40:52, felt wrote: > If this is zero, should it be named "NONE"? Done. https://codereview.chromium.org/318213002/diff/440001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/318213002/diff/440001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56654: + <int value="0" label="HANDLE_ALL"/> On 2014/12/28 15:40:52, felt wrote: > Can the label make it clear that this is the total and everything else is a > subset of it? Does making the distinction between nonoverridable/overridable imply that? Or did you have another suggestion?
lgtm https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.cc (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.cc:97: if (command == std::string("\"") + kOpenLoginPageCommand + "\"") { On 2015/01/08 14:29:16, Mustafa Acer (OOO until Jan13) wrote: > On 2014/12/28 15:40:52, felt wrote: > > Why is one of these std::string("\"") and the other simply "\""? > > The first one needs to be a string so that the expression is string + char* + > char*. The last one doesn't need to be string since string has a + operator that > accepts char*. I don't mind making the second one string though, doesn't really > change the meaning. Ahhh, that makes sense, thanks! https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:30: static const void* kTypeForTesting; On 2015/01/08 14:29:16, Mustafa Acer (OOO until Jan13) wrote: > On 2014/12/28 15:40:52, felt wrote: > > If this is only for testing, can it be protected or private w/ some friend > > classes? Same goes for GetTypeForTesting() below. > > If we do this, then we should do the same for SSLBlockingPage and > SafeBrowsingBlockingPage as well. Do you mind if I do this in a later CL? > > As for GetTypeForTesting, we want it to be available in the > SecurityInterstitialPage interface so that we can check the type without having > to cast the interstitial page. Yeah, no need to do it here but I generally think anything ForTesting should be protected. (Can't make it private but protected should work right?)
https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... File chrome/browser/ssl/captive_portal_blocking_page.h (right): https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... chrome/browser/ssl/captive_portal_blocking_page.h:30: static const void* kTypeForTesting; On 2015/01/08 16:47:44, felt wrote: > On 2015/01/08 14:29:16, Mustafa Acer (OOO until Jan13) wrote: > > On 2014/12/28 15:40:52, felt wrote: > > > If this is only for testing, can it be protected or private w/ some friend > > > classes? Same goes for GetTypeForTesting() below. > > > > If we do this, then we should do the same for SSLBlockingPage and > > SafeBrowsingBlockingPage as well. Do you mind if I do this in a later CL? > > > > As for GetTypeForTesting, we want it to be available in the > > SecurityInterstitialPage interface so that we can check the type without > having > > to cast the interstitial page. > > Yeah, no need to do it here but I generally think anything ForTesting should be > protected. (Can't make it private but protected should work right?) If you actually name it ...ForTesting, there's a presubmit check that makes sure you only call it from testing code.
On 2015/01/08 17:25:14, Bernhard Bauer wrote: > https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... > File chrome/browser/ssl/captive_portal_blocking_page.h (right): > > https://codereview.chromium.org/318213002/diff/440001/chrome/browser/ssl/capt... > chrome/browser/ssl/captive_portal_blocking_page.h:30: static const void* > kTypeForTesting; > On 2015/01/08 16:47:44, felt wrote: > > On 2015/01/08 14:29:16, Mustafa Acer (OOO until Jan13) wrote: > > > On 2014/12/28 15:40:52, felt wrote: > > > > If this is only for testing, can it be protected or private w/ some friend > > > > classes? Same goes for GetTypeForTesting() below. > > > > > > If we do this, then we should do the same for SSLBlockingPage and > > > SafeBrowsingBlockingPage as well. Do you mind if I do this in a later CL? > > > > > > As for GetTypeForTesting, we want it to be available in the > > > SecurityInterstitialPage interface so that we can check the type without > > having > > > to cast the interstitial page. > > > > Yeah, no need to do it here but I generally think anything ForTesting should > be > > protected. (Can't make it private but protected should work right?) > > If you actually name it ...ForTesting, there's a presubmit check that makes sure > you only call it from testing code. > Yeah, no need to do it here but I generally think anything ForTesting should be protected. (Can't make it private but protected should work right?) I don't think that would work either, since CaptivePortalBrowserTest::GetInterstitialType calls SecurityInterstitialPage::GetInterstitialType. But perhaps I'm misunderstanding your suggestion?
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/318213002/480001
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/4ef065e92f5822c7b0e73ed01433978c0566954f Cr-Commit-Position: refs/heads/master@{#310697} |
