Enable captive portal detection by default, but keep it

disabled for most browser tests.

Also remove the option from about:flags.  It can still be
disabled via --captive-portal-detection=0 or by deselecting
"use a web service to resolve navigation errors".

BUG=87100

Review URL: https://chromiumcodereview.appspot.com/10795038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148764 0039d316-1c4b-4281-b951-d872f2087c98

chrome/app/generated_resources.grd

@@ -5928,12 +5928,6 @@ Keep your key file in a safe place. You will need it to create new versions of y
         </message>
       </if>
 
-      <message name="IDS_FLAGS_CAPTIVE_PORTAL_CHECK_ON_ERROR_NAME" desc="Name for the flag to enable automatic checking for captive portals.">
-        Enable captive portal detection.
-      </message>
-      <message name="IDS_FLAGS_CAPTIVE_PORTAL_CHECK_ON_ERROR_DESCRIPTION" desc="Description for the flag to enable automatic checking for captive portals.">
-        Check for captive portals and open a new tab at the login page if one is found.
-      </message>
       <message name="IDS_FLAGS_OLD_CHECKBOX_STYLE" desc="Name of the flag to turn off the new checkbox style.">
         Disable new checkbox style.
       </message>

chrome/browser/about_flags.cc

@@ -675,13 +675,6 @@ const Experiment kExperiments[] = {
     kOsAll,
     SINGLE_VALUE_TYPE(switches::kDisableChromeToMobile)
   },
