Chrome OS: Use Manager.DefaultService for Default Network

chromeos/network/network_state_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/network/network_state_handler.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ skipping 161 matching lines @@
 }
 
 const NetworkState* NetworkStateHandler::GetNetworkState(
     const std::string& service_path) const {
   const NetworkState* network = GetModifiableNetworkState(service_path);
   if (network && !network->update_received())
     return NULL;
   return network;
 }
 
 const NetworkState* NetworkStateHandler::DefaultNetwork() const {

[pneubeck (no reviews), 2014/02/24 09:22:58]

you dropped the check for IsConnectedState.
Considering that Mukesh is adapting Shill's side, we might still add an error
log if it's not the case (e.g. because the State wasn't updated quickly enough).

[stevenjb, 2014/02/24 23:59:42]

So, this is kind of tricky. I want to keep the return value for this consistent
with what gets passed to DefaultNetworkChanged(). So, instead of checking
connection state here, I added logging in DefaultNetworkServiceChanged() and
OnNetworkConnectionStateChanged().

-  if (network_list_.empty())
-    return NULL;
-  const NetworkState* network = network_list_.front()->AsNetworkState();
-  DCHECK(network);
-  if (!network->update_received() || !network->IsConnectedState())
-    return NULL;
-  return network;
+  return GetNetworkState(default_network_path_);

[pneubeck (no reviews), 2014/02/24 09:22:58]

should early return on default_network_path_.empty()
(it works as it is, but we shouldn't on purpose search for incorrect paths)

[stevenjb, 2014/02/24 23:59:42]

Done.

 }
 
 const FavoriteState* NetworkStateHandler::DefaultFavoriteNetwork() const {
   const NetworkState* default_network = DefaultNetwork();
   if (!default_network)
     return NULL;
   const FavoriteState* default_favorite =
       GetFavoriteState(default_network->path());
   DCHECK(default_favorite);
   DCHECK(default_favorite->update_received());
@@ skipping 331 matching lines @@
        !iter.IsAtEnd(); iter.Advance()) {
     if (network->PropertyChanged(iter.key(), iter.value()))
       network_property_updated = true;
   }
   network_property_updated |= network->InitialPropertiesReceived(properties);
   // Notify observers of NetworkState changes.
   if (network_property_updated || network->update_requested()) {
     // Signal connection state changed after all properties have been updated.
     if (ConnectionStateChanged(network, prev_connection_state))
       OnNetworkConnectionStateChanged(network);
-    NetworkPropertiesUpdated(network);
+    NET_LOG_EVENT("NetworkPropertiesUpdated", GetManagedStateLogName(network));
+    NotifyNetworkPropertiesUpdated(network);
   }
 }
 
 void NetworkStateHandler::UpdateNetworkServiceProperty(
     const std::string& service_path,
     const std::string& key,
     const base::Value& value) {
   // Update any associated FavoriteState.
   ManagedState* favorite =
       GetModifiableManagedState(&favorite_list_, service_path);
@@ skipping 21 matching lines @@
   } else {
     std::string value_str;
     value.GetAsString(&value_str);
     // Some property changes are noisy and not interesting:
     // * Wifi SignalStrength
     // * WifiFrequencyList updates
     // * Device property changes to "/" (occurs before a service is removed)
     if (key != shill::kSignalStrengthProperty &&
         key != shill::kWifiFrequencyListProperty &&
         (key != shill::kDeviceProperty || value_str != "/")) {
+      std::string log_event = "NetworkPropertyUpdated";
       // Trigger a default network update for interesting changes only.
-      if (network->path() == default_network_path_)
-        OnDefaultNetworkChanged();
-      // Log interesting event.
+      if (network->path() == default_network_path_) {
+        NotifyDefaultNetworkChanged(network);
+        log_event = "Default" + log_event;
+      }
+      // Log event.
       std::string detail = network->name() + "." + key;
       detail += " = " + network_event_log::ValueAsString(value);
       network_event_log::LogLevel log_level;
       if (key == shill::kErrorProperty || key == shill::kErrorDetailsProperty) {
         log_level = network_event_log::LOG_LEVEL_ERROR;
       } else {
         log_level = network_event_log::LOG_LEVEL_EVENT;
       }
-      NET_LOG_LEVEL(log_level, "NetworkPropertyUpdated", detail);
+      NET_LOG_LEVEL(log_level, log_event, detail);
     }
   }
 
   // All property updates signal 'NetworkPropertiesUpdated'.
