Don't delete networks that have failed until user has been notified.

BUG=chromium-os:9607
TEST=Test connecting to networks, including hidden and bogus 'Other' networks. Notification should always occur on failure. Bogus networks should still be removed from list after failure notification. Test normaly and while constantly clicking on network menu (refreshing state).

Review URL: http://codereview.chromium.org/6880097

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

6 files changed, 79 insertions, 179 deletions

diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index c83ccb0..206d675 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -942,6 +942,21 @@ void NetworkDevice::ParseInfo(const DictionaryValue* info) {
 ////////////////////////////////////////////////////////////////////////////////
 // Network
 
+void Network::SetState(ConnectionState new_state) {
+  if (new_state == state_)
+    return;
+  state_ = new_state;
+  if (new_state == STATE_FAILURE) {
+    // The user needs to be notified of this failure.
+    notify_failure_ = true;
+  } else {
+    // State changed, so refresh IP address.
+    // Note: blocking DBus call. TODO(stevenjb): refactor this.
+    InitIPAddress();
+  }
+  VLOG(1) << " " << name() << ".State = " << GetStateString();
+}
+
 bool Network::ParseValue(int index, const Value* value) {
   switch (index) {
     case PROPERTY_INDEX_TYPE: {
@@ -962,13 +977,7 @@ bool Network::ParseValue(int index, const Value* value) {
     case PROPERTY_INDEX_STATE: {
       std::string state_string;
       if (value->GetAsString(&state_string)) {
-        ConnectionState prev_state = state_;
-        state_ = ParseState(state_string);
-        if (state_ != prev_state) {
-          // State changed, so refresh IP address.
-          // Note: blocking DBus call. TODO(stevenjb): refactor this.
-          InitIPAddress();
-        }
+        SetState(ParseState(state_string));
         return true;
       }
       break;
@@ -2476,7 +2485,7 @@ class NetworkLibraryImpl : public NetworkLibrary  {
           strcmp(error_message, kErrorPassphraseRequiredMsg) == 0) {
         // This will trigger the connection failed notification.
         // TODO(stevenjb): Remove if chromium-os:13203 gets fixed.
-        network->set_state(STATE_FAILURE);
+        network->SetState(STATE_FAILURE);
         network->set_error(ERROR_BAD_PASSPHRASE);
         networklib->NotifyNetworkManagerChanged(true);  // Forced update.
       }
@@ -3094,14 +3103,11 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     NetworkLibraryImpl* networklib = static_cast<NetworkLibraryImpl*>(object);
     DCHECK(networklib);
     if (service_path) {
-      if (!info) {
-        // Network no longer exists.
-        networklib->DeleteNetwork(std::string(service_path));
-      } else {
-        DCHECK_EQ(info->GetType(), Value::TYPE_DICTIONARY);
-        const DictionaryValue* dict = static_cast<const DictionaryValue*>(info);
-        networklib->ParseNetwork(std::string(service_path), dict);
-      }
+      if (!info)
+        return;  // Network no longer in visible list, ignore.
+      DCHECK_EQ(info->GetType(), Value::TYPE_DICTIONARY);
+      const DictionaryValue* dict = static_cast<const DictionaryValue*>(info);
+      networklib->ParseNetwork(std::string(service_path), dict);
     }
   }
 
@@ -3235,6 +3241,7 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     std::pair<NetworkMap::iterator,bool> result =
         network_map_.insert(std::make_pair(network->service_path(), network));
     DCHECK(result.second);  // Should only get called with new network.
+    VLOG(2) << "Adding Network: " << network->name();
     ConnectionType type(network->type());
     if (type == TYPE_WIFI) {
       if (wifi_enabled())
@@ -3248,63 +3255,19 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     // Do not set the active network here. Wait until we parse the network.
   }
 
-  // This only gets called when NetworkServiceUpdate receives a NULL update
-  // for an existing network, e.g. an error occurred while fetching a network.
-  void DeleteNetwork(const std::string& service_path) {
-    NetworkMap::iterator found = network_map_.find(service_path);
-    if (found == network_map_.end()) {
-      // This occurs when we receive an update request followed by a disconnect
-      // which triggers another update. See UpdateNetworkServiceList.
-      return;
-    }
-    Network* network = found->second;
-    network_map_.erase(found);
-    if (!network->unique_id().empty())
-      network_unique_id_map_.erase(network->unique_id());
+  // Deletes a network. It must already be removed from any lists.
+  void DeleteNetwork(Network* network) {
+    CHECK(network_map_.find(network->service_path()) == network_map_.end());
     ConnectionType type(network->type());
-    if (type == TYPE_ETHERNET) {
-      if (network == ethernet_) {
-        // This should never happen.
-        LOG(ERROR) << "Deleting active ethernet network: " << service_path;
-        ethernet_ = NULL;
-      }
-    } else if (type == TYPE_WIFI) {
-      WifiNetworkVector::iterator iter = std::find(
-          wifi_networks_.begin(), wifi_networks_.end(), network);
-      if (iter != wifi_networks_.end())
-        wifi_networks_.erase(iter);
-      if (network == active_wifi_) {
-        // This should never happen.
-        LOG(ERROR) << "Deleting active wifi network: " << service_path;
-        active_wifi_ = NULL;
-      }
-    } else if (type == TYPE_CELLULAR) {
-      CellularNetworkVector::iterator iter = std::find(
-          cellular_networks_.begin(), cellular_networks_.end(), network);
-      if (iter != cellular_networks_.end())
-        cellular_networks_.erase(iter);
-      if (network == active_cellular_) {
-        // This should never happen.
-        LOG(ERROR) << "Deleting active cellular network: " << service_path;
-        active_cellular_ = NULL;
-      }
+    if (type == TYPE_CELLULAR) {
       // Find and delete any existing data plans associated with |service_path|.
-      CellularDataPlanMap::iterator found =  data_plan_map_.find(service_path);
+      CellularDataPlanMap::iterator found =
+          data_plan_map_.find(network->service_path());
       if (found != data_plan_map_.end()) {
         CellularDataPlanVector* data_plans = found->second;
         delete data_plans;
         data_plan_map_.erase(found);
       }
-    } else if (type == TYPE_VPN) {
-      VirtualNetworkVector::iterator iter = std::find(
-          virtual_networks_.begin(), virtual_networks_.end(), network);
-      if (iter != virtual_networks_.end())
-        virtual_networks_.erase(iter);
-      if (network == active_virtual_) {
-        // This should never happen.
-        LOG(ERROR) << "Deleting active virtual network: " << service_path;
-        active_virtual_ = NULL;
-      }
     }
     delete network;
   }
@@ -3381,10 +3344,23 @@ class NetworkLibraryImpl : public NetworkLibrary  {
                                   this);
       }
     }
-    // Delete any old networks that no longer exist.
+    // Iterate through list of remaining networks that are no longer in the
+    // list and delete them or update their status and re-add them to the list.
     for (NetworkMap::iterator iter = old_network_map.begin();
          iter != old_network_map.end(); ++iter) {
-      delete iter->second;
+      Network* network = iter->second;
+      if (network->failed() && network->notify_failure()) {
+        // We have not notified observers of a connection failure yet.
+        AddNetwork(network);
+      } else if (network->connecting()) {
+        // Network was in connecting state; set state to failed.
+        network->SetState(STATE_FAILURE);
+        AddNetwork(network);
+      } else {
+        VLOG(2) << "Deleting non-existant Network: " << network->name()
+                << " State = " << network->GetStateString();
+        DeleteNetwork(network);
+      }
     }
   }
 
@@ -3696,6 +3672,11 @@ class NetworkLibraryImpl : public NetworkLibrary  {
     FOR_EACH_OBSERVER(NetworkManagerObserver,
                       network_manager_observers_,
                       OnNetworkManagerChanged(this));
+    // Clear notification flags.
+    for (NetworkMap::iterator iter = network_map_.begin();
+         iter != network_map_.end(); ++iter) {
+      iter->second->set_notify_failure(false);
+    }
   }
 
   void NotifyNetworkChanged(Network* network) {
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index b79acc3..6153dcf 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -287,9 +287,12 @@ class Network {
   bool auto_connect() const { return auto_connect_; }
   ConnectivityState connectivity_state() const { return connectivity_state_; }
   bool added() const { return added_; }
+  bool notify_failure() const { return notify_failure_; }
 
   const std::string& unique_id() const { return unique_id_; }
 
+  void set_notify_failure(bool state) { notify_failure_ = state; }
+
   // We don't have a setter for |favorite_| because to unfavorite a network is
   // equivalent to forget a network, so we call forget network on cros for
   // that.  See ForgetWifiNetwork().
@@ -318,6 +321,7 @@ class Network {
         connectivity_state_(CONN_STATE_UNKNOWN),
         priority_order_(0),
         added_(false),
+        notify_failure_(false),
         service_path_(service_path),
         type_(type) {}
 
@@ -361,7 +365,6 @@ class Network {
   void set_connected(bool connected) {
     state_ = (connected ? STATE_READY : STATE_IDLE);
   }
-  void set_state(ConnectionState state) { state_ = state; }
   void set_connectable(bool connectable) { connectable_ = connectable; }
   void set_active(bool is_active) { is_active_ = is_active; }
   void set_error(ConnectionError error) { error_ = error; }
@@ -370,6 +373,9 @@ class Network {
   }
   void set_added(bool added) { added_ = added; }
 
+  // Set the state and update flags if necessary.
+  void SetState(ConnectionState state);
+
   // Initialize the IP address field
   void InitIPAddress();
 
@@ -379,6 +385,10 @@ class Network {
   // Set to true if the UI requested this as a new network.
   bool added_;
 
+  // Set to true when a new connection failure occurs; cleared when observers
+  // are notified.
+  bool notify_failure_;
+
   // These must not be modified after construction.
   std::string service_path_;
   ConnectionType type_;
diff --git a/chrome/browser/chromeos/network_login_observer.cc b/chrome/browser/chromeos/network_login_observer.cc
index 9fa09d0..3325b83 100644
--- a/chrome/browser/chromeos/network_login_observer.cc
+++ b/chrome/browser/chromeos/network_login_observer.cc
@@ -19,8 +19,6 @@
 namespace chromeos {
 
 NetworkLoginObserver::NetworkLoginObserver(NetworkLibrary* netlib) {
-  RefreshStoredNetworks(netlib->wifi_networks(),
-                        netlib->virtual_networks());
   netlib->AddNetworkManagerObserver(this);
 }
 
@@ -49,85 +47,39 @@ void NetworkLoginObserver::CreateModalPopup(views::WindowDelegate* view) {
   }
 }
 
-void NetworkLoginObserver::RefreshStoredNetworks(
-    const WifiNetworkVector& wifi_networks,
-    const VirtualNetworkVector& virtual_networks) {
-  network_failures_.clear();
-  for (WifiNetworkVector::const_iterator it = wifi_networks.begin();
-       it != wifi_networks.end(); it++) {
-    const WifiNetwork* wifi = *it;
-    network_failures_[wifi->service_path()] = wifi->failed();
-  }
-  for (VirtualNetworkVector::const_iterator it = virtual_networks.begin();
-       it != virtual_networks.end(); it++) {
-    const VirtualNetwork* vpn = *it;
-    network_failures_[vpn->service_path()] = vpn->failed();
-  }
-}
-
 void NetworkLoginObserver::OnNetworkManagerChanged(NetworkLibrary* cros) {
   const WifiNetworkVector& wifi_networks = cros->wifi_networks();
   const VirtualNetworkVector& virtual_networks = cros->virtual_networks();
 
-  NetworkConfigView* view = NULL;
   // Check to see if we have any newly failed wifi network.
   for (WifiNetworkVector::const_iterator it = wifi_networks.begin();
        it != wifi_networks.end(); it++) {
     WifiNetwork* wifi = *it;
-    if (wifi->failed()) {
-      NetworkFailureMap::iterator iter =
-          network_failures_.find(wifi->service_path());
-      // If the network did not previously exist, then don't do anything.
-      // For example, if the user travels to a location and finds a service
-      // that has previously failed, we don't want to show an error.
-      if (iter == network_failures_.end())
-        continue;
-
-      // If this network was in a failed state previously, then it's not new.
-      if (iter->second)
-        continue;
-
+    if (wifi->notify_failure()) {
       // Display login dialog again for bad_passphrase and bad_wepkey errors.
-      // Always re-display the login dialog for added networks that failed to
-      // connect for any reason (e.g. incorrect security type was chosen).
+      // Always re-display the login dialog for encrypted networks that were
+      // added and failed to connect for any reason.
       if (wifi->error() == ERROR_BAD_PASSPHRASE ||
           wifi->error() == ERROR_BAD_WEPKEY ||
-          wifi->added()) {
-        // The NetworkConfigView will show the appropriate error message.
-        view = new NetworkConfigView(wifi);
-        // There should only be one wifi network that failed to connect.
-        // If for some reason, we have more than one failure,
-        // we only display the first one. So we break here.
-        break;
+          (wifi->encrypted() && wifi->added())) {
+        CreateModalPopup(new NetworkConfigView(wifi));
+        return;  // Only support one failure per notification.
       }
     }
   }
   // Check to see if we have any newly failed virtual network.
-  // See wifi section for detailed comments.
-  if (!view) {
-    for (VirtualNetworkVector::const_iterator it = virtual_networks.begin();
-         it != virtual_networks.end(); it++) {
-      VirtualNetwork* vpn = *it;
-      if (vpn->failed()) {
-        NetworkFailureMap::iterator iter =
-            network_failures_.find(vpn->service_path());
-        if (iter == network_failures_.end())
-          continue;  // New network.
-        if (iter->second)
-          continue;  // Previous failure.
-        if (vpn->error() == ERROR_BAD_PASSPHRASE || vpn->added()) {
-          view = new NetworkConfigView(vpn);
-          break;
-        }
+  for (VirtualNetworkVector::const_iterator it = virtual_networks.begin();
+       it != virtual_networks.end(); it++) {
+    VirtualNetwork* vpn = *it;
+    if (vpn->notify_failure()) {
+      // Display login dialog again for bad_passphrase and errors.
+      // Always re-display the login dialog for newly added networks.
+      if (vpn->error() == ERROR_BAD_PASSPHRASE || vpn->added()) {
+        CreateModalPopup(new NetworkConfigView(vpn));
+        return;  // Only support one failure per notification.
       }
     }
   }
-
-  RefreshStoredNetworks(wifi_networks, virtual_networks);
-
-  // Show login box if necessary.
-  if (view)
-    CreateModalPopup(view);
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/network_login_observer.h b/chrome/browser/chromeos/network_login_observer.h
index 34970aa..5057211 100644
--- a/chrome/browser/chromeos/network_login_observer.h
+++ b/chrome/browser/chromeos/network_login_observer.h
@@ -27,17 +27,11 @@ class NetworkLoginObserver : public NetworkLibrary::NetworkManagerObserver {
 
   typedef std::map<std::string, bool> NetworkFailureMap;
  private:
-  virtual void CreateModalPopup(views::WindowDelegate* view);
-
-  virtual void RefreshStoredNetworks(const WifiNetworkVector& wifi_networks,
-                                     const VirtualNetworkVector& vpn_networks);
+  void CreateModalPopup(views::WindowDelegate* view);
 
   // NetworkLibrary::NetworkManagerObserver implementation.
   virtual void OnNetworkManagerChanged(NetworkLibrary* obj);
 
-  // Wifi networks by service path mapped to if it failed previously.
-  NetworkFailureMap network_failures_;
-
   DISALLOW_COPY_AND_ASSIGN(NetworkLoginObserver);
 };
 
diff --git a/chrome/browser/chromeos/network_message_observer.cc b/chrome/browser/chromeos/network_message_observer.cc
index 77ab706..fa5b8cb 100644
--- a/chrome/browser/chromeos/network_message_observer.cc
+++ b/chrome/browser/chromeos/network_message_observer.cc
@@ -160,33 +160,13 @@ void NetworkMessageObserver::ShowLowDataNotification(
       false, false);
 }
 
-bool NetworkMessageObserver::CheckNetworkFailed(const Network* network) {
-  if (network->failed()) {
-    NetworkStateMap::iterator iter =
-        network_states_.find(network->service_path());
-    // If the network did not previously exist, then don't do anything.
-    // For example, if the user travels to a location and finds a service
-    // that has previously failed, we don't want to show a notification.
-    if (iter == network_states_.end())
-      return false;
-    // If network connection failed, display a notification.
-    // We only do this if we were trying to make a new connection.
-    // So if a previously connected network got disconnected for any reason,
-    // we don't display notification.
-    ConnectionState prev_state = iter->second;
-    if (Network::IsConnectingState(prev_state))
-      return true;
-  }
-  return false;
-}
-
 void NetworkMessageObserver::OnNetworkManagerChanged(NetworkLibrary* cros) {
   const Network* new_failed_network = NULL;
   // Check to see if we have any newly failed networks.
   for (WifiNetworkVector::const_iterator it = cros->wifi_networks().begin();
        it != cros->wifi_networks().end(); it++) {
     const WifiNetwork* net = *it;
-    if (CheckNetworkFailed(net)) {
+    if (net->notify_failure()) {
       new_failed_network = net;
       break;  // There should only be one failed network.
     }
@@ -197,7 +177,7 @@ void NetworkMessageObserver::OnNetworkManagerChanged(NetworkLibrary* cros) {
              cros->cellular_networks().begin();
          it != cros->cellular_networks().end(); it++) {
       const CellularNetwork* net = *it;
-      if (CheckNetworkFailed(net)) {
+      if (net->notify_failure()) {
         new_failed_network = net;
         break;  // There should only be one failed network.
       }
@@ -209,26 +189,13 @@ void NetworkMessageObserver::OnNetworkManagerChanged(NetworkLibrary* cros) {
              cros->virtual_networks().begin();
          it != cros->virtual_networks().end(); it++) {
       const VirtualNetwork* net = *it;
-      if (CheckNetworkFailed(net)) {
+      if (net->notify_failure()) {
         new_failed_network = net;
         break;  // There should only be one failed network.
       }
     }
   }
 
-  network_states_.clear();
-  for (WifiNetworkVector::const_iterator it = cros->wifi_networks().begin();
-       it != cros->wifi_networks().end(); it++)
-    network_states_[(*it)->service_path()] = (*it)->state();
-  for (CellularNetworkVector::const_iterator it =
-           cros->cellular_networks().begin();
-       it != cros->cellular_networks().end(); it++)
-    network_states_[(*it)->service_path()] = (*it)->state();
-  for (VirtualNetworkVector::const_iterator it =
-           cros->virtual_networks().begin();
-       it != cros->virtual_networks().end(); it++)
-    network_states_[(*it)->service_path()] = (*it)->state();
-
   // Show connection error notification if necessary.
   if (new_failed_network) {
     // Hide if already shown to force show it in case user has closed it.
diff --git a/chrome/browser/chromeos/network_message_observer.h b/chrome/browser/chromeos/network_message_observer.h
index fb4b010..17ee943 100644
--- a/chrome/browser/chromeos/network_message_observer.h
+++ b/chrome/browser/chromeos/network_message_observer.h
@@ -36,7 +36,6 @@ class NetworkMessageObserver : public NetworkLibrary::NetworkManagerObserver,
   virtual void ShowNeedsPlanNotification(const CellularNetwork* cellular);
   virtual void ShowNoDataNotification(CellularDataPlanType plan_type);
   virtual void ShowLowDataNotification(const CellularDataPlan* plan);
-  virtual bool CheckNetworkFailed(const Network* network);
 
   // NetworkLibrary::NetworkManagerObserver implementation.
   virtual void OnNetworkManagerChanged(NetworkLibrary* obj);
@@ -53,9 +52,6 @@ class NetworkMessageObserver : public NetworkLibrary::NetworkManagerObserver,
 
   typedef std::map<std::string, ConnectionState> NetworkStateMap;
 
-  // Network state by service path.
-  NetworkStateMap network_states_;
-
   // Current connect celluar service path.
   std::string cellular_service_path_;
   // Last cellular data plan unique id.