Portal detection is initiated if shill connection state for active network was changed.

BUG=166973
TEST=Manual tests on Lumpy


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

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

6 files changed, 116 insertions, 30 deletions

diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index ab07519..30cd45e 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -228,7 +228,8 @@ Network::Network(const std::string& service_path,
       notify_failure_(false),
       profile_type_(PROFILE_NONE),
       service_path_(service_path),
-      type_(type) {
+      type_(type),
+      is_behind_portal_for_testing_(false) {
 }
 
 Network::~Network() {
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index 17cb2d3..bac4976 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -322,7 +322,12 @@ class Network {
   class TestApi {
    public:
     explicit TestApi(Network* network) : network_(network) {}
+    void SetBehindPortal() {
+      network_->set_is_behind_portal_for_testing(true);
+      network_->set_behind_portal();
+    }
     void SetConnected() {
+      network_->set_is_behind_portal_for_testing(false);
       network_->set_connected();
     }
     void SetConnecting() {
@@ -552,6 +557,15 @@ class Network {
   void set_connecting() {
     state_ = STATE_CONNECT_REQUESTED;
   }
+  void set_is_behind_portal_for_testing(bool value) {
+    is_behind_portal_for_testing_ = value;
+  }
+  bool is_behind_portal_for_testing() const {
+    return is_behind_portal_for_testing_;
+  }
+  void set_behind_portal() {
+    state_ = STATE_PORTAL;
+  }
   void set_connected() {
     state_ = STATE_ONLINE;
   }
@@ -644,6 +658,8 @@ class Network {
   // Not all properties in this map are exposed via get methods.
   PropertyMap property_map_;
 
+  bool is_behind_portal_for_testing_;
+
   DISALLOW_COPY_AND_ASSIGN(Network);
 };
 
diff --git a/chrome/browser/chromeos/cros/network_library_impl_stub.cc b/chrome/browser/chromeos/cros/network_library_impl_stub.cc
index df16669..97fb500 100644
--- a/chrome/browser/chromeos/cros/network_library_impl_stub.cc
+++ b/chrome/browser/chromeos/cros/network_library_impl_stub.cc
@@ -420,7 +420,10 @@ void NetworkLibraryImplStub::ConnectToNetwork(Network* network) {
   }
 
   // Set connected state.
-  network->set_connected();
+  if (network->is_behind_portal_for_testing())
+    network->set_behind_portal();
+  else
+    network->set_connected();
   network->set_user_connect_state(USER_CONNECT_CONNECTED);
 
   // Make the connected network the highest priority network.
diff --git a/chrome/browser/chromeos/net/network_portal_detector.cc b/chrome/browser/chromeos/net/network_portal_detector.cc
index db9580b..8c7f0ec 100644
--- a/chrome/browser/chromeos/net/network_portal_detector.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector.cc
@@ -55,14 +55,14 @@ NetworkPortalDetector* g_network_portal_detector = NULL;
 
 NetworkPortalDetector::NetworkPortalDetector(
     const scoped_refptr<net::URLRequestContextGetter>& request_context)
-    : test_url_(CaptivePortalDetector::kDefaultURL),
+    : active_connection_state_(STATE_UNKNOWN),
+      test_url_(CaptivePortalDetector::kDefaultURL),
       weak_ptr_factory_(this),
       attempt_count_(0),
       min_time_between_attempts_(
           base::TimeDelta::FromSeconds(kMinTimeBetweenAttemptsSec)),
       request_timeout_(base::TimeDelta::FromSeconds(kRequestTimeoutSec)) {
-  captive_portal_detector_.reset(
-      new CaptivePortalDetector(request_context));
+  captive_portal_detector_.reset(new CaptivePortalDetector(request_context));
 }
 
 NetworkPortalDetector::~NetworkPortalDetector() {
@@ -70,15 +70,19 @@ NetworkPortalDetector::~NetworkPortalDetector() {
 
 void NetworkPortalDetector::Init() {
   DCHECK(CalledOnValidThread());
+  DCHECK(chromeos::CrosLibrary::Get());
 
   state_ = STATE_IDLE;
   chromeos::NetworkLibrary* network_library =
       chromeos::CrosLibrary::Get()->GetNetworkLibrary();
+  DCHECK(network_library);
   network_library->AddNetworkManagerObserver(this);
+  network_library->RemoveObserverForAllNetworks(this);
 }
 
 void NetworkPortalDetector::Shutdown() {
   DCHECK(CalledOnValidThread());
+  DCHECK(chromeos::CrosLibrary::Get());
 
   detection_task_.Cancel();
   detection_timeout_.Cancel();
@@ -88,7 +92,8 @@ void NetworkPortalDetector::Shutdown() {
   observers_.Clear();
   chromeos::NetworkLibrary* network_library =
       chromeos::CrosLibrary::Get()->GetNetworkLibrary();
-  network_library->RemoveNetworkManagerObserver(this);
+  if (network_library)
+    network_library->RemoveNetworkManagerObserver(this);
 }
 
 void NetworkPortalDetector::AddObserver(Observer* observer) {
@@ -111,7 +116,7 @@ NetworkPortalDetector::GetCaptivePortalState(const Network* network) {
   if (!network)
     return CAPTIVE_PORTAL_STATE_UNKNOWN;
   CaptivePortalStateMap::const_iterator it =
-      captive_portal_state_map_.find(network->unique_id());
+      captive_portal_state_map_.find(network->service_path());
   if (it == captive_portal_state_map_.end())
     return CAPTIVE_PORTAL_STATE_UNKNOWN;
   return it->second;
@@ -120,18 +125,26 @@ NetworkPortalDetector::GetCaptivePortalState(const Network* network) {
 void NetworkPortalDetector::OnNetworkManagerChanged(NetworkLibrary* cros) {
   DCHECK(CalledOnValidThread());
 
-  // Suppose that if there are no active network, then unique id is
-  // empty and connection state is unknown.
-  std::string new_active_network_id;
-  ConnectionState connection_state = STATE_UNKNOWN;
   const Network* active_network = cros->active_network();
-  if (active_network) {
-    new_active_network_id = active_network->unique_id();
-    connection_state = active_network->connection_state();
+  if (!active_network)
+    return;
+
+  active_network_id_ = active_network->unique_id();
+
+  bool network_changed =
+      (active_service_path_ != active_network->service_path());
+  if (network_changed) {
+    if (!active_service_path_.empty())
+      cros->RemoveNetworkObserver(active_service_path_, this);
+    active_service_path_ = active_network->service_path();
+    cros->AddNetworkObserver(active_service_path_, this);
   }
 
-  if (active_network_id_ != new_active_network_id) {
-    active_network_id_ = new_active_network_id;
+  bool connection_state_changed =
+      (active_connection_state_ != active_network->connection_state());
+  active_connection_state_ = active_network->connection_state();
+
+  if (network_changed || connection_state_changed) {
     attempt_count_ = 0;
     if (IsPortalCheckPending()) {
       detection_task_.Cancel();
@@ -141,22 +154,30 @@ void NetworkPortalDetector::OnNetworkManagerChanged(NetworkLibrary* cros) {
     }
     state_ = STATE_IDLE;
   }
+
   if (!IsCheckingForPortal() && !IsPortalCheckPending() &&
-      Network::IsConnectedState(connection_state) &&
+      Network::IsConnectedState(active_connection_state_) &&
       attempt_count_ < kMaxRequestAttempts) {
     DCHECK(active_network);
 
-    // Initiate Captive Portal detection only if network's captive
-    // portal state is unknown (e.g. for freshly created networks) or
-    // offline.
+    // Initiate Captive Portal detection if network's captive
+    // portal state is unknown (e.g. for freshly created networks),
+    // offline or if network connection state was changed.
     CaptivePortalState state = GetCaptivePortalState(active_network);
     if (state == CAPTIVE_PORTAL_STATE_UNKNOWN ||
-        state == CAPTIVE_PORTAL_STATE_OFFLINE) {
+        state == CAPTIVE_PORTAL_STATE_OFFLINE ||
+        (!network_changed && connection_state_changed)) {
       DetectCaptivePortal(base::TimeDelta());
     }
   }
 }
 
+void NetworkPortalDetector::OnNetworkChanged(chromeos::NetworkLibrary* cros,
+                                             const chromeos::Network* network) {
+  DCHECK(CalledOnValidThread());
+  OnNetworkManagerChanged(cros);
+}
+
 // static
 NetworkPortalDetector* NetworkPortalDetector::CreateInstance() {
   DCHECK(!g_network_portal_detector);
@@ -214,8 +235,7 @@ void NetworkPortalDetector::DetectCaptivePortalTask() {
   ++attempt_count_;
   attempt_start_time_ = GetCurrentTimeTicks();
 
-  VLOG(1) << "Portal detection started: "
-          << "network=" << active_network_id_ << ", "
+  VLOG(1) << "Portal detection started: network=" << active_network_id_ << ", "
           << "attempt=" << attempt_count_ << " of " << kMaxRequestAttempts;
 
   captive_portal_detector_->DetectCaptivePortal(
@@ -291,13 +311,13 @@ void NetworkPortalDetector::SetCaptivePortalState(const Network* network,
   DCHECK(network);
 
   CaptivePortalStateMap::const_iterator it =
-      captive_portal_state_map_.find(network->unique_id());
+      captive_portal_state_map_.find(network->service_path());
   if (it == captive_portal_state_map_.end() ||
       it->second != state) {
     VLOG(1) << "Updating Chrome Captive Portal state: "
             << "network=" << network->unique_id() << ", "
             << "state=" << CaptivePortalStateString(state);
-    captive_portal_state_map_[network->unique_id()] = state;
+    captive_portal_state_map_[network->service_path()] = state;
     NotifyPortalStateChanged(network, state);
   }
 }
diff --git a/chrome/browser/chromeos/net/network_portal_detector.h b/chrome/browser/chromeos/net/network_portal_detector.h
index 0d74750..af904b8 100644
--- a/chrome/browser/chromeos/net/network_portal_detector.h
+++ b/chrome/browser/chromeos/net/network_portal_detector.h
@@ -30,7 +30,8 @@ namespace chromeos {
 // network to CaptivePortalService.
 class NetworkPortalDetector
     : public base::NonThreadSafe,
-      public chromeos::NetworkLibrary::NetworkManagerObserver {
+      public chromeos::NetworkLibrary::NetworkManagerObserver,
+      public chromeos::NetworkLibrary::NetworkObserver {
  public:
   enum CaptivePortalState {
     CAPTIVE_PORTAL_STATE_UNKNOWN  = 0,
@@ -62,6 +63,10 @@ class NetworkPortalDetector
   // NetworkLibrary::NetworkManagerObserver implementation:
   virtual void OnNetworkManagerChanged(chromeos::NetworkLibrary* cros) OVERRIDE;
 
+  // NetworkLibrary::NetworkObserver implementation:
+  virtual void OnNetworkChanged(chromeos::NetworkLibrary* cros,
+                                const chromeos::Network* network) OVERRIDE;
+
   // Creates an instance of the NetworkPortalDetector.
   static NetworkPortalDetector* CreateInstance();
 
@@ -154,7 +159,15 @@ class NetworkPortalDetector
     time_ticks_for_testing_ += delta;
   }
 
+  // Unique identifier of the active network.
   std::string active_network_id_;
+
+  // Service path of the active network.
+  std::string active_service_path_;
+
+  // Connection state of the active network.
+  ConnectionState active_connection_state_;
+
   State state_;
   CaptivePortalStateMap captive_portal_state_map_;
   ObserverList<Observer> observers_;
diff --git a/chrome/browser/chromeos/net/network_portal_detector_unittest.cc b/chrome/browser/chromeos/net/network_portal_detector_unittest.cc
index d235f63..b7899db 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_unittest.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector_unittest.cc
@@ -103,6 +103,14 @@ class NetworkPortalDetectorTest
     network_portal_detector()->set_time_ticks_for_testing(time_ticks);
   }
 
+  void SetBehindPortal(Network* network) {
+    Network::TestApi test_api(network);
+        test_api.SetBehindPortal();
+    static_cast<NetworkLibraryImplBase*>(
+        network_library())->CallConnectToNetwork(network);
+    MessageLoop::current()->RunUntilIdle();
+  }
+
   void SetConnected(Network* network) {
     Network::TestApi test_api(network);
     test_api.SetConnected();
@@ -252,10 +260,35 @@ TEST_F(NetworkPortalDetectorTest, NetworkStateNotChanged) {
 }
 
 TEST_F(NetworkPortalDetectorTest, NetworkStateChanged) {
-  // TODO (ygorshenin): currently portal detection isn't invoked if
-  // previous detection results are PORTAL or ONLINE. We're planning
-  // to change this behaviour in future, so, there should be
-  // corresponding test.
+  // Test for Portal -> Online -> Portal network state transitions.
+  ASSERT_TRUE(is_state_idle());
+
+  SetBehindPortal(wifi1_network());
+  ASSERT_TRUE(is_state_checking_for_portal());
+
+  CompleteURLFetch(net::OK, 200, NULL);
+
+  ASSERT_TRUE(is_state_idle());
+  ASSERT_EQ(NetworkPortalDetector::CAPTIVE_PORTAL_STATE_PORTAL,
+            network_portal_detector()->GetCaptivePortalState(wifi1_network()));
+
+  SetConnected(wifi1_network());
+  ASSERT_TRUE(is_state_checking_for_portal());
+
+  CompleteURLFetch(net::OK, 204, NULL);
+
+  ASSERT_TRUE(is_state_idle());
+  ASSERT_EQ(NetworkPortalDetector::CAPTIVE_PORTAL_STATE_ONLINE,
+            network_portal_detector()->GetCaptivePortalState(wifi1_network()));
+
+  SetBehindPortal(wifi1_network());
+  ASSERT_TRUE(is_state_checking_for_portal());
+
+  CompleteURLFetch(net::OK, 200, NULL);
+
+  ASSERT_TRUE(is_state_idle());
+  ASSERT_EQ(NetworkPortalDetector::CAPTIVE_PORTAL_STATE_PORTAL,
+            network_portal_detector()->GetCaptivePortalState(wifi1_network()));
 }
 
 TEST_F(NetworkPortalDetectorTest, PortalDetectionTimeout) {