-  NetworkPropertiesUpdated(network);
+  NotifyNetworkPropertiesUpdated(network);
 
   // If added to a Profile, request a full update so that a FavoriteState
   // gets created.
   if (prev_profile_path.empty() && !network->profile_path().empty())
     RequestUpdateForNetwork(service_path);
 }
 
 void NetworkStateHandler::UpdateDeviceProperty(const std::string& device_path,
                                                const std::string& key,
                                                const base::Value& value) {
@@ skipping 39 matching lines @@
 }
 
 void NetworkStateHandler::ManagedStateListChanged(
     ManagedState::ManagedType type) {
   if (type == ManagedState::MANAGED_TYPE_NETWORK) {
     // Notify observers that the list of networks has changed.
     NET_LOG_EVENT("NetworkListChanged",
                   base::StringPrintf("Size:%" PRIuS, network_list_.size()));
     FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                       NetworkListChanged());
-    // The list order may have changed, so check if the default network changed.
-    if (CheckDefaultNetworkChanged())
-      OnDefaultNetworkChanged();
     // Update UMA stats.
     UMA_HISTOGRAM_COUNTS_100("Networks.Visible", network_list_.size());
   } else if (type == ManagedState::MANAGED_TYPE_FAVORITE) {
     NET_LOG_DEBUG("FavoriteListChanged",
                   base::StringPrintf("Size:%" PRIuS, favorite_list_.size()));
     // The FavoriteState list only changes when the NetworkState list changes,
     // so no need to signal observers here again.
 
     // Update UMA stats.
     size_t shared = 0, unshared = 0;
     for (ManagedStateList::iterator iter = favorite_list_.begin();
          iter != favorite_list_.end(); ++iter) {
       FavoriteState* favorite = (*iter)->AsFavoriteState();
       if (!favorite->is_favorite())
         continue;
       if (favorite->IsPrivate())
         ++unshared;
       else
         ++shared;
     }
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedShared", shared);
     UMA_HISTOGRAM_COUNTS_100("Networks.RememberedUnshared", unshared);
   } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
     NotifyDeviceListChanged();
   } else {
     NOTREACHED();
   }
 }
 
+void NetworkStateHandler::DefaultNetworkServiceChanged(
+    const std::string& service_path) {
+  default_network_path_ = service_path;
+  const NetworkState* network = DefaultNetwork();
+  NET_LOG_EVENT("DefaultNetworkServiceChanged",
+                GetManagedStateLogName(network));
+  NotifyDefaultNetworkChanged(network);
+}
+
 //------------------------------------------------------------------------------
 // Private methods
 
 void NetworkStateHandler::NotifyDeviceListChanged() {
   NET_LOG_DEBUG("NotifyDeviceListChanged",
                 base::StringPrintf("Size:%" PRIuS, device_list_.size()));
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DeviceListChanged());
 }
 
@@ skipping 36 matching lines @@
     case ManagedState::MANAGED_TYPE_DEVICE:
       return &device_list_;
   }
   NOTREACHED();
   return NULL;
 }
 
 void NetworkStateHandler::OnNetworkConnectionStateChanged(
     NetworkState* network) {
   DCHECK(network);
-  NET_LOG_EVENT("NetworkConnectionStateChanged", base::StringPrintf(
-      "%s:%s", GetManagedStateLogName(network).c_str(),
-      network->connection_state().c_str()));
+  std::string event = "NetworkConnectionStateChanged";
+  if (network->path() == default_network_path_)
+    event = "Default" + event;
+  NET_LOG_EVENT(event + ": " + network->connection_state(),
+                GetManagedStateLogName(network));

[pneubeck (no reviews), 2014/02/24 09:22:58]

just an idea: you could also add "(default)" to the result of
GetManagedStateLogName if the network is the default one.

[stevenjb, 2014/02/24 23:59:42]

Interesting thought. I'm not sure I want to lengthen every log event for the
network though (whereas calling attention to specific events that affect the NCN
is helpful).

   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkConnectionStateChanged(network));
-  if (CheckDefaultNetworkChanged() || network->path() == default_network_path_)
-    OnDefaultNetworkChanged();
+  if (network->path() == default_network_path_)
+    NotifyDefaultNetworkChanged(network);
 }
 
-bool NetworkStateHandler::CheckDefaultNetworkChanged() {
-  std::string new_default_network_path;
-  const NetworkState* new_default_network = DefaultNetwork();
-  if (new_default_network)
-    new_default_network_path = new_default_network->path();
-  if (new_default_network_path == default_network_path_)
-    return false;
-  default_network_path_ = new_default_network_path;
-  return true;
-}
-
-void NetworkStateHandler::OnDefaultNetworkChanged() {
-  const NetworkState* default_network = DefaultNetwork();
-  NET_LOG_EVENT("DefaultNetworkChanged",
-                GetManagedStateLogName(default_network));
+void NetworkStateHandler::NotifyDefaultNetworkChanged(
+    const NetworkState* default_network) {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     DefaultNetworkChanged(default_network));
 }
 
-void NetworkStateHandler::NetworkPropertiesUpdated(
+void NetworkStateHandler::NotifyNetworkPropertiesUpdated(
     const NetworkState* network) {
   FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
                     NetworkPropertiesUpdated(network));
 }
 
 void NetworkStateHandler::ScanCompleted(const std::string& type) {
   size_t num_callbacks = scan_complete_callbacks_.count(type);
   NET_LOG_EVENT("ScanCompleted",
                 base::StringPrintf("%s:%" PRIuS, type.c_str(), num_callbacks));
   if (num_callbacks == 0)
@@ skipping 24 matching lines @@
   }
 
   if (type.MatchesType(shill::kTypeCellular))
     return shill::kTypeCellular;
 
   NOTREACHED();
   return std::string();
 }
 
 }  // namespace chromeos