-  {
-    "enable-captive-portal-detection",
-    IDS_FLAGS_CAPTIVE_PORTAL_CHECK_ON_ERROR_NAME,
-    IDS_FLAGS_CAPTIVE_PORTAL_CHECK_ON_ERROR_DESCRIPTION,
-    kOsMac | kOsWin | kOsLinux | kOsCrOS,
-    SINGLE_VALUE_TYPE(switches::kCaptivePortalDetection)
-  },
 #if defined(GOOGLE_CHROME_BUILD)
   {
     "disable-asynchronous-spellchecking",

chrome/browser/captive_portal/captive_portal_browsertest.cc

@@ -761,7 +761,6 @@ class CaptivePortalBrowserTest : public InProcessBrowserTest {
   CaptivePortalBrowserTest();
 
   // InProcessBrowserTest:
-  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE;
   virtual void SetUpOnMainThread() OVERRIDE;
   virtual void CleanUpOnMainThread() OVERRIDE;
 
@@ -898,6 +897,11 @@ void CaptivePortalBrowserTest::SetUpOnMainThread() {
       base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true));
   URLRequestMockCaptivePortalJobFactory::AddUrlHandlers();
 
+  // Double-check that the captive portal service isn't enabled by default for
+  // browser tests.
+  EXPECT_TRUE(CaptivePortalService::is_disabled_for_testing());
+
+  CaptivePortalService::set_is_disabled_for_testing(false);
   EnableCaptivePortalDetection(browser()->profile(), true);
 
   // Set the captive portal service to use URLRequestMockCaptivePortalJob's
@@ -911,11 +915,6 @@ void CaptivePortalBrowserTest::CleanUpOnMainThread() {
   EXPECT_FALSE(CheckPending(browser()));
 }
 
-void CaptivePortalBrowserTest::SetUpCommandLine(
-    CommandLine* command_line) {
-  command_line->AppendSwitch(switches::kCaptivePortalDetection);
-}
-
 void CaptivePortalBrowserTest::EnableCaptivePortalDetection(
     Profile* profile, bool enabled) {
   profile->GetPrefs()->SetBoolean(prefs::kAlternateErrorPagesEnabled, enabled);

chrome/browser/captive_portal/captive_portal_service.cc

@@ -13,7 +13,6 @@
 #include "base/string_number_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_notification_types.h"
-#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 #include "net/base/load_flags.h"
@@ -95,6 +94,8 @@ void RecordRepeatHistograms(Result result,
 
 }  // namespace
 
+bool CaptivePortalService::is_disabled_for_testing_ = false;
+
 class CaptivePortalService::RecheckBackoffEntry : public net::BackoffEntry {
  public:
   explicit RecheckBackoffEntry(CaptivePortalService* captive_portal_service)
@@ -326,9 +327,8 @@ void CaptivePortalService::ResetBackoffEntry(Result result) {
 
 void CaptivePortalService::UpdateEnabledState() {
   bool enabled_before = enabled_;
-  enabled_ = resolve_errors_with_web_service_.GetValue() &&
-             CommandLine::ForCurrentProcess()->HasSwitch(
-                 switches::kCaptivePortalDetection);
+  enabled_ = !is_disabled_for_testing_ &&
+             resolve_errors_with_web_service_.GetValue();
   if (enabled_before == enabled_)
     return;
 

chrome/browser/captive_portal/captive_portal_service.h

@@ -76,6 +76,13 @@ class CaptivePortalService : public ProfileKeyedService,
   // checks return INTERNET_CONNECTED.
   bool enabled() const { return enabled_; }
 
+  // Used to disable captive portal detection so it doesn't interfere with
+  // tests.  Should be called before the service is created.
+  static void set_is_disabled_for_testing(bool is_disabled_for_testing) {
+    is_disabled_for_testing_ = is_disabled_for_testing;
+  }
+  static bool is_disabled_for_testing() { return is_disabled_for_testing_; }
+
  private:
   friend class CaptivePortalServiceTest;
   friend class CaptivePortalBrowserTest;
@@ -211,6 +218,8 @@ class CaptivePortalService : public ProfileKeyedService,
 
   base::OneShotTimer<CaptivePortalService> check_captive_portal_timer_;
 
+  static bool is_disabled_for_testing_;
+
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalService);
 };
 

chrome/browser/captive_portal/captive_portal_service_unittest.cc

@@ -132,15 +132,21 @@ class CaptivePortalObserver : public content::NotificationObserver {
 
 class CaptivePortalServiceTest : public testing::Test {
  public:
-  CaptivePortalServiceTest() {}
+  CaptivePortalServiceTest()
+      : was_service_disabled_for_testing_(
+            CaptivePortalService::is_disabled_for_testing()) {
+  }
 
-  virtual ~CaptivePortalServiceTest() {}
+  virtual ~CaptivePortalServiceTest() {
+    CaptivePortalService::set_is_disabled_for_testing(
+        was_service_disabled_for_testing_);
+  }
 
-  void Initialize(bool enable_on_command_line) {
-    if (enable_on_command_line) {
-      CommandLine::ForCurrentProcess()->AppendSwitch(
-          switches::kCaptivePortalDetection);
-    }
+  // |enable_service| is whether or not the captive portal service itself
+  // should be disabled.  This is different from enabling the captive portal
+  // detection preference.
+  void Initialize(bool enable_service) {
+    CaptivePortalService::set_is_disabled_for_testing(!enable_service);
 
     profile_.reset(new TestingProfile());
     service_.reset(new TestCaptivePortalService(profile_.get()));
@@ -161,11 +167,11 @@ class CaptivePortalServiceTest : public testing::Test {
     // captive portal test in a row that ends up with the same result.
     set_num_errors_to_ignore(0);
 
-    EnableCaptivePortalDetection(true);
+    EnableCaptivePortalDetectionPreference(true);
   }
 
   // Sets the captive portal checking preference.
-  void EnableCaptivePortalDetection(bool enabled) {
+  void EnableCaptivePortalDetectionPreference(bool enabled) {
     profile()->GetPrefs()->SetBoolean(prefs::kAlternateErrorPagesEnabled,
                                       enabled);
   }
@@ -323,6 +329,10 @@ class CaptivePortalServiceTest : public testing::Test {
   TestCaptivePortalService* service() { return service_.get(); }
 
  private:
+  // Stores the initial value of CaptivePortalService::is_disabled_for_tests
+  // so it can be restored after the test.
+  const bool was_service_disabled_for_testing_;
+
   MessageLoop message_loop_;
 
   // Note that the construction order of these matters.
@@ -435,13 +445,13 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabled) {
 
   set_initial_backoff_portal(base::TimeDelta::FromSeconds(100));
 
-  EnableCaptivePortalDetection(false);
+  EnableCaptivePortalDetectionPreference(false);
 
   RunDisabledTest(0);
   for (int i = 0; i < 6; ++i)
     RunDisabledTest(100);
 
-  EnableCaptivePortalDetection(true);
+  EnableCaptivePortalDetectionPreference(true);
 
   RunTest(RESULT_BEHIND_CAPTIVE_PORTAL, net::OK, 200, 0, NULL);
 }
@@ -460,7 +470,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabledWhileRunning) {
   EXPECT_TRUE(FetchingURL());
   EXPECT_FALSE(TimerRunning());
 
-  EnableCaptivePortalDetection(false);
+  EnableCaptivePortalDetectionPreference(false);
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());
   EXPECT_EQ(0, observer.num_results_received());
@@ -488,7 +498,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabledWhilePending) {
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());
 
-  EnableCaptivePortalDetection(false);
+  EnableCaptivePortalDetectionPreference(false);
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());
   EXPECT_EQ(0, observer.num_results_received());
@@ -507,7 +517,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabledWhilePending) {
 TEST_F(CaptivePortalServiceTest, CaptivePortalPrefEnabledWhilePending) {
   Initialize(true);
 
-  EnableCaptivePortalDetection(false);
+  EnableCaptivePortalDetectionPreference(false);
   RunDisabledTest(0);
 
   CaptivePortalObserver observer(profile(), service());
@@ -517,7 +527,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefEnabledWhilePending) {
 
   net::TestURLFetcherFactory factory;
 
-  EnableCaptivePortalDetection(true);
+  EnableCaptivePortalDetectionPreference(true);
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());
 
@@ -535,8 +545,8 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefEnabledWhilePending) {
   EXPECT_EQ(RESULT_BEHIND_CAPTIVE_PORTAL, observer.captive_portal_result());
 }
 
-// Checks that disabling with a command line flag works as expected.
-TEST_F(CaptivePortalServiceTest, CaptivePortalDisabledAtCommandLine) {
+// Checks that disabling for browser tests works as expected.
+TEST_F(CaptivePortalServiceTest, CaptivePortalDisableForTests) {
   Initialize(false);
   RunDisabledTest(0);
 }

chrome/common/chrome_switches.cc

@@ -134,10 +134,6 @@ const char kAutomationReinitializeOnChannelError[] =
 // existing BrowserWindow Panels.
 const char kBrowserlessPanels[] = "browserless-panels";
 
-// This enables automatic captive portal checking on certain network errors.
-// If a captive portal is detected, a login tab will be opened.
-const char kCaptivePortalDetection[]        = "enable-captive-portal-detection";
-
 // How often (in seconds) to check for updates. Should only be used for testing
 // purposes.
 const char kCheckForUpdateIntervalSec[]     = "check-for-update-interval";

chrome/common/chrome_switches.h

@@ -51,7 +51,6 @@ extern const char kAutoLaunchAtStartup[];
 extern const char kAutomationClientChannelID[];
 extern const char kAutomationReinitializeOnChannelError[];
 extern const char kBrowserlessPanels[];
-extern const char kCaptivePortalDetection[];
 extern const char kCheckForUpdateIntervalSec[];
 extern const char kCheckCloudPrintConnectorPolicy[];
 extern const char kChromeFrameShutdownDelay[];

chrome/test/base/in_process_browser_test.cc

@@ -14,6 +14,7 @@
 #include "base/string_number_conversions.h"
 #include "base/test/test_file_util.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/captive_portal/captive_portal_service.h"
 #include "chrome/browser/io_thread.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile.h"
@@ -145,6 +146,11 @@ void InProcessBrowserTest::SetUp() {
   // time code is directly executed.
   autorelease_pool_ = new base::mac::ScopedNSAutoreleasePool;
 #endif
+
+#if defined(ENABLE_CAPTIVE_PORTAL_DETECTION)
+  captive_portal::CaptivePortalService::set_is_disabled_for_testing(true);
+#endif
+
   BrowserTestBase::SetUp();
 }
 
@@ -175,8 +181,8 @@ void InProcessBrowserTest::PrepareTestCommandLine(CommandLine* command_line) {
   command_line->AppendSwitch(switches::kDisableZeroBrowsersOpenForTests);
 
   if (!command_line->HasSwitch(switches::kHomePage)) {
-      command_line->AppendSwitchASCII(
-          switches::kHomePage, chrome::kAboutBlankURL);
+    command_line->AppendSwitchASCII(
+        switches::kHomePage, chrome::kAboutBlankURL);
   }
   if (command_line->GetArgs().empty())
     command_line->AppendArg(chrome::kAboutBlankURL);