Network config: Ignore unavailable device states and update enabling

If a technology gets left in the 'enabling' (or any other) list
but becomes unavailable, it should not be provided as an available
technology. Also, 'prohibited' technologies are correctly prioritized.

This CL also modernizes the technology list iterators and only signals
a change when the list actually changes.

Bug: 809940
Change-Id: I9aa60ea5027f74b52d831e3a872c8ce7ffccb863
Reviewed-on: https://chromium-review.googlesource.com/1186097Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590341}

chromeos/network/network_state_handler.cc

@@ -41,7 +41,7 @@ bool ConnectionStateChanged(const NetworkState* network,
   if (network->is_captive_portal() != prev_is_captive_portal)
     return true;
   std::string connection_state = network->connection_state();
-  // Treat 'idle' and 'discoonect' the same.
+  // Treat 'idle' and 'disconnect' the same.
   bool prev_idle = prev_connection_state.empty() ||
                    prev_connection_state == shill::kStateIdle ||
                    prev_connection_state == shill::kStateDisconnect;
@@ -108,7 +108,7 @@ NetworkStateHandler::NetworkStateHandler() {
 NetworkStateHandler::~NetworkStateHandler() {
   // Normally Shutdown() will get called in ~NetworkHandler, however unit
   // tests do not use that class so this needs to call Shutdown when we
-  // destry the class.
+  // destroy the class.
   if (!did_shutdown_)
     Shutdown();
 }
@@ -193,21 +193,28 @@ NetworkStateHandler::TechnologyState NetworkStateHandler::GetTechnologyState(
     return tether_technology_state_;
   }
 
-  TechnologyState state;
+  // If a technology is not in Shill's 'AvailableTechnologies' list, it is
+  // always unavailable.
+  if (!shill_property_handler_->IsTechnologyAvailable(technology))
+    return TECHNOLOGY_UNAVAILABLE;
+
+  // Prohibited should take precedence over other states.
+  if (shill_property_handler_->IsTechnologyProhibited(technology))
+    return TECHNOLOGY_PROHIBITED;
+
+  // Enabled and Uninitialized should be mutually exclusive. 'Enabling', which
+  // is a pseudo state used by the UI, takes precedence over 'Uninitialized',
+  // but not 'Enabled'.
   if (shill_property_handler_->IsTechnologyEnabled(technology))
-    state = TECHNOLOGY_ENABLED;
-  else if (shill_property_handler_->IsTechnologyEnabling(technology))
-    state = TECHNOLOGY_ENABLING;
-  else if (shill_property_handler_->IsTechnologyProhibited(technology))
-    state = TECHNOLOGY_PROHIBITED;
-  else if (shill_property_handler_->IsTechnologyUninitialized(technology))
-    state = TECHNOLOGY_UNINITIALIZED;
-  else if (shill_property_handler_->IsTechnologyAvailable(technology))
-    state = TECHNOLOGY_AVAILABLE;
-  else
-    state = TECHNOLOGY_UNAVAILABLE;
-  VLOG(2) << "GetTechnologyState: " << type.ToDebugString() << " = " << state;
-  return state;
+    return TECHNOLOGY_ENABLED;
+  if (shill_property_handler_->IsTechnologyEnabling(technology))
+    return TECHNOLOGY_ENABLING;
+  if (shill_property_handler_->IsTechnologyUninitialized(technology))
+    return TECHNOLOGY_UNINITIALIZED;
+
+  // Default state is 'Available', which is equivalent to 'Initialized but not
+  // enabled'.
+  return TECHNOLOGY_AVAILABLE;
 }
 
 void NetworkStateHandler::SetTechnologyEnabled(
@@ -1863,7 +1870,7 @@ std::string NetworkStateHandler::GetTechnologyForType(
   if (type.Equals(NetworkTypePattern::Wimax()))
     return shill::kTypeWimax;
 
-  // Prefer Wimax over Cellular only if it's available.
+  // Prefer WiMAX over Cellular only if it's available.
   if (type.MatchesType(shill::kTypeWimax) &&
       shill_property_handler_->IsTechnologyAvailable(shill::kTypeWimax)) {
     return shill::kTypeWimax;

chromeos/network/network_state_handler_unittest.cc

@@ -700,11 +700,11 @@ TEST_F(NetworkStateHandlerTest, TechnologyChanged) {
       NetworkStateHandler::TECHNOLOGY_AVAILABLE,
       network_state_handler_->GetTechnologyState(NetworkTypePattern::WiFi()));
 
-  // Run the message loop. An additional notification will be received when
-  // Shill updates the enabled technologies. The state should remain AVAILABLE.
+  // Run the message loop. No additional notification should be received when
+  // Shill updates the enabled technologies since the state remains AVAILABLE.
   test_observer_->reset_change_counts();
   base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1u, test_observer_->device_list_changed_count());
+  EXPECT_EQ(0u, test_observer_->device_list_changed_count());
   EXPECT_EQ(
       NetworkStateHandler::TECHNOLOGY_AVAILABLE,
       network_state_handler_->GetTechnologyState(NetworkTypePattern::WiFi()));

chromeos/network/shill_property_handler.cc

@@ -430,13 +430,20 @@ void ShillPropertyHandler::UpdateObserved(ManagedState::ManagedType type,
 void ShillPropertyHandler::UpdateAvailableTechnologies(
     const base::ListValue& technologies) {
   NET_LOG(EVENT) << "AvailableTechnologies:" << technologies;
-  available_technologies_.clear();
-  for (base::ListValue::const_iterator iter = technologies.begin();
-       iter != technologies.end(); ++iter) {
-    std::string technology;
-    iter->GetAsString(&technology);
-    DCHECK(!technology.empty());
-    available_technologies_.insert(technology);
+  std::set<std::string> new_available_technologies;
+  for (const base::Value& technology : technologies.GetList())
+    new_available_technologies.insert(technology.GetString());
+  if (new_available_technologies == available_technologies_)
+    return;
+  available_technologies_.swap(new_available_technologies);
+  // If any entries in |enabling_technologies_| are no longer available,
+  // remove them from the enabling list.
+  for (auto iter = enabling_technologies_.begin();
+       iter != enabling_technologies_.end();) {
+    if (!available_technologies_.count(*iter))
+      iter = enabling_technologies_.erase(iter);
+    else
+      ++iter;
   }
   listener_->TechnologyListChanged();
 }
@@ -444,14 +451,20 @@ void ShillPropertyHandler::UpdateAvailableTechnologies(
 void ShillPropertyHandler::UpdateEnabledTechnologies(
     const base::ListValue& technologies) {
   NET_LOG(EVENT) << "EnabledTechnologies:" << technologies;
-  enabled_technologies_.clear();
-  for (base::ListValue::const_iterator iter = technologies.begin();
-       iter != technologies.end(); ++iter) {
-    std::string technology;
-    iter->GetAsString(&technology);
-    DCHECK(!technology.empty());
-    enabled_technologies_.insert(technology);
-    enabling_technologies_.erase(technology);
+  std::set<std::string> new_enabled_technologies;
+  for (const base::Value& technology : technologies.GetList())
+    new_enabled_technologies.insert(technology.GetString());
+  if (new_enabled_technologies == enabled_technologies_)
+    return;
+  enabled_technologies_.swap(new_enabled_technologies);
+  // If any entries in |enabling_technologies_| are enabled, remove them from
+  // the enabling list.
+  for (auto iter = enabling_technologies_.begin();
+       iter != enabling_technologies_.end();) {
+    if (enabled_technologies_.count(*iter))
+      iter = enabling_technologies_.erase(iter);
+    else
+      ++iter;
   }
   listener_->TechnologyListChanged();
 }
@@ -459,14 +472,12 @@ void ShillPropertyHandler::UpdateEnabledTechnologies(
 void ShillPropertyHandler::UpdateUninitializedTechnologies(
     const base::ListValue& technologies) {
   NET_LOG(EVENT) << "UninitializedTechnologies:" << technologies;
-  uninitialized_technologies_.clear();
-  for (base::ListValue::const_iterator iter = technologies.begin();
-       iter != technologies.end(); ++iter) {
-    std::string technology;
-    iter->GetAsString(&technology);
-    DCHECK(!technology.empty());
-    uninitialized_technologies_.insert(technology);
-  }
+  std::set<std::string> new_uninitialized_technologies;
+  for (const base::Value& technology : technologies.GetList())
+    new_uninitialized_technologies.insert(technology.GetString());
+  if (new_uninitialized_technologies == uninitialized_technologies_)
+    return;
+  uninitialized_technologies_.swap(new_uninitialized_technologies);
   listener_->TechnologyListChanged();
 }