ChromeOS: Implement CaptivePortalAuthenticationIgnoresProxy policy.

chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc

Index: chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
diff --git a/chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc b/chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
index b34cf1ee7c1170726bd7f79d5a03c90d46c75115..ffbf860e86c11fd899b19c7cf92b48fd2d6d403d 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector_impl_browsertest.cc
@@ -5,12 +5,15 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/login_manager_test.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/net/network_portal_detector_impl.h"
 #include "chrome/browser/chromeos/net/network_portal_detector_test_utils.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/common/pref_names.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_service_client.h"
@@ -141,6 +144,16 @@ class NetworkPortalDetectorImplBrowserTest
 
   MessageCenter* message_center() { return MessageCenter::Get(); }
 
+  void SetIgnoreNoNetworkForTesting() {
+    network_portal_detector_->notification_controller_

[bartfab (slow), 2015/01/07 17:38:42]

Nit: #include
"chrome/browser/chromeos/net/network_portal_notification_controller.h"

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+        .SetIgnoreNoNetworkForTesting();
+  }
+
+  const NetworkPortalWebDialog* GetDialog() const {

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Forward-declare |NetworkPortalWebDialog|.

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+    return network_portal_detector_->notification_controller_
+        .GetDialogForTesting();
+  }
+
  private:
   NetworkPortalDetectorImpl* network_portal_detector_;
 
@@ -203,4 +216,105 @@ IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTest,
       action_checker.Expect(Controller::USER_ACTION_METRIC_CLOSED, 1)->Check());
 }
 
+class NetworkPortalDetectorImplBrowserTestIgnoreProxy
+    : public NetworkPortalDetectorImplBrowserTest {
+ public:
+  NetworkPortalDetectorImplBrowserTestIgnoreProxy()
+      : NetworkPortalDetectorImplBrowserTest() {}
+
+  virtual ~NetworkPortalDetectorImplBrowserTestIgnoreProxy() {}

[bartfab (slow), 2015/01/07 17:38:42]

Nit 1: Use |override|, not |virtual|.
Nit 2: There is actually no need to override the destructor at all, since you
are leaving it empty.

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+
+  void SetUpCommandLine(CommandLine* command_line) override {
+    NetworkPortalDetectorImplBrowserTest::SetUpCommandLine(command_line);
+
+    command_line->AppendSwitch(

[bartfab (slow), 2015/01/07 17:38:42]

Nit: #include "base/command_line.h"

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+        chromeos::switches::kEnableCaptivePortalBypassProxy);
+  }
+
+ protected:
+  void TestImpl(const bool preference_value);
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkPortalDetectorImplBrowserTestIgnoreProxy);
+};
+
+void NetworkPortalDetectorImplBrowserTestIgnoreProxy::TestImpl(
+    const bool preference_value) {
+  typedef NetworkPortalNotificationController Controller;

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Use using=, not typedef.

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+
+  TestObserver observer;
+
+  EnumHistogramChecker ui_checker(
+      kNotificationMetric, Controller::NOTIFICATION_METRIC_COUNT, nullptr);
+  EnumHistogramChecker action_checker(
+      kUserActionMetric, Controller::USER_ACTION_METRIC_COUNT, nullptr);
+
+  LoginUser(kTestUser);
+  content::RunAllPendingInMessageLoop();
+
+  SetIgnoreNoNetworkForTesting();
+
+  ProfileManager::GetActiveUserProfile()->GetPrefs()->SetBoolean(

[bartfab (slow), 2015/01/07 17:38:42]

Nit 1: #include "chrome/browser/profiles/profile_manager.h"
Nit 2: #include "chrome/browser/profiles/profile.h"
Nit 3: #include "chrome/browser/prefs/pref_service_syncable.h"

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+      prefs::kCaptivePortalAuthenticationIgnoresProxy, preference_value);

[bartfab (slow), 2015/01/07 17:38:42]

Nit: #include "chrome/common/pref_names.h"

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+
+  // User connects to wifi.
+  SetConnected(kWifiServicePath);
+
+  ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_SESSION, strategy()->Id());

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+
+  // No notification until portal detection is completed.
+  ASSERT_FALSE(message_center()->FindVisibleNotificationById(kNotificationId));

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+  RestartDetection();
+  CompleteURLFetch(net::OK, 200, nullptr);
+
+  // Check that wifi is marked as behind the portal and that notification

[bartfab (slow), 2015/01/07 17:38:42]

Nit 1: s/wifi/WiFi/
Nit 2: s/the/a/
Nit 3: s/notification/a notification/

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+  // is displayed.
+  ASSERT_TRUE(message_center()->FindVisibleNotificationById(kNotificationId));

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+  ASSERT_EQ(

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+      NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL,
+      NetworkPortalDetector::Get()->GetCaptivePortalState(kWifiGuid).status);
+
+  // Wait until notification is displayed.
+  observer.WaitAndReset();
+
+  ASSERT_TRUE(

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+      ui_checker.Expect(Controller::NOTIFICATION_METRIC_DISPLAYED, 1)->Check());
+  ASSERT_TRUE(action_checker.Check());

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+
+  message_center()->ClickOnNotification(kNotificationId);
+
+  base::RunLoop().RunUntilIdle();

[bartfab (slow), 2015/01/07 17:38:42]

Nit: It is strange to use |base::RunLoop| and
|content::RunAllPendingInMessageLoop()| in the same method. The latter is
preferable in browser tests.

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+
+  if (preference_value) {
+    // Check that dialog is displayed.
+    EXPECT_TRUE(GetDialog());
+  } else {
+    // Check that dialog is not displayed.
+    EXPECT_FALSE(GetDialog());
+  }

[bartfab (slow), 2015/01/07 17:38:42]

Nit: This entire conditional can be collapsed to:

EXPECT_EQ(preference_value, GetDialog());

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,
+                       PRE_CaptivePortalAuthenticationIgnoresProxyTrue) {
+  RegisterUser(kTestUser);
+  StartupUtils::MarkOobeCompleted();
+  ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,

[ygorshenin1, 2015/01/12 10:34:49]

nit: instead of two explicitly defined test fixtures you can try to reuse
IN_PROC_BROWSER_TEST_P:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

[Alexander Alekseev, 2015/01/12 18:41:09]

Done.

+                       CaptivePortalAuthenticationIgnoresProxyTrue) {
+  TestImpl(true);
+}
+
+IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,
+                       PRE_CaptivePortalAuthenticationIgnoresProxyFalse) {
+  RegisterUser(kTestUser);
+  StartupUtils::MarkOobeCompleted();
+  ASSERT_EQ(PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN, strategy()->Id());

[bartfab (slow), 2015/01/07 17:38:42]

Nit: Why ASSERT, not EXPECT?

[Alexander Alekseev, 2015/01/12 18:41:08]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(NetworkPortalDetectorImplBrowserTestIgnoreProxy,
+                       CaptivePortalAuthenticationIgnoresProxyFalse) {
+  TestImpl(false);
+}
+
 }  // namespace chromeos