Add custom interstitial for captive portals.

chrome/browser/ssl/ssl_error_handler_unittest.cc

Index: chrome/browser/ssl/ssl_error_handler_unittest.cc
diff --git a/chrome/browser/ssl/ssl_error_handler_unittest.cc b/chrome/browser/ssl/ssl_error_handler_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..483a1bc5acad3828530492b0431ff9ee9c8f4a56
--- /dev/null
+++ b/chrome/browser/ssl/ssl_error_handler_unittest.cc
@@ -0,0 +1,154 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.

[mmenke, 2014/06/24 18:09:27]

nit:  Remove "(c)"

[meacer, 2014/10/22 23:04:30]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ssl/ssl_error_handler.h"
+
+#include "base/callback.h"
+#include "base/message_loop/message_loop.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "net/ssl/ssl_info.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+

[mmenke, 2014/06/24 18:09:27]

nit:  Remove extre blank line.

[meacer, 2014/10/22 23:04:29]

Done.

+class TestSSLErrorHandler : public SSLErrorHandler {
+ public:
+  TestSSLErrorHandler(content::WebContents* web_contents,
+                      const net::SSLInfo& ssl_info,
+                      bool overridable)
+      : SSLErrorHandler(web_contents,
+                        -1,
+                        ssl_info,
+                        GURL(),
+                        overridable,
+                        false,
+                        base::Callback<void(bool)>()) {
+  }
+
+  virtual ~TestSSLErrorHandler() {
+  }
+
+  void set_ssl_interstitial_display_delay(base::TimeDelta delay) {

[mmenke, 2014/06/24 18:09:27]

Is this method needed?  Seems like you can just call it in the constructor with
a delay of 0 seconds.

[meacer, 2014/10/22 23:04:29]

Done.

+    EXPECT_FALSE(TimerRunning());
+    SSLErrorHandler::set_ssl_interstitial_display_delay(delay);
+  }
+
+  bool TimerRunning() {
+    return timer_.IsRunning();
+  }
+
+  MOCK_METHOD0(CheckForCaptivePortal, void());
+  MOCK_METHOD0(ShowCaptivePortalInterstitial, void());
+  MOCK_METHOD0(ShowSSLInterstitial, void());
+ private:

[mmenke, 2014/06/24 18:09:27]

nit:  blank line before private section.

[meacer, 2014/10/22 23:04:29]

Done.

+  DISALLOW_COPY_AND_ASSIGN(TestSSLErrorHandler);
+};
+
+class SSLErrorHandlerTest : public ChromeRenderViewHostTestHarness {
+ public:
+  // testing::Test:
+  virtual void SetUp() OVERRIDE {
+    ChromeRenderViewHostTestHarness::SetUp();
+    ssl_info_.reset(new net::SSLInfo);
+    error_handler_.reset(
+        new testing::StrictMock<TestSSLErrorHandler>(web_contents(),
+                                                     *ssl_info_.get(),
+                                                     false));
+    error_handler_->set_ssl_interstitial_display_delay(base::TimeDelta());
+  }
+
+  virtual void TearDown() OVERRIDE {
+    EXPECT_FALSE(error_handler().TimerRunning());
+    error_handler_.reset(NULL);
+    ssl_info_.reset(NULL);
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  TestSSLErrorHandler& error_handler() { return *error_handler_.get(); }
+
+ private:
+  scoped_ptr<net::SSLInfo> ssl_info_;

[mmenke, 2014/06/24 18:09:27]

Does this need to be a scoped_ptr?

[meacer, 2014/10/22 23:04:29]

Done.

+  scoped_ptr<TestSSLErrorHandler> error_handler_;
+};
+
+#ifdef ENABLE_CAPTIVE_PORTAL_DETECTION
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnTimerExpired) {

[mmenke, 2014/06/24 18:09:27]

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.

[meacer, 2014/10/22 23:04:30]

Done.

+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_CALL(error_handler(), CheckForCaptivePortal()).Times(1);
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().TimerRunning());
+  EXPECT_FALSE(error_handler().handled());
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_TRUE(error_handler().handled());
+
+  // Any captive portal result after the timer expired should be ignored.
+  error_handler().OnCaptivePortalResult();
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(0);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);
+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowCustomInterstitialOnCaptivePortalResult) {
+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_CALL(error_handler(), CheckForCaptivePortal()).Times(1);
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().TimerRunning());
+  EXPECT_FALSE(error_handler().handled());
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(0);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(1);
+  // Fake a captive portal result.
+  error_handler().OnCaptivePortalResult();
+  base::MessageLoop::current()->RunUntilIdle();
+
+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_TRUE(error_handler().handled());
+
+  // Any captive portal result after the timer expired should be ignored.
+  error_handler().OnCaptivePortalResult();
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(0);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);

[mmenke, 2014/06/24 18:09:27]

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).

[meacer, 2014/10/22 23:04:29]

Done.

+}
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnTooEarlyCaptivePortalResult) {
+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_CALL(error_handler(), CheckForCaptivePortal()).Times(1);
+
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(0);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);
+  // A captive portal result arrives before error handler starts.
+  error_handler().OnCaptivePortalResult();
+
+  EXPECT_FALSE(error_handler().handled());
+  EXPECT_FALSE(error_handler().TimerRunning());
+
+  error_handler().Handle();
+
+  EXPECT_TRUE(error_handler().TimerRunning());
+  EXPECT_FALSE(error_handler().handled());
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);
+  base::MessageLoop::current()->RunUntilIdle();
+}
+
+#else
+
+TEST_F(SSLErrorHandlerTest,
+       ShouldShowSSLInterstitialOnCaptivePortalDetectionDisabled) {
+  EXPECT_FALSE(error_handler().TimerRunning());
+  EXPECT_CALL(error_handler(), ShowSSLInterstitial()).Times(1);
+  EXPECT_CALL(error_handler(), CheckForCaptivePortal()).Times(0);
+  EXPECT_CALL(error_handler(), ShowCaptivePortalInterstitial()).Times(0);
+  error_handler().Handle();
+}
+
+#endif